Since 3c610df550 this resulted in `getting status of`
errors on paths inside the chroot if a path was already valid. Careful inspection
of the logic shows that if buildMode != bmCheck actualPath gets reassigned to
store.toRealPath(finalDestPath). The only branch that cares about actualPath is
the buildMode == bmCheck case, which doesn't lead to optimisePath anyway.
Realisations are conceptually key-value pairs, mapping `DrvOutputs` (the
key) to information about that derivation output.
This separate the value type, which will be useful in maps, etc., where
we don't want to denormalize by including the key twice.
This matches similar changes for existing types:
| keyed | unkeyed |
|--------------------|------------------------|
| `ValidPathInfo` | `UnkeyedValidPathInfo` |
| `KeyedBuildResult` | `BuildResult` |
| `Realisation` | `UnkeyedRealisation` |
Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo>
The macro now accurately reflects its purpose: gating only AWS
authentication code, not all S3 functionality. S3 URL parsing, store
configuration, and public bucket access work regardless of this flag.
This rename clarifies that:
- S3 support is always available (URL parsing, store registration)
- Only AWS credential resolution requires the flag
- The flag controls AWS CRT SDK dependency, not S3 protocol support
Add support for pre-resolving AWS credentials in the parent process
before forking for builtin:fetchurl. This avoids recreating credential
providers in the forked child process.
I want to separate "policy" from "mechanism".
Now the logic to decide how to build (a policy choice, though with some
hard constraints) is all in derivation building goal, and all in the
same spot. build hook, external builder, or local builder --- the choice
between all three is made in the same spot --- pure policy.
Now, if you want to use the external deriation builder, you simply
provide the `ExternalBuilder` you wish to use, and there is no
additional checking --- pure mechanism. It is the responsibility of the
caller to choose an external builder that works for the derivation in
question.
Also, `checkSystem()` was the only thing throwing `BuildError` from
`startBuilder`. Now that that is gone, we can now remove the
`try...catch` around that.
These are helper programs that execute derivations for specified
system types (e.g. using QEMU to emulate another system type).
To use, set `external-builders`:
external-builders = [{"systems": ["aarch64-linux"], "program": "/path/to/external-builder.py"}]
The external builder gets one command line argument, the path to a JSON file containing all necessary information about the derivation:
{
"args": [...],
"builder": "/nix/store/kwcyvgdg98n98hqapaz8sw92pc2s78x6-bash-5.2p37/bin/bash",
"env": {
"HOME": "/homeless-shelter",
...
},
"realStoreDir": "/tmp/nix/nix/store",
"storeDir": "/nix/store",
"tmpDir": "/tmp/nix-shell.dzQ2hE/nix-build-patchelf-0.14.3.drv-46/build",
"tmpDirInSandbox": "/build"
}
Co-authored-by: Cole Helbling <cole.helbling@determinate.systems>
Realisations are conceptually key-value pairs, mapping `DrvOutputs` (the
key) to information about that derivation output.
This separate the value type, which will be useful in maps, etc., where
we don't want to denormalize by including the key twice.
This matches similar changes for existing types:
| keyed | unkeyed |
|--------------------|------------------------|
| `ValidPathInfo` | `UnkeyedValidPathInfo` |
| `KeyedBuildResult` | `BuildResult` |
| `Realisation` | `UnkeyedRealisation` |
Replace non-thread-safe ptsname() calls with a new getPtsName() helper
function that:
- Uses thread-safe ptsname_r() on Linux/BSD platforms
- Uses mutex-protected ptsname() on macOS (which lacks ptsname_r())
In the case where the store object doesn't exist, we do correctly move
(rather than copy) the scratch data into place. In this case, the
destination store object already exists, but we still want to clean up
after ourselves.
It is only done in the `force = true` case, and the only
`cleanupBuild(true)` call is right after where it used to be, so this
has the exact same behavior as before.
Calling `reset` on this `std::optional` field of `DerivationBuilderImpl`
is also what the (automatically created) destructor of
`DerivationBuilderImpl` will do. We should be making sure that the
derivation builder is cleaned up by the goal anyways, and if we do that,
then this `Finally` is no longer needed.
Before, had a very ugly `appendLogTailErrorMsg` callback. Now, we
instead have a `fixupBuilderFailureErrorMessage` that is just used by
`DerivationBuildingGoal`, and `DerivationBuilder` just returns the raw
data needed by this.
Now we have better separation of the core logic --- an integral part of
the store layer spec even --- from the goal mechanism and other
minutiae.
Co-authored-by: Jeremy Kolb <kjeremy@gmail.com>
I think this should be fine for repairing. If anything, it is better,
because it would be weird to "mark and output good" only for it to then
fail output checks.
Sadly we cannot unexpose `DerivationBuilder::killChild` yet, because
`DerivationBuildingGoal` calls it elsewhere, but we can at least haave a
better division of labor between the two destructors.
It's hard to tell if I changed any behavior, but if I did, I think I
made it better, because now we explicitly move stuff out of the chroot
(if we were going to) before trying to delete the chroot.
Do this to match `DerivationBuilder::deleteTmpDir`, which we'll want to
combine it with next.
Also chenge one caller from `deleteTmpDir(true)` to `cleanupBuild(true)`
now that this is done, because it will not make a difference.
This should be a pure refactor with no behavioral change.
Aftet the previous simplifications, there is no reason to catch the
error and immediately return it with a `std::variant` --- just let the
caller catch it instead.
Instead of that funny business, the fixed output checks are not put in
`checkOutputs`, with the other (newer) output checks, where they also
better belong. The control flow is reworked (with comments!) so that
`checkOutputs` also runs in the `bmCheck` case.
Not only does this preserve existing behavior of `bmCheck`
double-checking fixed output hashes with less tricky code, it also makes
`bmCheck` better by also double-checking the other output checks, rather
than just assuming they pass if the derivation is deterministic.
It's fine to set these worker flags a little later in the control flow,
since we'll be sure to reach those points in the error cases. And doing
that is much nicer than having these tangled callbacks.
I originally made the callbacks to meticulously recreate the exact
behavior which I didn't quite understand. Now, thanks to cleaning up the
error handling, I do understand what is going on, so I can be confident
that this change is safe to make.
Instead of passing them around separately, or doing finicky logic in a
try-catch block to recover them, just make `BuildError` always contain a
status, and make it the thrower's responsibility to set it. This is much
more simple and explicit.
Once that change is done, split the `done` functions of `DerivationGoal`
and `DerivationBuildingGoal` into separate success and failure
functions, which ends up being easier to understand and hardly any
duplication.
Also, change the handling of failures in resolved cases to use
`BuildResult::DependencyFailed` and a new message. This is because the
underlying derivation will also get its message printed --- which is
good, because in general the resolved derivation is not unique. One dyn
drv test had to be updated, but CA (and dyn drv) is experimental, so I
do not mind.
Finally, delete `SubstError` because it is unused.
We currently just use this during the build of a derivation, but there is no
reason we wouldn't want to use it elsewhere, e.g. to check the outputs
of someone else's build after the fact.
Moreover, I like pulling things out of `DerivationBuilder` that are
simple and don't need access to all that state. While
`DerivationBuilder` is unix-only, this refactor also make the code more
portable "for free".
The header is private, at Eelco's request.
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: https://github.com/NixOS/infra/pull/764
for context.
It is suppposed to be "post build" not "during the build" after all. Its
location now matches that for the hook case (see elsewhere in
`DerivationdBuildingGoal`).
It was in a try-catch before, and now it isn't, but I believe that it is
impossible for it to throw `BuildError`, which is sufficient for this
code motion to be correct.
This is a nicer separation of concerns --- `DerivationBuilder` just
mounts the extra paths you tell it too, and the outside world is
responsible for making sure those extra paths make sense.
Since the closure only depends on global settings, and not
per-derivation information, we also have the option of moving this up
further and caching it across all local builds. (I only just realized
this after having done this refactor. I am not doing that change at this
time, however.)
Now, `DerivationBuilder` only concerns itself with `finalEnv` and
`extraFiles`, in straightforward unconditional code. All the fancy
desugaring logic is consolidated in `DerivationBuildingGoal`.
We should better share the pulled-out logic with `nix-shell`/`nix
develop`, which would fill in some missing features, arguably fixing
bugs.
I think this is a better separation of concerns. `DerivationBuilder`
doesn't need to to the final, query-heavy details about how these things
are constructed. It just operates on the level of "simple, stupid" files
and environment variables.
As much as I prefer rewriting the parsed rather than unparsed JSON for
elegance, this gets in the way of the separation of concerns that I am
trying to do.
As a practical matter, any rewriting that this did will also be done by
the second round of rewriting that remains below, so removing this code
should have no effect.
