This is mostly theoretical, but the code was calling getenv("TZ")
twice: once to check if it's non-null, and again to get its value.
This creates a potential race condition where the environment could
change between calls.
The nix_api_store.cc tests and derivation-parser-bench.cc were using raw
getenv() calls or unsafe .value() calls on optional, which would segfault
when passed to std::filesystem::path constructor if the
_NIX_TEST_UNIT_DATA environment variable was not set.
The previous implementation called .value() on std::optional without
checking if it had a value. When _NIX_TEST_UNIT_DATA was not set, this
would throw std::bad_optional_access or cause a segfault in code that
used the raw getenv() result.
The new implementation checks the optional first and throws an Error
with a helpful message directing users to run tests via meson. The
example includes --gdb since this situation may arise when trying to
debug tests without knowing about meson's test infrastructure.
Pretty self-explanatory. More RAII is good and unclutters the already heavily overloaded
destructors from ownership logic. Not yet touching CURL *req because that would be too churny.
Fetching gcc-15.2.0.tar.gz I get a warning about UTF8 archive names. This
now mentions problematic pathnames.
warning: getting archive member 'gcc-15.2.0/gcc/testsuite/go.test/test/fixedbugs/issue27836.dir/Äfoo.go': Pathname can't be converted from UTF-8 to current locale.
warning: getting archive member 'gcc-15.2.0/gcc/testsuite/go.test/test/fixedbugs/issue27836.dir/Ämain.go': Pathname can't be converted from UTF-8 to current locale.
Also apparently libarchive depends on locale (yikes). Fixing reproducibility issues
that stem from this is a separate issue. At least having the warning actually mention
the pathname should be useful enough even though it's not actionable.
At least using the default locale yields something sane:
builtins.readDir "${gcc}/gcc/testsuite/go.test/test/fixedbugs/issue27836.dir"
{
"Äfoo.go" = "regular";
"Ämain.go" = "regular";
}
This matches what we just did for `nix path-info`, and I hope will allow
us to avoiding any more breaking changes to this command for the
foreseeable future.
Previously the daemon didn't validate that it got a valid GCAction
and did a naive C-style cast to the enum. This is certainly unintentional,
albeit mostly harmless in practice.
The fact that we were introducing a conversion from the output of `nix
path-info` into the input of `builtins.fetchTree` was the deciding
factor. We want scripting outputs into inputs like that to be easy.
Since JSON strings and objects are trivially distinguishable, we still
have the option of introducing the JSON format as an alternative input
scheme in the future, should we want to. (The output format would still
be SRI in that case, presumably.)
This restores compatibility with Nix 2.18, which behaved this
way. Note that this doesn't scan for the actually visible references.
Unlike in Nix 2.18, we only do this for paths with context, i.e. it
applies to `builtins.storePath "/nix/store/bla..."` but not
`"/nix/store/bla..."`. We don't want the latter because it shouldn't
matter whether a source file happens to be in the Nix store.
This was lost after 2.32 while making the accessor lazy. We can restore the support
for it pretty easily. Also this is significant optimization for nix nar cat.
E.g. with a NAR of a linux repo this speeds up by ~3x:
Benchmark 1: nix nar cat /tmp/linux.nar README
Time (mean ± σ): 737.2 ms ± 5.6 ms [User: 298.1 ms, System: 435.7 ms]
Range (min … max): 728.6 ms … 746.9 ms 10 runs
Benchmark 2: build/src/nix/nix nar cat /tmp/linux.nar README
Time (mean ± σ): 253.5 ms ± 2.9 ms [User: 56.4 ms, System: 196.3 ms]
Range (min … max): 248.1 ms … 258.7 ms 12 runs
The whole NarAccessor -> listing -> lazy NarAccessor is very weird. Source
can now be seek-ed over when supported, so we can support it pretty easily.
Alternatively we could also make it single-pass very easily with a custom
FileSystemObjectSink. It will get removed in a follow-up commit anyway.
This somehow fails https://hydra.nixos.org/build/315675349/nixlog/1. I don't know the exact
details, but it seems that something goes very wrong with LTO and sanitizers that lead to the
string literal to be moved? Instead of relying on the string literal deduplication to provide
a consistent address we can use a global. That should have a single address (modulo wonky copy
relocations).
I didn't do things quite right in 496e43ec72:
- Forgot to remove the now-redundant `isAllowed` check.
- Called the non-virtual, not the superclass's impl, in
`addDependencyPrep`, causing bad recursion / UB.
Doing this fixes a crash I encountered with manual testing an Nix Ninja
--- hopefully we will get Nix Ninja or similar in a NixOS test longer
term to defend against this thing happening again.
We have the machinery to make a more informative error, telling the
user what format was actually encountered, and not just that it is not
the format that was requested.
