Turns out we didn't have tests for some of the important behavior introduced
for flake reference fragments and url queries [1]. This is rather important
and is relied upon by existing tooling. This fixes up these exact cases before
handing off the URL to the Boost.URL parser.
To the best of my knowledge this implements the same behavior as prior regex-based
parser did [2]:
> fragmentRegex = "(?:" + pcharRegex + "|[/? \"^])*";
> queryRegex = "(?:" + pcharRegex + "|[/? \"])*";
[1]: 9c0a09f09f
[2]: https://github.com/NixOS/nix/blob/2.30.2/src/libutil/include/nix/util/url-parts.hh
It's about time we added debuggers to the dev-shell. Having it in build inputs
does some magic so pretty printers for standard library types work better.
Since this goal has no (goal-wide) notion of "wanted outputs" (we're
building the derivation, and thus making all outputs), we should have
`initialOutputs` for all outputs, and if we're missing one that's an
internal error caused by a bug in Nix.
Concretely, `DerivationBuildingGoal::gaveUpOnSubstitution` now clearly
does create `initialOutputs` for all outputs, whereas a few commits ago
that was not obvious, so I feel confident in saying that this invariant
that should be upheld, in fact is upheld.
`scatchOutputs` is initialized for every initial output, so the same
change to it follows for the same reasons.
This is just more honest, since we downcasted it to `LocalStore` in many
places. We had the downcast before because it wasn't needed in the hook
case, just the local building case, but now that `DerivationBuilder` is
separated and just does the building case, we have formalized the
boundary where the single downcast should occur.
No derivation goal type has a notion of variable wanted outputs any
more. They either want them all, or they just care about a single
output, in which case we would just store this information for the one
output in question.
This is good for e.g. `std::string_view` and `StringMap`.
Needed by #11139
Co-authored-by: Sergei Zimmerman <145775305+xokdvium@users.noreply.github.com>
We can cut out some gratuitous inhertence as follows:
- `MixStoreDirMethods` -> `StoreDirConfig`
- `StoreDirConfig` deleted because no longer needed. It is just folded
into `StoreConfig`.
- `StoreDirConfigBase` -> `StoreConfigBase` same trick still needed, but
now is for `StoreConfig` not `StoreDirConfig`
Here's how we got here:
1. I once factored out `StoreDirConfig` in #6236.
2. I factored out `MixStoreDirMethods` in #13154.
But, I didn't realize at point (2) that we didn't need `StoreDirConfig`
anymore, all uses of `StoreDirConfig` could instead be uses of
`MixStoreDirMethods`. Now I am doing that, and renaming
`MixStoreDirMethods` to just `StoreDirConfig` to reduce churn.
This leads to a use-after free, because staticOutputHashes returns a temporary
object that dies before we can do a `return *mOutputHash`.
This is most likely the cause for random failures in Hydra [1].
[1]: https://hydra.nixos.org/build/305091330/nixlog/2
Old code completely ignored query parameters and it seems ok to keep
that behavior. There's a lot of code out there that parses nix code
like nix-output-monitor and it can't parse messages like:
> copying path '/nix/store/wha2hi4yhkjmccqhivxavbfspsg1wrsj-source' from 'https://cache.nixos.org' to 'local://'...
Let's not break these tools without a good reason. This goes in line
with what other code does by ignoring parameters in logs.
The issue is just in detecting the shorthand notations for the store
reference - not in printing the url in logs.
By default the daemon opens a local store with ?path-info-cache-size=0,
so that leads to the erronenous 'local://'.
The problem with old code was that it used getUri for both the `diskCache`
as well as logging. This is really bad because it mixes the textual human
readable representation with the caching.
Also using getUri for the cache key is really problematic for the S3 store,
since it doesn't include the `endpoint` in the cache key, so it's totally broken.
This starts separating the logging / cache concerns by introducing a
`getHumanReadableURI` that should only be used for logging. The caching
logic now instead uses `getReference().render(/*withParams=*/false)` exclusively.
This would need to be fixed in follow-ups, because that's really fragile and
broken for some store types (but it was already broken before).
Move output result filtering logic and assert just into the branch where
it is not obviously a no op / meeting the assertion.
Add a comment too, while we are at it.
Now that `DerivationGoal::checkPathValidity` is legible, we can see that
it only sets `outputKnown`, and doesn't read it. Likewise, with
co-routines, we don't have tiny scopes that make local variables
difficult. Between these two things, we can simply have
`checkPathValidity` return what it finds, rather than mutate some state,
and update everyting to use local variables.
The same transformation could probably be done to the other derivation
goal types (which currently, unfortunately, contain their own
`checkPathValidity`s, though they are diverging, and we hope and believe
that they continue to diverge).
`Store::queryPartialDerivationOutputMap` is nothing but checking
statically-known output paths, and then `Store::queryRealisation`, and
we were doing both of those things already. Inline that and simplify,
again taking advantage of the fact that we only care about one output.
