Move HttpBinaryCacheStore class from .cc file to header to enable
inheritance by S3BinaryCacheStore. Create S3BinaryCacheStore class that
overrides upsertFile() to implement multipart upload logic.
Add a sizeHint parameter to BinaryCacheStore::upsertFile() to enable
size-based upload decisions in implementations. This lays the groundwork
for reintroducing S3 multipart upload support.
Extract the path-based compression method determination logic into a
protected method that returns std::optional<std::string>. This allows
subclasses to reuse the logic and makes the semantics clearer (nullopt
means no compression, not empty string).
This prepares for S3BinaryCacheStore to apply the same compression
rules when implementing multipart uploads.
This commit adds two key fixes to http-binary-cache-store.cc to
properly support the new curl-based S3 implementation:
1. **Consistent cache key handling**: Use `getReference().render(withParams=false)`
for disk cache keys instead of `cacheUri.to_string()`. This ensures cache
keys are consistent with the S3 implementation and don't include query
parameters, which matches the behavior expected by Store::queryPathInfo()
lookups.
2. **S3 query parameter preservation**: When generating file transfer requests
for S3 URLs, preserve query parameters from the base URL (region, endpoint,
etc.) when the relative path doesn't have its own query parameters. This
ensures S3-specific configuration is propagated to all requests.
These stragglers have been accidentally left out when implementing the StoreConfig::getReference.
Also HttpBinaryCacheStore::getReference now returns the actual store parameters, not the cacheUri
parameters.
See the new extensive doxygen in `url.hh`.
This fixes fetching gitlab: flakes.
Paths are now stored as a std::vector of individual path
segments, which can themselves contain path separators '/' (%2F).
This is necessary to make the Gitlab's /projects/ API work.
Co-authored-by: John Ericson <John.Ericson@Obsidian.Systems>
Co-authored-by: Sergei Zimmerman <sergei@zimmerman.foo>
This allows us to replace some very hacky and not correct string
concatentation in `HttpBinaryCacheStore`. It will especially be useful
with #13752, when today's hacks started to cause problems in practice,
not just theory.
Also make `fixGitURL` returned a `ParsedURL`.
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.
* 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.
The existing header is a bit too big. Now the following use-cases are
separated, and get their own headers:
- Using or implementing an arbitrary store: remaining `store-api.hh`
This is closer to just being about the `Store` (and `StoreConfig`)
classes, as one would expect.
- Opening a store from a textual description: `store-open.hh`
Opening an aribtrary store implementation like this requires some sort
of store registration mechanism to exists, but the caller doesn't need
to know how it works. This just exposes the functions which use such a
mechanism, without exposing the mechanism itself
- Registering a store implementation: `store-registration.hh`
This requires understanding how the mechanism actually works, and the
mechanism in question involves templated machinery in headers we
rather not expose to things that don't need it, as it would slow down
compilation for no reason.
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
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.
Instead of the unhelpful
warning: 'https://cache.flakehub.com' does not appear to be a binary cache
you now get
warning: unable to download 'https://cache.flakehub.com/nix-cache-info': HTTP error 401
response body:
{"code":401,"error":"Unauthorized","message":"Unauthorized."}
This method is marked as `noexcept`, but `enqueueFileTransfer()` can
throw `Interrupted` if the user has hit Ctrl-C or if the `ThreadPool`
that the thread is a part of is shutting down.
Following what is outlined in #10766 refactor the uds-remote-store such
that the member variables (state) don't live in the store itself but in
the config object.
Additionally, the config object includes a new necessary constructor
that takes a scheme & authority.
Tests are commented out because of linking errors with the current config system.
When there is a new config system we can reenable them.
Co-authored-by: John Ericson <John.Ericson@Obsidian.Systems>
In particular `local://<path>` and `unix://` (without any path) now
work, and mean the same things as `local` and `daemon`, respectively. We
thus now have the opportunity to desguar `local` and `daemon` early.
This will allow me to make a change to
https://github.com/NixOS/nix/pull/9839 requested during review to
desugar those earlier.
Co-authored-by: Théophane Hufschmitt <7226587+thufschmitt@users.noreply.github.com>
This function returns true or false depending on whether the Nix client
is trusted or not. Mostly relevant when speaking to a remote store with
a daemon.
We include this information in `nix ping store` and `nix doctor`
Co-Authored-By: John Ericson <John.Ericson@Obsidian.Systems>
This is slightly more accurate considering that an outdated record
may exist in the persistent cache. Possibly-outdated records are
quite relevant as they may be foreign keys to more recent information
that we want to keep, but we will not return them here.
We embrace virtual the rest of the way, and get rid of the
`assert(false)` 0-param constructors.
We also list config base classes first, so the constructor order is
always:
1. all the configs
2. all the stores
Each in the same order
Rework the `Store` hierarchy so that there's now one hierarchy for the
store configs and one for the implementations (where each implementation
extends the corresponding config). So a class hierarchy like
```
StoreConfig-------->Store
| |
v v
SubStoreConfig----->SubStore
| |
v v
SubSubStoreConfig-->SubSubStore
```
(with virtual inheritance to prevent DDD).
The advantage of this architecture is that we can now introspect the configuration of a store without having to instantiate the store itself
Add a new `init()` method to the `Store` class that is supposed to
handle all the effectful initialisation needed to set-up the store.
The constructor should remain side-effect free and just initialize the
c++ data structure.
The goal behind that is that we can create “dummy” instances of each
store to query static properties about it (the parameters it accepts for
example)
Directly register the store classes rather than a function to build an
instance of them.
This gives the possibility to introspect static members of the class or
choose different ways of instantiating them.
