diff --git a/src/libstore-tests/http-binary-cache-store.cc b/src/libstore-tests/http-binary-cache-store.cc index f4a3408b5..0e3be4ced 100644 --- a/src/libstore-tests/http-binary-cache-store.cc +++ b/src/libstore-tests/http-binary-cache-store.cc @@ -8,14 +8,14 @@ TEST(HttpBinaryCacheStore, constructConfig) { HttpBinaryCacheStoreConfig config{"http", "foo.bar.baz", {}}; - EXPECT_EQ(config.cacheUri, "http://foo.bar.baz"); + EXPECT_EQ(config.cacheUri.to_string(), "http://foo.bar.baz"); } TEST(HttpBinaryCacheStore, constructConfigNoTrailingSlash) { HttpBinaryCacheStoreConfig config{"https", "foo.bar.baz/a/b/", {}}; - EXPECT_EQ(config.cacheUri, "https://foo.bar.baz/a/b"); + EXPECT_EQ(config.cacheUri.to_string(), "https://foo.bar.baz/a/b"); } } // namespace nix diff --git a/src/libstore-tests/nix_api_store.cc b/src/libstore-tests/nix_api_store.cc index cff889ab9..2310c4395 100644 --- a/src/libstore-tests/nix_api_store.cc +++ b/src/libstore-tests/nix_api_store.cc @@ -24,7 +24,7 @@ TEST_F(nix_api_store_test, nix_store_get_uri) std::string str; auto ret = nix_store_get_uri(ctx, store, OBSERVE_STRING(str)); ASSERT_EQ(NIX_OK, ret); - auto expectedStoreURI = "local?" + auto expectedStoreURI = "local://?" + nix::encodeQuery({ {"log", nixLogDir}, {"state", nixStateDir}, @@ -104,7 +104,7 @@ TEST_F(nix_api_util_context, nix_store_open_dummy) nix_libstore_init(ctx); Store * store = nix_store_open(ctx, "dummy://", nullptr); ASSERT_EQ(NIX_OK, ctx->last_err_code); - ASSERT_STREQ("dummy", store->ptr->config.getUri().c_str()); + ASSERT_STREQ("dummy://", store->ptr->config.getUri().c_str()); std::string str; nix_store_get_version(ctx, store, OBSERVE_STRING(str)); diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc index bc8f5b6f5..d0e298968 100644 --- a/src/libstore/dummy-store.cc +++ b/src/libstore/dummy-store.cc @@ -33,9 +33,14 @@ struct DummyStoreConfig : public std::enable_shared_from_this, ref openStore() const override; - std::string getUri() const override + StoreReference getReference() const override { - return *uriSchemes().begin(); + return { + .variant = + StoreReference::Specified{ + .scheme = *uriSchemes().begin(), + }, + }; } }; diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index 31899f629..fc19bc1f8 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -22,13 +22,25 @@ HttpBinaryCacheStoreConfig::HttpBinaryCacheStoreConfig( std::string_view scheme, std::string_view _cacheUri, const Params & params) : StoreConfig(params) , BinaryCacheStoreConfig(params) - , cacheUri( + , cacheUri(parseURL( std::string{scheme} + "://" + (!_cacheUri.empty() ? _cacheUri - : throw UsageError("`%s` Store requires a non-empty authority in Store URL", scheme))) + : throw UsageError("`%s` Store requires a non-empty authority in Store URL", scheme)))) { - while (!cacheUri.empty() && cacheUri.back() == '/') - cacheUri.pop_back(); + while (!cacheUri.path.empty() && cacheUri.path.back() == '/') + cacheUri.path.pop_back(); +} + +StoreReference HttpBinaryCacheStoreConfig::getReference() const +{ + return { + .variant = + StoreReference::Specified{ + .scheme = cacheUri.scheme, + .authority = (cacheUri.authority ? cacheUri.authority->to_string() : "") + cacheUri.path, + }, + .params = cacheUri.query, + }; } std::string HttpBinaryCacheStoreConfig::doc() @@ -65,16 +77,17 @@ public: void init() override { // FIXME: do this lazily? - if (auto cacheInfo = diskCache->upToDateCacheExists(config->cacheUri)) { + if (auto cacheInfo = diskCache->upToDateCacheExists(config->cacheUri.to_string())) { config->wantMassQuery.setDefault(cacheInfo->wantMassQuery); config->priority.setDefault(cacheInfo->priority); } else { try { BinaryCacheStore::init(); } catch (UploadToHTTP &) { - throw Error("'%s' does not appear to be a binary cache", config->cacheUri); + throw Error("'%s' does not appear to be a binary cache", config->cacheUri.to_string()); } - diskCache->createCache(config->cacheUri, config->storeDir, config->wantMassQuery, config->priority); + diskCache->createCache( + config->cacheUri.to_string(), config->storeDir, config->wantMassQuery, config->priority); } } @@ -134,16 +147,29 @@ protected: try { getFileTransfer()->upload(req); } catch (FileTransferError & e) { - throw UploadToHTTP("while uploading to HTTP binary cache at '%s': %s", config->cacheUri, e.msg()); + throw UploadToHTTP( + "while uploading to HTTP binary cache at '%s': %s", config->cacheUri.to_string(), e.msg()); } } FileTransferRequest makeRequest(const std::string & path) { + /* FIXME path is not a path, but a full relative or absolute + URL, e.g. we've seen in the wild NARINFO files have a URL + field which is + `nar/15f99rdaf26k39knmzry4xd0d97wp6yfpnfk1z9avakis7ipb9yg.nar?hash=zphkqn2wg8mnvbkixnl2aadkbn0rcnfj` + (note the query param) and that gets passed here. + + What should actually happen is that we have two parsed URLs + (if we support relative URLs), and then we combined them with + a URL `operator/` which would be like + `std::filesystem::path`'s equivalent operator, which properly + combines the the URLs, whether the right is relative or + absolute. */ return FileTransferRequest( hasPrefix(path, "https://") || hasPrefix(path, "http://") || hasPrefix(path, "file://") ? path - : config->cacheUri + "/" + path); + : config->cacheUri.to_string() + "/" + path); } void getFile(const std::string & path, Sink & sink) override diff --git a/src/libstore/include/nix/store/http-binary-cache-store.hh b/src/libstore/include/nix/store/http-binary-cache-store.hh index ef13aa7b6..e0f6ce42f 100644 --- a/src/libstore/include/nix/store/http-binary-cache-store.hh +++ b/src/libstore/include/nix/store/http-binary-cache-store.hh @@ -1,3 +1,4 @@ +#include "nix/util/url.hh" #include "nix/store/binary-cache-store.hh" namespace nix { @@ -11,7 +12,7 @@ struct HttpBinaryCacheStoreConfig : std::enable_shared_from_this openStore() const override; - std::string getUri() const override - { - return cacheUri; - } + StoreReference getReference() const override; }; } // namespace nix diff --git a/src/libstore/include/nix/store/legacy-ssh-store.hh b/src/libstore/include/nix/store/legacy-ssh-store.hh index e53d18559..91e021433 100644 --- a/src/libstore/include/nix/store/legacy-ssh-store.hh +++ b/src/libstore/include/nix/store/legacy-ssh-store.hh @@ -54,7 +54,7 @@ struct LegacySSHStoreConfig : std::enable_shared_from_this ref openStore() const override; - std::string getUri() const override; + StoreReference getReference() const override; }; struct LegacySSHStore : public virtual Store diff --git a/src/libstore/include/nix/store/local-binary-cache-store.hh b/src/libstore/include/nix/store/local-binary-cache-store.hh index 5ca5ca43e..2846a9225 100644 --- a/src/libstore/include/nix/store/local-binary-cache-store.hh +++ b/src/libstore/include/nix/store/local-binary-cache-store.hh @@ -27,7 +27,7 @@ struct LocalBinaryCacheStoreConfig : std::enable_shared_from_this openStore() const override; - std::string getUri() const override; + StoreReference getReference() const override; }; } // namespace nix diff --git a/src/libstore/include/nix/store/local-overlay-store.hh b/src/libstore/include/nix/store/local-overlay-store.hh index 1180f0466..b89d0a1a0 100644 --- a/src/libstore/include/nix/store/local-overlay-store.hh +++ b/src/libstore/include/nix/store/local-overlay-store.hh @@ -88,10 +88,7 @@ struct LocalOverlayStoreConfig : virtual LocalStoreConfig ref openStore() const override; - std::string getUri() const override - { - return "local-overlay://"; - } + StoreReference getReference() const override; protected: /** diff --git a/src/libstore/include/nix/store/local-store.hh b/src/libstore/include/nix/store/local-store.hh index af243d480..3d7e8301a 100644 --- a/src/libstore/include/nix/store/local-store.hh +++ b/src/libstore/include/nix/store/local-store.hh @@ -112,7 +112,7 @@ struct LocalStoreConfig : std::enable_shared_from_this, ref openStore() const override; - std::string getUri() const override; + StoreReference getReference() const override; }; class LocalStore : public virtual IndirectRootStore, public virtual GcStore diff --git a/src/libstore/include/nix/store/s3-binary-cache-store.hh b/src/libstore/include/nix/store/s3-binary-cache-store.hh index ec3aae149..2fe66b0ad 100644 --- a/src/libstore/include/nix/store/s3-binary-cache-store.hh +++ b/src/libstore/include/nix/store/s3-binary-cache-store.hh @@ -107,7 +107,7 @@ public: ref openStore() const override; - std::string getUri() const override; + StoreReference getReference() const override; }; struct S3BinaryCacheStore : virtual BinaryCacheStore diff --git a/src/libstore/include/nix/store/ssh-store.hh b/src/libstore/include/nix/store/ssh-store.hh index ff6c3ed69..9584a1a86 100644 --- a/src/libstore/include/nix/store/ssh-store.hh +++ b/src/libstore/include/nix/store/ssh-store.hh @@ -34,7 +34,7 @@ struct SSHStoreConfig : std::enable_shared_from_this, ref openStore() const override; - std::string getUri() const override; + StoreReference getReference() const override; }; struct MountedSSHStoreConfig : virtual SSHStoreConfig, virtual LocalFSStoreConfig diff --git a/src/libstore/include/nix/store/store-api.hh b/src/libstore/include/nix/store/store-api.hh index 8f09fee48..3e32c49a3 100644 --- a/src/libstore/include/nix/store/store-api.hh +++ b/src/libstore/include/nix/store/store-api.hh @@ -199,11 +199,16 @@ struct StoreConfig : public StoreDirConfig virtual ref openStore() const = 0; /** - * Render the config back to a "store URL". It should round-trip + * Render the config back to a `StoreReference`. It should round-trip * with `resolveStoreConfig` (for stores configs that are * registered). */ - virtual std::string getUri() const; + virtual StoreReference getReference() const; + + std::string getUri() const + { + return getReference().render(); + } }; /** diff --git a/src/libstore/include/nix/store/uds-remote-store.hh b/src/libstore/include/nix/store/uds-remote-store.hh index c77a29a8b..37c239796 100644 --- a/src/libstore/include/nix/store/uds-remote-store.hh +++ b/src/libstore/include/nix/store/uds-remote-store.hh @@ -45,7 +45,7 @@ struct UDSRemoteStoreConfig : std::enable_shared_from_this ref openStore() const override; - std::string getUri() const override; + StoreReference getReference() const override; }; struct UDSRemoteStore : virtual IndirectRootStore, virtual RemoteStore diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index 9592994a1..0435cfa62 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -88,9 +88,16 @@ ref LegacySSHStore::openConnection() return conn; }; -std::string LegacySSHStoreConfig::getUri() const +StoreReference LegacySSHStoreConfig::getReference() const { - return ParsedURL{.scheme = *uriSchemes().begin(), .authority = authority, .query = getQueryParams()}.to_string(); + return { + .variant = + StoreReference::Specified{ + .scheme = *uriSchemes().begin(), + .authority = authority.to_string(), + }, + .params = getQueryParams(), + }; } std::map LegacySSHStore::queryPathInfosUncached(const StorePathSet & paths) diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc index 645a01b09..b5e43de68 100644 --- a/src/libstore/local-binary-cache-store.cc +++ b/src/libstore/local-binary-cache-store.cc @@ -23,9 +23,15 @@ std::string LocalBinaryCacheStoreConfig::doc() ; } -std::string LocalBinaryCacheStoreConfig::getUri() const +StoreReference LocalBinaryCacheStoreConfig::getReference() const { - return "file://" + binaryCacheDir; + return { + .variant = + StoreReference::Specified{ + .scheme = "file", + .authority = binaryCacheDir, + }, + }; } struct LocalBinaryCacheStore : virtual BinaryCacheStore diff --git a/src/libstore/local-overlay-store.cc b/src/libstore/local-overlay-store.cc index 1e8d1429c..2b000b3db 100644 --- a/src/libstore/local-overlay-store.cc +++ b/src/libstore/local-overlay-store.cc @@ -23,6 +23,16 @@ ref LocalOverlayStoreConfig::openStore() const ref{std::dynamic_pointer_cast(shared_from_this())}); } +StoreReference LocalOverlayStoreConfig::getReference() const +{ + return { + .variant = + StoreReference::Specified{ + .scheme = *uriSchemes().begin(), + }, + }; +} + Path LocalOverlayStoreConfig::toUpperPath(const StorePath & path) const { return upperLayer + "/" + path.to_string(); diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index dfffdea6a..f4d1b66ba 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -439,15 +439,15 @@ LocalStore::~LocalStore() } } -std::string LocalStoreConfig::getUri() const +StoreReference LocalStoreConfig::getReference() const { - std::ostringstream oss; - oss << *uriSchemes().begin(); - auto queryParams = getQueryParams(); - if (!queryParams.empty()) - oss << "?"; - oss << encodeQuery(queryParams); - return std::move(oss).str(); + return { + .variant = + StoreReference::Specified{ + .scheme = *uriSchemes().begin(), + }, + .params = getQueryParams(), + }; } int LocalStore::getSchema() diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc index 5f91a8129..84eb63f7f 100644 --- a/src/libstore/s3-binary-cache-store.cc +++ b/src/libstore/s3-binary-cache-store.cc @@ -254,9 +254,15 @@ std::string S3BinaryCacheStoreConfig::doc() ; } -std::string S3BinaryCacheStoreConfig::getUri() const +StoreReference S3BinaryCacheStoreConfig::getReference() const { - return "s3://" + bucketName; + return { + .variant = + StoreReference::Specified{ + .scheme = *uriSchemes().begin(), + .authority = bucketName, + }, + }; } struct S3BinaryCacheStoreImpl : virtual S3BinaryCacheStore diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index d3420186f..dafe14fea 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -25,9 +25,16 @@ std::string SSHStoreConfig::doc() ; } -std::string SSHStoreConfig::getUri() const +StoreReference SSHStoreConfig::getReference() const { - return ParsedURL{.scheme = *uriSchemes().begin(), .authority = authority, .query = getQueryParams()}.to_string(); + return { + .variant = + StoreReference::Specified{ + .scheme = *uriSchemes().begin(), + .authority = authority.to_string(), + }, + .params = getQueryParams(), + }; } struct SSHStore : virtual RemoteStore diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index a720084a0..a30a07952 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -300,9 +300,9 @@ Store::Store(const Store::Config & config) assertLibStoreInitialized(); } -std::string StoreConfig::getUri() const +StoreReference StoreConfig::getReference() const { - return ""; + return {.variant = StoreReference::Auto{}}; } bool Store::PathInfoCacheValue::isKnownNow() diff --git a/src/libstore/uds-remote-store.cc b/src/libstore/uds-remote-store.cc index e1881c602..1d3ecb415 100644 --- a/src/libstore/uds-remote-store.cc +++ b/src/libstore/uds-remote-store.cc @@ -54,14 +54,19 @@ UDSRemoteStore::UDSRemoteStore(ref config) { } -std::string UDSRemoteStoreConfig::getUri() const +StoreReference UDSRemoteStoreConfig::getReference() const { - return path == settings.nixDaemonSocketFile ? // FIXME: Not clear why we return daemon here and not default - // to settings.nixDaemonSocketFile - // - // unix:// with no path also works. Change what we return? - "daemon" - : std::string(*uriSchemes().begin()) + "://" + path; + return { + .variant = + StoreReference::Specified{ + .scheme = *uriSchemes().begin(), + // We return the empty string when the path looks like the + // default path, but we could also just return the path + // verbatim always, to be robust to overall config changes + // at the cost of some verbosity. + .authority = path == settings.nixDaemonSocketFile ? "" : path, + }, + }; } void UDSRemoteStore::Connection::closeWrite() diff --git a/tests/functional/store-info.sh b/tests/functional/store-info.sh index b1e0772b5..7c9257215 100755 --- a/tests/functional/store-info.sh +++ b/tests/functional/store-info.sh @@ -2,12 +2,61 @@ source common.sh +# Different versions of the Nix daemon normalize or don't normalize +# store URLs, plus NIX_REMOTE (per the test suite) might not be using on +# store URL in normal form, so the easiest thing to do is normalize URLs +# after the fact before comparing them for equality. +normalize_nix_store_url () { + local url="$1" + case "$url" in + 'auto' ) + # Need to actually ask Nix in this case + echo "$defaultStore" + ;; + 'local://'* ) + # To not be captured by next pattern + echo "$url" + ;; + local | 'local?'* ) + echo "local://${url#local}" + ;; + daemon | 'daemon?'* ) + echo "unix://${url#daemon}" + ;; + * ) + echo "$url" + ;; + esac +} + STORE_INFO=$(nix store info 2>&1) LEGACY_STORE_INFO=$(nix store ping 2>&1) # alias to nix store info STORE_INFO_JSON=$(nix store info --json) -echo "$STORE_INFO" | grep "Store URL: ${NIX_REMOTE}" -echo "$LEGACY_STORE_INFO" | grep "Store URL: ${NIX_REMOTE}" +defaultStore="$(normalize_nix_store_url "$(echo "$STORE_INFO_JSON" | jq -r ".url")")" + +# Test cases for `normalize_nix_store_url` itself + +# Normalize local store +[[ "$(normalize_nix_store_url "local://")" = "local://" ]] +[[ "$(normalize_nix_store_url "local")" = "local://" ]] +[[ "$(normalize_nix_store_url "local?foo=bar")" = "local://?foo=bar" ]] + +# Normalize unix domain socket remote store +[[ "$(normalize_nix_store_url "unix://")" = "unix://" ]] +[[ "$(normalize_nix_store_url "daemon")" = "unix://" ]] +[[ "$(normalize_nix_store_url "daemon?x=y")" = "unix://?x=y" ]] + +# otherwise unchanged +[[ "$(normalize_nix_store_url "https://site")" = "https://site" ]] + +nixRemoteOrDefault=$(normalize_nix_store_url "${NIX_REMOTE:-"auto"}") + +check_human_readable () { + [[ "$(normalize_nix_store_url "$(echo "$1" | grep 'Store URL:' | sed 's^Store URL: ^^')")" = "${nixRemoteOrDefault}" ]] +} +check_human_readable "$STORE_INFO" +check_human_readable "$LEGACY_STORE_INFO" if [[ -v NIX_DAEMON_PACKAGE ]] && isDaemonNewer "2.7.0pre20220126"; then DAEMON_VERSION=$("$NIX_DAEMON_PACKAGE"/bin/nix daemon --version | cut -d' ' -f3) @@ -21,4 +70,4 @@ expect 127 NIX_REMOTE=unix:"$PWD"/store nix store info || \ TODO_NixOS -[[ "$(echo "$STORE_INFO_JSON" | jq -r ".url")" == "${NIX_REMOTE:-local}" ]] +[[ "$(normalize_nix_store_url "$(echo "$STORE_INFO_JSON" | jq -r ".url")")" == "${nixRemoteOrDefault}" ]]