This systematizes the way our s3:// URLs are parsed in filetransfer.cc.
Yoinked out and refactored out of [1].
[1]: https://github.com/NixOS/nix/pull/13752
Co-authored-by: Bernardo Meurer Costa <beme@anthropic.com>
Compilers in nixpkgs have caught up and major distros
should also have recent enough compilers. It would be
nice to have newer features like more full featured
ranges and deducing this.
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).
Rather than having store implementations return a free-form URI string,
have them return a `StoreReference`. This reflects that fact that this
method is supposed to invert `resolveStoreConfig`, which goes from a
`StoreReference` to some `StoreConfig` concrete derived class (based on
the registry).
`StoreConfig::getUri` is kept only as a convenience for the common case
that we want to immediately render the `StoreReference`.
A few tests were changed to use `local://` not `local`, since
`StoreReference` does not encode the `local` and `daemon` shorthands
(and instead desugars them to `local://` and `unix://` right away). I
think that is fine. `local` and `daemon` still work as input.
Previously `getUri` didn't include store query parameters,
`ssh-ng` didn't include any information at all and the local
store didn't have the path:
```
$ nix store info --store "local?root=/tmp/aaa&require-sigs=false"
Store URL: local
Version: 2.31.0
Trusted: 1
$ nix store info --store "ssh-ng://localhost?remote-program=nix-daemon"
Store URL: ssh-ng://
Version: 2.31.0
Trusted: 1
$ nix store info --store "ssh://localhost?remote-program=nix-store"
Store URL: ssh://localhost
```
This commit changes this to:
```
$ nix store info --store "local?root=/tmp/aaa&require-sigs=false"
Store URL: local?require-sigs=false&root=/tmp/aaa
Version: 2.31.0
Trusted: 1
$ nix store info --store "ssh-ng://localhost?remote-program=nix-daemon"
Store URL: ssh-ng://localhost?remote-program=nix-daemon
Version: 2.31.0
Trusted: 1
$ nix store info --store "ssh://localhost?remote-program=nix-store"
Store URL: ssh://localhost?remote-program=nix-store
```
The implicit dependency on refLength (which is the StorePath::HashLen)
is not good. Also the companion tests and benchmarks are already in libstore-tests.
This patch allows users to specify the connection port
in the store URLS like so:
```
nix store info --store "ssh-ng://localhost:22" --json
```
Previously this failed with: `error: failed to start SSH connection to 'localhost:22'`,
because the code did not distinguish the port from the hostname. This
patch remedies that problem by introducing a ParsedURL::Authority type
for working with parsed authority components of URIs.
Now that the URL parsing code is less ad-hoc we can
add more long-awaited fixes for specifying SSH connection
ports in store URIs.
Builds upon the work from bd1d2d1041.
Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo>
Co-authored-by: John Ericson <John.Ericson@Obsidian.Systems>
This keeps things fast by making the function inline, but also prevents
people from having to know about the `0xFF` implementation detail
directly, instead making one go through a `std::optional` (which could be
fused away with a sufficiently smart compiler).
Additionally, the base "nix32" implementation is moved to its own header
file pair, as it is logically distinct and prior to the `Hash` data
type. It would probably be nice to do this with all the hash format
implementations.
This benchmark should provide a relatively realistic
scenario for reference scanning.
As shown by the following results, reference scanning code
is already plenty fast and is definitely not a bottleneck:
```
BM_RefScanSinkRandom/10000 1672 ns 1682 ns 413354 bytes_per_second=5.53691Gi/s
BM_RefScanSinkRandom/100000 11217 ns 11124 ns 64341 bytes_per_second=8.37231Gi/s
BM_RefScanSinkRandom/1000000 205745 ns 204421 ns 3360 bytes_per_second=4.55591Gi/s
BM_RefScanSinkRandom/5000000 1208407 ns 1201046 ns 597 bytes_per_second=3.87713Gi/s
BM_RefScanSinkRandom/10000000 2534397 ns 2523344 ns 273 bytes_per_second=3.69083Gi/s
```
(Measurements on Ryzen 5900X via `nix build --file ci/gha/tests componentTests.nix-store-tests-run -L`)
This changes our GHA CI and nix-store-tests packaging
to build and run the benchmarks. This does not affect
the default packaging - the overrides apply only for the
GHA CI.
Instead of parsing a structured attrs at some later point, we parsed it
right away when parsing the A-Term format, and likewise serialize it to
`__json = <JSON dump>` when serializing a derivation to A-Term.
The JSON format can directly contain the JSON structured attrs without
so encoding it, so we just do that.
* It is tough to contribute to a project that doesn't use a formatter,
* It is extra hard to contribute to a project which has configured the formatter, but ignores it for some files
* Code formatting makes it harder to hide obscure / weird bugs by accident or on purpose,
Let's rip the bandaid off?
Note that PRs currently in flight should be able to be merged relatively easily by applying `clang-format` to their tip prior to merge.
This failed on macOS:
nix-store-tests-run> C++ exception with description "../nix_api_store.cc:33: nix_err_code(ctx) != NIX_OK, message: error: getting status of '/nix/var/nix/daemon-socket/socket': Operation not permitted" thrown in the test body.
Makes the behavoral change of #13263 without the underlying refactor.
Hopefully this clearly safe from a perf and GC perspective, and will
make it easier to benchmark #13263.
Unfortunately Feature is just an alias to `std::string`
and not a new-type, so a ton of code relies on it being
exactly a `std::string`.
Using transparent comparators just for StringSet necessitates
using it here as well.
The intention is to switch to transparent comparators from N3657 for
ordered set containers for strings and using the alias consistently
would simplify things.
Since we dropped fs::symlink_exists, we no longer have a need for the fs
namespace. Having less abstractions makes it easier to lookup the
functions in reference documentations.
We had fields set to the same values before in our test data. This is
not a problem per-se, but does mean we wouldn't catch certain mixups.
Now, the fields are set to distinct values (where possible), which makes
the test more robust.
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.
Now, both the unit and functional tests relating to derivation options
are tested both ways -- with input addressing and content-addressing
derivations.
It is just use for adding context to errors, but we have `addTrace` to
do that. Let the callers do that instead.
The callers doing so is a bit duplicated, yes, but this will get better
once `DerivationOptions` is included in `Derivation`.
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
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.
