Provide the pre-build hook with a JSON serialization of the derivation#424
Open
Provide the pre-build hook with a JSON serialization of the derivation#424
Conversation
…vation The hook gets the path of the derivation as an argument, but the derivation typically doesn't exist when called as a remote build.
📝 WalkthroughWalkthroughJSON serialization for derivations has been refactored to move logic from Changes
Sequence DiagramsequenceDiagram
participant Builder as Builder
participant Serializer as JSON Serializer
participant TempFile as Temp File
participant Hook as Pre-build Hook
participant Sandbox as Sandbox
Builder->>Serializer: serialize derivation to JSON
Serializer->>Serializer: construct BasicDerivation JSON
Serializer->>TempFile: write JSON to temp file
TempFile-->>Builder: return file path
Builder->>Hook: execute with NIX_DERIVATION_V4=file_path
Hook->>TempFile: read derivation JSON
TempFile-->>Hook: JSON content
Hook->>Hook: parse derivation name & features
Hook->>Sandbox: emit extra-sandbox-paths commands
Sandbox-->>Builder: apply path mappings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libstore/derivations.cc (1)
1526-1561:⚠️ Potential issue | 🟠 MajorComplete the new
BasicDerivationJSON specialization.This serializer is now public via
JSON_IMPL_WITH_XP_FEATURES(nix::BasicDerivation), but it only implementsto_json, and the emitted document still omitsinputs.drvs. That means directBasicDerivationserialization does not produce the full v4 derivation shape thatadl_serializer<Derivation>::from_jsonexpects, and any futureBasicDerivationdeserialization will be declared in the header but undefined at link time. Please either make the specialization self-contained (inputs.drvs = {}and a matchingfrom_json) or keep this helper private toDerivation::to_json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/derivations.cc` around lines 1526 - 1561, The adl_serializer<BasicDerivation>::to_json currently emits only partial fields (omitting inputs.drvs) and lacks a matching adl_serializer<BasicDerivation>::from_json, so make the specialization self-contained: update to_json (adl_serializer<BasicDerivation>::to_json) to emit an empty inputs.drvs array alongside inputs.srcs, and implement adl_serializer<BasicDerivation>::from_json to parse version, name, outputs, inputs.srcs into BasicDerivation::inputSrcs, inputs.drvs (accept empty array), platform/system, builder, args, env, and structuredAttrs (reconstructing structuredAttrs pointer if present); ensure you use the same symbols (BasicDerivation, inputSrcs, outputs, platform, builder, args, env, structuredAttrs, expectedJsonVersionDerivation) and validate JSON version matching while throwing on invalid shape.
🧹 Nitpick comments (2)
tests/functional/linux-sandbox.sh (1)
112-115: Read the canonicalnamefield in the hook test.The serializer writes the derivation name at the top level;
.env.nameonly works if that value also happens to be duplicated into the environment map. Switching this to.namemakes the test assert the new JSON contract directly.Proposed change
-jq -r .env.name < "\$NIX_DERIVATION_V4" > "$DEST" +jq -r .name < "\$NIX_DERIVATION_V4" > "$DEST"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/functional/linux-sandbox.sh` around lines 112 - 115, The test hook currently extracts the derivation name using jq -r .env.name from the NIX_DERIVATION_V4 file, but the serializer emits the canonical name at the top level; update the jq invocation in the hook script to use .name instead of .env.name (i.e., change jq -r .env.name < "\$NIX_DERIVATION_V4" > "$DEST" to jq -r .name < "\$NIX_DERIVATION_V4" > "$DEST") so the test reads the top-level name field directly (symbols: HOOK, NIX_DERIVATION_V4, DEST).src/libstore/unix/build/derivation-builder.cc (1)
938-940: Write the hook JSON through the temp fd you already created.Line 939 reopens the file by path instead of writing through
tmpFd. Using the descriptor keeps the write tied to the exact inode returned bycreateTempFile()and avoids an unnecessary second pathname lookup.Proposed change
- auto [tmpFd, drvJsonPath] = createTempFile("nix-drv-json"); - writeFile(drvJsonPath, drvJson.dump()); + auto [tmpFd, drvJsonPath] = createTempFile("nix-drv-json"); + writeFile(tmpFd, drvJsonPath, drvJson.dump());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/unix/build/derivation-builder.cc` around lines 938 - 940, The code currently reopens drvJsonPath to write the JSON, which negates the createTempFile() tmpFd guarantee; instead write the hook JSON through the existing tmpFd returned by createTempFile("nix-drv-json") (use the tmpFd to write drvJson.dump() and then close the descriptor) so the write is tied to the same inode and avoid a second pathname lookup—update the writeFile call to accept/use tmpFd (or perform a direct write to tmpFd) and keep AutoDelete(drvJsonPath, false) as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/libstore/derivations.cc`:
- Around line 1526-1561: The adl_serializer<BasicDerivation>::to_json currently
emits only partial fields (omitting inputs.drvs) and lacks a matching
adl_serializer<BasicDerivation>::from_json, so make the specialization
self-contained: update to_json (adl_serializer<BasicDerivation>::to_json) to
emit an empty inputs.drvs array alongside inputs.srcs, and implement
adl_serializer<BasicDerivation>::from_json to parse version, name, outputs,
inputs.srcs into BasicDerivation::inputSrcs, inputs.drvs (accept empty array),
platform/system, builder, args, env, and structuredAttrs (reconstructing
structuredAttrs pointer if present); ensure you use the same symbols
(BasicDerivation, inputSrcs, outputs, platform, builder, args, env,
structuredAttrs, expectedJsonVersionDerivation) and validate JSON version
matching while throwing on invalid shape.
---
Nitpick comments:
In `@src/libstore/unix/build/derivation-builder.cc`:
- Around line 938-940: The code currently reopens drvJsonPath to write the JSON,
which negates the createTempFile() tmpFd guarantee; instead write the hook JSON
through the existing tmpFd returned by createTempFile("nix-drv-json") (use the
tmpFd to write drvJson.dump() and then close the descriptor) so the write is
tied to the same inode and avoid a second pathname lookup—update the writeFile
call to accept/use tmpFd (or perform a direct write to tmpFd) and keep
AutoDelete(drvJsonPath, false) as-is.
In `@tests/functional/linux-sandbox.sh`:
- Around line 112-115: The test hook currently extracts the derivation name
using jq -r .env.name from the NIX_DERIVATION_V4 file, but the serializer emits
the canonical name at the top level; update the jq invocation in the hook script
to use .name instead of .env.name (i.e., change jq -r .env.name <
"\$NIX_DERIVATION_V4" > "$DEST" to jq -r .name < "\$NIX_DERIVATION_V4" >
"$DEST") so the test reads the top-level name field directly (symbols: HOOK,
NIX_DERIVATION_V4, DEST).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9726bd19-2eb3-48d0-9294-188c1761590e
📒 Files selected for processing (6)
src/libstore/derivations.ccsrc/libstore/include/nix/store/derivations.hhsrc/libstore/include/nix/store/globals.hhsrc/libstore/unix/build/derivation-builder.cctests/functional/linux-sandbox.shtests/functional/symlink-derivation.nix
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This allows the hook to inspect derivation attributes such as
requiredSystemFeatures.The hook already gets the path of the derivation as an argument, but the derivation typically doesn't exist when called as a remote build.
Context
Summary by CodeRabbit
New Features
NIX_DERIVATION_V4environment variable, enabling structured inspection of derivation details.Documentation