-
Notifications
You must be signed in to change notification settings - Fork 2
don't include derivation name in temporary build directories #207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
With the migration to /nix/var/nix/builds we now have failing builds when the derivation name is too long. This change removes the derivation name from the temporary build to have a predictable prefix length: Also see: NixOS/infra#764 for context. (cherry picked from commit 725a2f3) (cherry picked from commit 7c3fd50)
WalkthroughTemporary build directory naming is changed from including derivation names to a generic “nix” prefix. Corresponding tests update their glob patterns to match the new naming, and documentation adds a page describing the change and rationale. No public APIs are modified. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant DB as DerivationBuilder
participant FS as Filesystem
participant Env as Build Env
DB->>FS: createTempDir(prefix="nix")
FS-->>DB: returns /nix/var/nix/builds/nix-XXXX
DB->>Env: setBuildTmpDir(/nix/.../nix-XXXX)
note over DB,Env: Subsequent build steps use the generic-prefixed temp dir
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/functional/check.sh (1)
55-56
: Drop redundant count check and rely on the array length.The array length check below already validates cardinality; the
count
call adds complexity and can misbehave without nullglob.- [[ 1 == "$(count "$customBuildDir/nix-"*)" ]] - local buildDir=("$customBuildDir/nix-"*) + local buildDir=("$customBuildDir/nix-"*)tests/nixos/user-sandboxing/default.nix (2)
107-109
: Prefer selecting the newest nix- dir to avoid multi-match flakiness.*Using a time-sorted ls reduces risk if multiple builds are present.
- machine.wait_until_succeeds("stat /nix/var/nix/builds/nix-*/build/syncPoint") - dir = machine.succeed("ls -d /nix/var/nix/builds/nix-*").strip() + machine.wait_until_succeeds("stat /nix/var/nix/builds/nix-*/build/syncPoint") + dir = machine.succeed("ls -dt /nix/var/nix/builds/nix-* | head -n1").strip()
128-130
: Same here: pick the most recent directory.- machine.wait_until_succeeds("stat /nix/var/nix/builds/nix-*/build/syncPoint") - dir = machine.succeed("ls -d /nix/var/nix/builds/nix-*").strip() + machine.wait_until_succeeds("stat /nix/var/nix/builds/nix-*/build/syncPoint") + dir = machine.succeed("ls -dt /nix/var/nix/builds/nix-* | head -n1").strip()doc/manual/rl-next/shorter-build-dir-names.md (1)
1-6
: Add a short migration note for downstream scripts.Call out the glob change explicitly to help users updating tests/tooling.
Temporary build directories created during derivation builds no longer include the derivation name in their path to avoid build failures when the derivation name is too long. This change ensures predictable prefix lengths for build directories under `/nix/var/nix/builds`. + + Migration: if you relied on matching `/nix/var/nix/builds/nix-build-*`, update globs to `/nix/var/nix/builds/nix-*`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
doc/manual/rl-next/shorter-build-dir-names.md
(1 hunks)src/libstore/unix/build/derivation-builder.cc
(1 hunks)tests/functional/check.sh
(1 hunks)tests/nixos/user-sandboxing/default.nix
(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/libstore/unix/build/derivation-builder.cc (2)
src/libutil/include/nix/util/file-system.hh (1)
createTempDir
(330-330)src/libutil/file-system.cc (2)
createTempDir
(677-707)createTempDir
(677-677)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (3)
tests/functional/check.sh (1)
58-58
: Good: updated error message matches new glob.doc/manual/rl-next/shorter-build-dir-names.md (1)
3-3
: Confirm PR reference metadata.Is
prs: [13839]
intentional for this repo? If you want to cross-link this PR as well, add it here per your docs conventions.src/libstore/unix/build/derivation-builder.cc (1)
720-720
: Shorter tmpdir prefix LGTM — remaining "nix-build-" usages found; verify or update.
- src/libstore/unix/build/linux-derivation-builder.cc: cgroup names use "nix-build-uid-"/"nix-build-pid-" (lines ~235–236).
- tests/nixos/cgroups/default.nix: looks for "nix-build-uid-*" (lines 33, 38).
- src/libstore/include/nix/store/globals.hh: example tmpDir/topTmpDir contain "nix-build-..." (lines ~1450, 1452).
- doc/manual/source/advanced-topics/diff-hook.md: references '/tmp/nix-build-...' (line 94).
- tests/functional/nix-build.sh: test invocations reference nix-build (lines 37–46).
Confirm these are intentionally unchanged or update tests/docs/cgroup naming to match the new "nix" tmpdir prefix.
Motivation
With the migration to /nix/var/nix/builds we now have failing builds when the derivation name is too long.
This change removes the derivation name from the temporary build to have a predictable prefix length:
Also see: NixOS/infra#764
for context.
(cherry picked from commit 725a2f3) (cherry picked from commit 7c3fd50)
Context
Summary by CodeRabbit
Bug Fixes
Documentation
Tests