This refactor shouldn't change much except add a new `nix formatter run`
command. This creates space for the new `nix formatter build` command,
which I'll be introducing in the next commit.
Before the change `nix` was stripping warning flags
reported by `gcc-14` too eagerly:
$ nix build -f. texinfo4
error: builder for '/nix/store/i9948l91s3df44ip5jlpp6imbrcs646x-texinfo-4.13a.drv' failed with exit code 2;
last 25 log lines:
> 1495 | info_tag (mbi_iterator_t iter, int handle, size_t *plen)
> | ~~~~~~~~^~~~
> window.c:1887:39: error: passing argument 4 of 'printed_representation' from incompatible pointer type []
> 1887 | &replen);
> | ^~~~~~~
> | |
> | int *
After the change the compiler flag remains:
$ ~/patched.nix build -f. texinfo4
error: builder for '/nix/store/i9948l91s3df44ip5jlpp6imbrcs646x-texinfo-4.13a.drv' failed with exit code 2;
last 25 log lines:
> 1495 | info_tag (mbi_iterator_t iter, int handle, size_t *plen)
> | ~~~~~~~~^~~~
> window.c:1887:39: error: passing argument 4 of 'printed_representation' from incompatible pointer type [-Wincompatible-pointer-types]
> 1887 | &replen);
> | ^~~~~~~
> | |
> | int *
Note the difference in flag rendering around the warning.
https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda has a
good sumamry of why it happens. Befomre the change `nix` was handling
just one form or URL separator:
$ printf '\e]8;;http://example.com\e\\This is a link\e]8;;\e\\\n'
Now it also handled another for (used by gcc-14`):
printf '\e]8;;http://example.com\aThis is a link\e]8;;\a\n'
While at it fixed accumulation of trailing escape `\e\\` symbol.
The explicit include is needed for clangd to not get confused somehow,
which is also what threw me off initially and made me pick the wrong
constructor.
The (pointer, number, number) constructor first constructs a C string
and then takes a substring from that, but we didn't specify that the
buffer needs to be NUL-terminated, and then what would be the point of
the size argument anyway...
basic_string.h:
> basic_string(const _Tp& __t, size_type __pos, size_type __n,
> const _Alloc& __a = _Alloc())
> : basic_string(_S_to_string_view(__t).substr(__pos, __n), __a) { }
Valgrind on nixops4/rust/nix-flake tests:
==1344422== Conditional jump or move depends on uninitialised value(s)
==1344422== at 0x48513E8: strlen (vg_replace_strmem.c:505)
==1344422== by 0x488E941: UnknownInlinedFun (char_traits.h:391)
==1344422== by 0x488E941: UnknownInlinedFun (string_view:141)
==1344422== by 0x488E941: UnknownInlinedFun (basic_string.h:790)
==1344422== by 0x488E941: nix_flake_reference_and_fragment_from_string (nix_api_flake.cc:81)
==1344422== by 0x127332: nix_flake::FlakeReference::parse_with_fragment (lib.rs:123)
It seems that the intention was to format a number in base 8 (as
suggested by the %o format specifier), but `perms` is a `std::string`
and not a number. Looks like `rawMode` is the correct thing to use here.
This name is close to the Nixpkgs lib function `escapeShellArg`,
making it easier to find.
A friendlier function with the same behavior as lib could be added
later.
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.
