As it turns out the orignal implementation of symlink_exists cannot be
used in Nix because it did now std::filesystem::filesystem_error.
The new implementation fixes that but is now actually the same as
pathExists except for the path type.
Fixes
../src/libstore/unix/build/derivation-builder.cc:1130:86: warning: the compiler can assume that the address of ‘nix::DerivationBuilderParams::drv’ will never be NULL [-Waddress]
1130 | if (useChroot && settings.preBuildHook != "" && dynamic_cast<const Derivation *>(&drv)) {
| ^~~~
Assuming this check was left over from the time `drv` could be a
`BasicDerivation`.
I split it out before to try to separate the building logic, but now we
have the much better `DerivationBuilder` abstraction for that. With that
change, I think `LocalDerivationGoal` has outlived its usefulness.
We just inline it back into `DerivationGoal`, and do so with minimal
`#ifdef` for Windows.
Note that the order of statements in `~DerivationGoal` is different than
it was after the `~LocalDerivationGoal` split, but it is *restored* to
the way it original was before --- evidently I did the split slightly
wrong, but nobody noticed, probably because the order doesn't actually
matter.
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
Now that `DerivationBuilder` is created after the underlying data has
already been initialized, we can just refer this data normally, with a
direct reference.
Only `parsedDrv` takes a (borrowing) pointer, because independent of
initialization the derivation may or may not have structured attrs.
The building logic is now free of the scheduling logic!
(The interface between them is just what is in the new header. This
makes it much easier to audit, and shrink over time.)
We have a new `DerivationBuilder` struct, and `DerivationBuilderParams`
`DerivationBuilderCallbacks` supporting it.
`LocalDerivationGoal` doesn't subclass any of these, so we are ready to
now move them out to a new file!
Now, most of it is in two new functions:
`LocalDerivationGoal::{,un}repareBuild`.
This might seems like a step backwards from coroutines --- now we have
more functions, and are stuck with class vars --- but I don't think it
needs to be.
There's a few options here:
- (Re)introduce coroutines for the isolated building logic. We could use the
same coroutines types, or simpler ones specialized to this use-case.
The `tryLocalBuild` caller can still use `Goal::Co`, and just will
manually "pump" this inner coroutine.
- Return closures from each step. This is sort of like coroutines by
hand, but it still allows us to stop writing down the local variables
in each type.
Being able to fully-use RAII again would be very nice!
- Keep top-level first-order functions like now, but make more
functional. Instead of having one state object (`DerivationBuilder`)
for all steps (setup, run, teardown), we can have separate structs for
the live variables at each point we consume and return.
This at least avoids "are these variables active at this time?"
questions, but doesn't give us the full benefit of RAII as we must
manually ensure FIFO create/destroy orders still.
One thing to note is that by keeping the `outputLock` unlocking in
`tryLocalBuild`, we are arguably uncovering a rebuild scheduling vs
building distinction, as the output locks are pretty squarely a
scheduling concern. It's nice that the builder doesn't need to know
about them at all.
Only a much smaller `StructuredAttrs` remains, the rest is is now moved
to `DerivationOptions`.
This gets us quite close to `std::optional<StructuredAttrs>` and
`DerivationOptions` being included in `Derivation` as fields.
Revert most of "Hack together a fix for the public headers"
- The `libmain` change is kept, and one more libmain change is made.
(Need to update Meson and Nix per the package alike).
- The S3 situation is fixed in a different way: the variable is public
now, used in the header, and fixed accordingly.
- Fix TODO for `HAVE_EMBEDDED_SANDBOX_SHELL`
This reverts commit 2b51250534.
(cherry picked from commit 3294b22a68)
Revert most of "Hack together a fix for the public headers"
- The `libmain` change is kept, and one more libmain change is made.
(Need to update Meson and Nix per the package alike).
- The S3 situation is fixed in a different way: the variable is public
now, used in the header, and fixed accordingly.
- Fix TODO for `HAVE_EMBEDDED_SANDBOX_SHELL`
This reverts commit 2b51250534.
For example, instead of doing
#include "nix/store-config.hh"
#include "nix/derived-path.hh"
Now do
#include "nix/store/config.hh"
#include "nix/store/derived-path.hh"
This was originally planned in the issue, and also recent requested by
Eelco.
Most of the change is purely mechanical. There is just one small
additional issue. See how, in the example above, we took this
opportunity to also turn `<comp>-config.hh` into `<comp>/config.hh`.
Well, there was already a `nix/util/config.{cc,hh}`. Even though there
is not a public configuration header for libutil (which also would be
called `nix/util/config.{cc,hh}`) that's still confusing, To avoid any
such confusion, we renamed that to `nix/util/configuration.{cc,hh}`.
Finally, note that the libflake headers already did this, so we didn't
need to do anything to them. We wouldn't want to mistakenly get
`nix/flake/flake/flake.hh`!
Progress on #7876
(cherry picked from commit cc24766fa6)
For example, instead of doing
#include "nix/store-config.hh"
#include "nix/derived-path.hh"
Now do
#include "nix/store/config.hh"
#include "nix/store/derived-path.hh"
This was originally planned in the issue, and also recent requested by
Eelco.
Most of the change is purely mechanical. There is just one small
additional issue. See how, in the example above, we took this
opportunity to also turn `<comp>-config.hh` into `<comp>/config.hh`.
Well, there was already a `nix/util/config.{cc,hh}`. Even though there
is not a public configuration header for libutil (which also would be
called `nix/util/config.{cc,hh}`) that's still confusing, To avoid any
such confusion, we renamed that to `nix/util/configuration.{cc,hh}`.
Finally, note that the libflake headers already did this, so we didn't
need to do anything to them. We wouldn't want to mistakenly get
`nix/flake/flake/flake.hh`!
Progress on #7876
There are two big changes:
1. Public and private config is now separated. Configuration variables
that are only used internally do not go in a header which is
installed.
(Additionally, libutil has a unix-specific private config header,
which should only be used in unix-specific code. This keeps things a
bit more organized, in a purely private implementation-internal way.)
2. Secondly, there is no more `-include`. There are very few config
items that need to be publically exposed, so now it is feasible to
just make the headers that need them just including the (public)
configuration header.
And there are also a few more small cleanups on top of those:
- The configuration files have better names.
- The few CPP variables that remain exposed in the public headers are
now also renamed to always start with `NIX_`. This ensures they should
not conflict with variables defined elsewhere.
- We now always use `#if` and not `#ifdef`/`#ifndef` for our
configuration variables, which helps avoid bugs by requiring that
variables must be defined in all cases.
(cherry picked from commit c204e307ac)
There are two big changes:
1. Public and private config is now separated. Configuration variables
that are only used internally do not go in a header which is
installed.
(Additionally, libutil has a unix-specific private config header,
which should only be used in unix-specific code. This keeps things a
bit more organized, in a purely private implementation-internal way.)
2. Secondly, there is no more `-include`. There are very few config
items that need to be publically exposed, so now it is feasible to
just make the headers that need them just including the (public)
configuration header.
And there are also a few more small cleanups on top of those:
- The configuration files have better names.
- The few CPP variables that remain exposed in the public headers are
now also renamed to always start with `NIX_`. This ensures they should
not conflict with variables defined elsewhere.
- We now always use `#if` and not `#ifdef`/`#ifndef` for our
configuration variables, which helps avoid bugs by requiring that
variables must be defined in all cases.
The short answer for why we need to do this is so we can consistently do
`#include "nix/..."`. Without this change, there are ways to still make
that work, but they are hacky, and they have downsides such as making it
harder to make sure headers from the wrong Nix library (e..g.
`libnixexpr` headers in `libnixutil`) aren't being used.
The C API alraedy used `nix_api_*`, so its headers are *not* put in
subdirectories accordingly.
Progress on #7876
We resisted doing this for a while because it would be annoying to not
have the header source file pairs close by / easy to change file
path/name from one to the other. But I am ameliorating that with
symlinks in the next commit.
(cherry picked from commit f3e1c47f47)
The short answer for why we need to do this is so we can consistently do
`#include "nix/..."`. Without this change, there are ways to still make
that work, but they are hacky, and they have downsides such as making it
harder to make sure headers from the wrong Nix library (e..g.
`libnixexpr` headers in `libnixutil`) aren't being used.
The C API alraedy used `nix_api_*`, so its headers are *not* put in
subdirectories accordingly.
Progress on #7876
We resisted doing this for a while because it would be annoying to not
have the header source file pairs close by / easy to change file
path/name from one to the other. But I am ameliorating that with
symlinks in the next commit.
Before the change "illegal reference" was hard to interpret as it did
not mention what derivation actually hits it.
Today's `nixpkgs` example:
Before the change:
$ nix build --no-link -f. postgresql_14
...
error: derivation contains an illegal reference specifier 'man'
After the change:
$ nix build --no-link -f. postgresql_14
...
error: derivation '/nix/store/bxp6g57limvwiga61vdlyvhy7i8rp6wd-postgresql-14.15.drv' output check for 'lib' contains an illegal reference specifier 'man', expected store path or output name (one of [debug, dev, doc, lib, out])
(cherry picked from commit bbdc3197a9)
Before the change "illegal reference" was hard to interpret as it did
not mention what derivation actually hits it.
Today's `nixpkgs` example:
Before the change:
$ nix build --no-link -f. postgresql_14
...
error: derivation contains an illegal reference specifier 'man'
After the change:
$ nix build --no-link -f. postgresql_14
...
error: derivation '/nix/store/bxp6g57limvwiga61vdlyvhy7i8rp6wd-postgresql-14.15.drv' output check for 'lib' contains an illegal reference specifier 'man', expected store path or output name (one of [debug, dev, doc, lib, out])
