From caabc4f64889d5a4c47d6102b3aa1d3c80bbc107 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 12 Jul 2023 23:33:43 -0400 Subject: [PATCH 1/5] Feature gate `DownstreamPlaceholder::unknownCaOutput` This is a part of CA derivations that we forgot to put behind the experimental feature. This was caught by @fricklerhandwerk in https://github.com/NixOS/nix/pull/8369#discussion_r1258133719 --- src/libexpr/eval.cc | 5 ++-- src/libexpr/eval.hh | 5 +++- src/libexpr/primops.cc | 25 ++++++++++---------- src/libexpr/tests/derived-path.cc | 9 ++++++- src/libstore/derivations.cc | 8 ++++--- src/libstore/downstream-placeholder.cc | 4 +++- src/libstore/downstream-placeholder.hh | 5 +++- src/libstore/tests/downstream-placeholder.cc | 16 +++++++++---- src/nix/app.cc | 11 +++++---- 9 files changed, 57 insertions(+), 31 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index be1bdb806..7071815d4 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1068,14 +1068,15 @@ void EvalState::mkOutputString( Value & value, const StorePath & drvPath, const std::string outputName, - std::optional optOutputPath) + std::optional optOutputPath, + const ExperimentalFeatureSettings & xpSettings) { value.mkString( optOutputPath ? store->printStorePath(*std::move(optOutputPath)) /* Downstream we would substitute this for an actual path once we build the floating CA derivation */ - : DownstreamPlaceholder::unknownCaOutput(drvPath, outputName).render(), + : DownstreamPlaceholder::unknownCaOutput(drvPath, outputName, xpSettings).render(), NixStringContext { NixStringContextElem::Built { .drvPath = drvPath, diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 277e77ad5..7f11ce71d 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -689,12 +689,15 @@ public: * be passed if and only if output store object is input-addressed. * Will be printed to form string if passed, otherwise a placeholder * will be used (see `DownstreamPlaceholder`). + * + * @param xpSettings Stop-gap to avoid globals during unit tests. */ void mkOutputString( Value & value, const StorePath & drvPath, const std::string outputName, - std::optional optOutputPath); + std::optional optOutputPath, + const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings); void concatLists(Value & v, size_t nrLists, Value * * lists, const PosIdx pos, std::string_view errorCtx); diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 8a61e57cc..5dab06f26 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -83,22 +83,21 @@ StringMap EvalState::realiseContext(const NixStringContext & context) for (auto & d : drvs) buildReqs.emplace_back(DerivedPath { d }); store->buildPaths(buildReqs); - /* Get all the output paths corresponding to the placeholders we had */ for (auto & drv : drvs) { auto outputs = resolveDerivedPath(*store, drv); for (auto & [outputName, outputPath] : outputs) { - res.insert_or_assign( - DownstreamPlaceholder::unknownCaOutput(drv.drvPath, outputName).render(), - store->printStorePath(outputPath) - ); - } - } - - /* Add the output of this derivations to the allowed - paths. */ - if (allowedPaths) { - for (auto & [_placeholder, outputPath] : res) { - allowPath(store->toRealPath(outputPath)); + /* Add the output of this derivations to the allowed + paths. */ + if (allowedPaths) { + allowPath(outputPath); + } + /* Get all the output paths corresponding to the placeholders we had */ + if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) { + res.insert_or_assign( + DownstreamPlaceholder::unknownCaOutput(drv.drvPath, outputName).render(), + store->printStorePath(outputPath) + ); + } } } diff --git a/src/libexpr/tests/derived-path.cc b/src/libexpr/tests/derived-path.cc index 8210efef2..c713fe28a 100644 --- a/src/libexpr/tests/derived-path.cc +++ b/src/libexpr/tests/derived-path.cc @@ -37,8 +37,15 @@ RC_GTEST_FIXTURE_PROP( prop_built_path_placeholder_round_trip, (const StorePath & drvPath, const StorePathName & outputName)) { + /** + * We set these in tests rather than the regular globals so we don't have + * to worry about race conditions if the tests run concurrently. + */ + ExperimentalFeatureSettings mockXpSettings; + mockXpSettings.set("experimental-features", "ca-derivations"); + auto * v = state.allocValue(); - state.mkOutputString(*v, drvPath, outputName.name, std::nullopt); + state.mkOutputString(*v, drvPath, outputName.name, std::nullopt, mockXpSettings); auto [d, _] = state.coerceToDerivedPathUnchecked(noPos, *v, ""); DerivedPath::Built b { .drvPath = drvPath, diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 6f63685d4..234d70ec5 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -878,9 +878,11 @@ std::optional Derivation::tryResolve( for (auto & [inputDrv, inputOutputs] : inputDrvs) { for (auto & outputName : inputOutputs) { if (auto actualPath = get(inputDrvOutputs, { inputDrv, outputName })) { - inputRewrites.emplace( - DownstreamPlaceholder::unknownCaOutput(inputDrv, outputName).render(), - store.printStorePath(*actualPath)); + if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) { + inputRewrites.emplace( + DownstreamPlaceholder::unknownCaOutput(inputDrv, outputName).render(), + store.printStorePath(*actualPath)); + } resolved.inputSrcs.insert(*actualPath); } else { warn("output '%s' of input '%s' missing, aborting the resolving", diff --git a/src/libstore/downstream-placeholder.cc b/src/libstore/downstream-placeholder.cc index 1752738f2..d623c05e2 100644 --- a/src/libstore/downstream-placeholder.cc +++ b/src/libstore/downstream-placeholder.cc @@ -11,8 +11,10 @@ std::string DownstreamPlaceholder::render() const DownstreamPlaceholder DownstreamPlaceholder::unknownCaOutput( const StorePath & drvPath, - std::string_view outputName) + std::string_view outputName, + const ExperimentalFeatureSettings & xpSettings) { + xpSettings.require(Xp::CaDerivations); auto drvNameWithExtension = drvPath.name(); auto drvName = drvNameWithExtension.substr(0, drvNameWithExtension.size() - 4); auto clearText = "nix-upstream-output:" + std::string { drvPath.hashPart() } + ":" + outputPathName(drvName, outputName); diff --git a/src/libstore/downstream-placeholder.hh b/src/libstore/downstream-placeholder.hh index f0c0dee77..97f77e6b8 100644 --- a/src/libstore/downstream-placeholder.hh +++ b/src/libstore/downstream-placeholder.hh @@ -52,10 +52,13 @@ public: * * The derivation itself is known (we have a store path for it), but * the output doesn't yet have a known store path. + * + * @param xpSettings Stop-gap to avoid globals during unit tests. */ static DownstreamPlaceholder unknownCaOutput( const StorePath & drvPath, - std::string_view outputName); + std::string_view outputName, + const ExperimentalFeatureSettings & xpSettings = experimentalFeatureSettings); /** * Create a placehold for the output of an unknown derivation. diff --git a/src/libstore/tests/downstream-placeholder.cc b/src/libstore/tests/downstream-placeholder.cc index ec3e1000f..fd29530ac 100644 --- a/src/libstore/tests/downstream-placeholder.cc +++ b/src/libstore/tests/downstream-placeholder.cc @@ -5,17 +5,24 @@ namespace nix { TEST(DownstreamPlaceholder, unknownCaOutput) { + /** + * We set these in tests rather than the regular globals so we don't have + * to worry about race conditions if the tests run concurrently. + */ + ExperimentalFeatureSettings mockXpSettings; + mockXpSettings.set("experimental-features", "ca-derivations"); + ASSERT_EQ( DownstreamPlaceholder::unknownCaOutput( StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo.drv" }, - "out").render(), + "out", + mockXpSettings).render(), "/0c6rn30q4frawknapgwq386zq358m8r6msvywcvc89n6m5p2dgbz"); } TEST(DownstreamPlaceholder, unknownDerivation) { /** - * We set these in tests rather than the regular globals so we don't have - * to worry about race conditions if the tests run concurrently. + * Same reason as above */ ExperimentalFeatureSettings mockXpSettings; mockXpSettings.set("experimental-features", "dynamic-derivations ca-derivations"); @@ -24,7 +31,8 @@ TEST(DownstreamPlaceholder, unknownDerivation) { DownstreamPlaceholder::unknownDerivation( DownstreamPlaceholder::unknownCaOutput( StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo.drv.drv" }, - "out"), + "out", + mockXpSettings), "out", mockXpSettings).render(), "/0gn6agqxjyyalf0dpihgyf49xq5hqxgw100f0wydnj6yqrhqsb3w"); diff --git a/src/nix/app.cc b/src/nix/app.cc index e678b54f0..e0f68b4fc 100644 --- a/src/nix/app.cc +++ b/src/nix/app.cc @@ -22,11 +22,12 @@ StringPairs resolveRewrites( StringPairs res; for (auto & dep : dependencies) if (auto drvDep = std::get_if(&dep.path)) - for (auto & [ outputName, outputPath ] : drvDep->outputs) - res.emplace( - DownstreamPlaceholder::unknownCaOutput(drvDep->drvPath, outputName).render(), - store.printStorePath(outputPath) - ); + if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) + for (auto & [ outputName, outputPath ] : drvDep->outputs) + res.emplace( + DownstreamPlaceholder::unknownCaOutput(drvDep->drvPath, outputName).render(), + store.printStorePath(outputPath) + ); return res; } From d9e7758f4756c10592407b3fe99dc253ef56eac9 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 1 Aug 2023 16:07:20 +0200 Subject: [PATCH 2/5] Don't require .tar/.zip extension for tarball flakerefs Special-casing the file name is rather ugly, so we shouldn't do that. So now any {file,http,https} URL is handled by TarballInputScheme, except for non-flake inputs (i.e. inputs that have the attribute `flake = false`). --- src/libexpr/flake/flakeref.cc | 6 +++--- src/libfetchers/fetchers.cc | 8 ++++---- src/libfetchers/fetchers.hh | 6 +++--- src/libfetchers/git.cc | 2 +- src/libfetchers/github.cc | 2 +- src/libfetchers/indirect.cc | 2 +- src/libfetchers/mercurial.cc | 2 +- src/libfetchers/path.cc | 2 +- src/libfetchers/tarball.cc | 18 +++++++++--------- tests/fetchTree-file.sh | 14 +++++++++++++- 10 files changed, 37 insertions(+), 25 deletions(-) diff --git a/src/libexpr/flake/flakeref.cc b/src/libexpr/flake/flakeref.cc index 08adbe0c9..d3fa1d557 100644 --- a/src/libexpr/flake/flakeref.cc +++ b/src/libexpr/flake/flakeref.cc @@ -105,7 +105,7 @@ std::pair parseFlakeRefWithFragment( }; return std::make_pair( - FlakeRef(Input::fromURL(parsedURL), ""), + FlakeRef(Input::fromURL(parsedURL, isFlake), ""), percentDecode(match.str(6))); } @@ -176,7 +176,7 @@ std::pair parseFlakeRefWithFragment( parsedURL.query.insert_or_assign("shallow", "1"); return std::make_pair( - FlakeRef(Input::fromURL(parsedURL), getOr(parsedURL.query, "dir", "")), + FlakeRef(Input::fromURL(parsedURL, isFlake), getOr(parsedURL.query, "dir", "")), fragment); } @@ -204,7 +204,7 @@ std::pair parseFlakeRefWithFragment( std::string fragment; std::swap(fragment, parsedURL.fragment); - auto input = Input::fromURL(parsedURL); + auto input = Input::fromURL(parsedURL, isFlake); input.parent = baseDir; return std::make_pair( diff --git a/src/libfetchers/fetchers.cc b/src/libfetchers/fetchers.cc index f86c0604e..e683b9f80 100644 --- a/src/libfetchers/fetchers.cc +++ b/src/libfetchers/fetchers.cc @@ -13,9 +13,9 @@ void registerInputScheme(std::shared_ptr && inputScheme) inputSchemes->push_back(std::move(inputScheme)); } -Input Input::fromURL(const std::string & url) +Input Input::fromURL(const std::string & url, bool requireTree) { - return fromURL(parseURL(url)); + return fromURL(parseURL(url), requireTree); } static void fixupInput(Input & input) @@ -31,10 +31,10 @@ static void fixupInput(Input & input) input.locked = true; } -Input Input::fromURL(const ParsedURL & url) +Input Input::fromURL(const ParsedURL & url, bool requireTree) { for (auto & inputScheme : *inputSchemes) { - auto res = inputScheme->inputFromURL(url); + auto res = inputScheme->inputFromURL(url, requireTree); if (res) { res->scheme = inputScheme; fixupInput(*res); diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index d0738f619..6e10e9513 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -44,9 +44,9 @@ struct Input std::optional parent; public: - static Input fromURL(const std::string & url); + static Input fromURL(const std::string & url, bool requireTree = true); - static Input fromURL(const ParsedURL & url); + static Input fromURL(const ParsedURL & url, bool requireTree = true); static Input fromAttrs(Attrs && attrs); @@ -129,7 +129,7 @@ struct InputScheme virtual ~InputScheme() { } - virtual std::optional inputFromURL(const ParsedURL & url) const = 0; + virtual std::optional inputFromURL(const ParsedURL & url, bool requireTree) const = 0; virtual std::optional inputFromAttrs(const Attrs & attrs) const = 0; diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index be5842d53..f8d89ab2f 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -256,7 +256,7 @@ std::pair fetchFromWorkdir(ref store, Input & input, co struct GitInputScheme : InputScheme { - std::optional inputFromURL(const ParsedURL & url) const override + std::optional inputFromURL(const ParsedURL & url, bool requireTree) const override { if (url.scheme != "git" && url.scheme != "git+http" && diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index 80598e7f8..291f457f0 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -30,7 +30,7 @@ struct GitArchiveInputScheme : InputScheme virtual std::optional> accessHeaderFromToken(const std::string & token) const = 0; - std::optional inputFromURL(const ParsedURL & url) const override + std::optional inputFromURL(const ParsedURL & url, bool requireTree) const override { if (url.scheme != type()) return {}; diff --git a/src/libfetchers/indirect.cc b/src/libfetchers/indirect.cc index b99504a16..4874a43ff 100644 --- a/src/libfetchers/indirect.cc +++ b/src/libfetchers/indirect.cc @@ -7,7 +7,7 @@ std::regex flakeRegex("[a-zA-Z][a-zA-Z0-9_-]*", std::regex::ECMAScript); struct IndirectInputScheme : InputScheme { - std::optional inputFromURL(const ParsedURL & url) const override + std::optional inputFromURL(const ParsedURL & url, bool requireTree) const override { if (url.scheme != "flake") return {}; diff --git a/src/libfetchers/mercurial.cc b/src/libfetchers/mercurial.cc index 86e8f81f4..51fd1ed42 100644 --- a/src/libfetchers/mercurial.cc +++ b/src/libfetchers/mercurial.cc @@ -43,7 +43,7 @@ static std::string runHg(const Strings & args, const std::optional struct MercurialInputScheme : InputScheme { - std::optional inputFromURL(const ParsedURL & url) const override + std::optional inputFromURL(const ParsedURL & url, bool requireTree) const override { if (url.scheme != "hg+http" && url.scheme != "hg+https" && diff --git a/src/libfetchers/path.cc b/src/libfetchers/path.cc index 61541e69d..01f1be978 100644 --- a/src/libfetchers/path.cc +++ b/src/libfetchers/path.cc @@ -6,7 +6,7 @@ namespace nix::fetchers { struct PathInputScheme : InputScheme { - std::optional inputFromURL(const ParsedURL & url) const override + std::optional inputFromURL(const ParsedURL & url, bool requireTree) const override { if (url.scheme != "path") return {}; diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index a012234e0..107d38e92 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -194,11 +194,11 @@ struct CurlInputScheme : InputScheme || hasSuffix(path, ".tar.zst"); } - virtual bool isValidURL(const ParsedURL & url) const = 0; + virtual bool isValidURL(const ParsedURL & url, bool requireTree) const = 0; - std::optional inputFromURL(const ParsedURL & _url) const override + std::optional inputFromURL(const ParsedURL & _url, bool requireTree) const override { - if (!isValidURL(_url)) + if (!isValidURL(_url, requireTree)) return std::nullopt; Input input; @@ -265,13 +265,13 @@ struct FileInputScheme : CurlInputScheme { const std::string inputType() const override { return "file"; } - bool isValidURL(const ParsedURL & url) const override + bool isValidURL(const ParsedURL & url, bool requireTree) const override { auto parsedUrlScheme = parseUrlScheme(url.scheme); return transportUrlSchemes.count(std::string(parsedUrlScheme.transport)) && (parsedUrlScheme.application - ? parsedUrlScheme.application.value() == inputType() - : !hasTarballExtension(url.path)); + ? parsedUrlScheme.application.value() == inputType() + : (!requireTree && !hasTarballExtension(url.path))); } std::pair fetch(ref store, const Input & input) override @@ -285,14 +285,14 @@ struct TarballInputScheme : CurlInputScheme { const std::string inputType() const override { return "tarball"; } - bool isValidURL(const ParsedURL & url) const override + bool isValidURL(const ParsedURL & url, bool requireTree) const override { auto parsedUrlScheme = parseUrlScheme(url.scheme); return transportUrlSchemes.count(std::string(parsedUrlScheme.transport)) && (parsedUrlScheme.application - ? parsedUrlScheme.application.value() == inputType() - : hasTarballExtension(url.path)); + ? parsedUrlScheme.application.value() == inputType() + : (requireTree || hasTarballExtension(url.path))); } std::pair fetch(ref store, const Input & _input) override diff --git a/tests/fetchTree-file.sh b/tests/fetchTree-file.sh index fe569cfb8..6395c133d 100644 --- a/tests/fetchTree-file.sh +++ b/tests/fetchTree-file.sh @@ -27,6 +27,7 @@ test_file_flake_input () { mkdir inputs echo foo > inputs/test_input_file + echo '{ outputs = { self }: { }; }' > inputs/flake.nix tar cfa test_input.tar.gz inputs cp test_input.tar.gz test_input_no_ext input_tarball_hash="$(nix hash path test_input.tar.gz)" @@ -50,6 +51,9 @@ test_file_flake_input () { url = "file+file://$PWD/test_input.tar.gz"; flake = false; }; + inputs.flake_no_ext = { + url = "file://$PWD/test_input_no_ext"; + }; outputs = { ... }: {}; } EOF @@ -58,7 +62,7 @@ EOF nix eval --file - < Date: Fri, 4 Aug 2023 23:11:08 +0200 Subject: [PATCH 3/5] nix/why-depends: fix output of `--precise` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I haven't checked when this was exactly introduced, but on Nix 2.16 I realized that the additional lines inserted when using `--precise` are completely separated from the tree: nix why-depends /nix/store/ccgr4faaxys39s091qridxg1947lggh4-evcxr-0.14.2 /nix/store/b7hvml0m3qmqraz1022fwvyyg6fc1vdy-gcc-12.2.0 --precise --extra-experimental-features nix-command /nix/store/ccgr4faaxys39s091qridxg1947lggh4-evcxr-0.14.2 → /nix/store/lcf37pgp3rgww67v9x2990hbfwx96c1w-gcc-wrapper-12.2.0 → /nix/store/b7hvml0m3qmqraz1022fwvyyg6fc1vdy-gcc-12.2.0 └───bin/evcxr: …':'}.PATH=${PATH/':''/nix/store/lcf37pgp3rgww67v9x2990hbfwx96c1w-gcc-wrapper-12.2.0/bin'':'/':'}… └───bin/cpp: …k disable=SC2193.[[ "/nix/store/b7hvml0m3qmqraz1022fwvyyg6fc1vdy-gcc-12.2.0/bin/cpp" = *++ ]] &&… This is apparently because `std::cout` is buffered and flushed in the end whereas the rest of the output isn't. The fix is rather simple, just use `logger->cout` as it's already the case for the rest of the code. This way we also don't need to insert additional newlines in the `hits` map since that's something the logger takes care of. Also added a small test to make sure that the layout of this is somehow tested to reduce the risk of further regressions here. --- src/nix/why-depends.cc | 10 +++++----- tests/why-depends.sh | 5 +++++ 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/nix/why-depends.cc b/src/nix/why-depends.cc index a3a9dc698..592de773c 100644 --- a/src/nix/why-depends.cc +++ b/src/nix/why-depends.cc @@ -239,7 +239,7 @@ struct CmdWhyDepends : SourceExprCommand, MixOperateOnOptions if (pos != std::string::npos) { size_t margin = 32; auto pos2 = pos >= margin ? pos - margin : 0; - hits[hash].emplace_back(fmt("%s: …%s…\n", + hits[hash].emplace_back(fmt("%s: …%s…", p2, hilite(filterPrintable( std::string(contents, pos2, pos - pos2 + hash.size() + margin)), @@ -255,7 +255,7 @@ struct CmdWhyDepends : SourceExprCommand, MixOperateOnOptions for (auto & hash : hashes) { auto pos = target.find(hash); if (pos != std::string::npos) - hits[hash].emplace_back(fmt("%s -> %s\n", p2, + hits[hash].emplace_back(fmt("%s -> %s", p2, hilite(target, pos, StorePath::HashLen, getColour(hash)))); } } @@ -272,9 +272,9 @@ struct CmdWhyDepends : SourceExprCommand, MixOperateOnOptions for (auto & hit : hits[hash]) { bool first = hit == *hits[hash].begin(); - std::cout << tailPad - << (first ? (last ? treeLast : treeConn) : (last ? treeNull : treeLine)) - << hit; + logger->cout("%s%s%s", tailPad, + (first ? (last ? treeLast : treeConn) : (last ? treeNull : treeLine)), + hit); if (!all) break; } diff --git a/tests/why-depends.sh b/tests/why-depends.sh index b35a0d1cf..9680bf80e 100644 --- a/tests/why-depends.sh +++ b/tests/why-depends.sh @@ -22,3 +22,8 @@ echo "$PRECISE_WHY_DEPENDS_OUTPUT" | grepQuiet input-2 # But only the “precise” one should refer to `reference-to-input-2` echo "$FAST_WHY_DEPENDS_OUTPUT" | grepQuietInverse reference-to-input-2 echo "$PRECISE_WHY_DEPENDS_OUTPUT" | grepQuiet reference-to-input-2 + +<<<"$PRECISE_WHY_DEPENDS_OUTPUT" sed -n '2p' | grepQuiet "└───reference-to-input-2 -> " +<<<"$PRECISE_WHY_DEPENDS_OUTPUT" sed -n '3p' | grep " →" | grepQuiet "dependencies-input-2" +<<<"$PRECISE_WHY_DEPENDS_OUTPUT" sed -n '4p' | grepQuiet " └───input0: …" # in input-2, file input0 +<<<"$PRECISE_WHY_DEPENDS_OUTPUT" sed -n '5p' | grep " →" | grepQuiet "dependencies-input-0" # is dependencies-input-0 referenced From 3fefc2b28494468c7a6078430eaaf8a6c8f17230 Mon Sep 17 00:00:00 2001 From: Felix Uhl Date: Fri, 28 Jul 2023 22:23:56 +0200 Subject: [PATCH 4/5] Fix derivation load assertion errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When loading a derivation from a JSON, malformed input would trigger cryptic "assertion failed" errors. Simply replacing calls to `operator []` with calls to `.at()` was not enough, as this would cause json.execptions to be printed verbatim. Display nice error messages instead and give some indication where the error happened. *Before:* ``` $ echo 4 | nix derivation add error: [json.exception.type_error.305] cannot use operator[] with a string argument with number $ nix derivation show nixpkgs#hello | nix derivation add Assertion failed: (it != m_value.object->end()), function operator[], file /nix/store/8h9pxgq1776ns6qi5arx08ifgnhmgl22-nlohmann_json-3.11.2/include/nlohmann/json.hpp, line 2135. $ nix derivation show nixpkgs#hello | jq '.[] | .name = 5' | nix derivation add error: [json.exception.type_error.302] type must be string, but is object $ nix derivation show nixpkgs#hello | jq '.[] | .outputs = { out: "/nix/store/8j3f8j-hello" }' | nix derivation add error: [json.exception.type_error.302] type must be object, but is string ``` *After:* ``` $ echo 4 | nix derivation add error: Expected JSON of derivation to be of type 'object', but it is of type 'number' $ nix derivation show nixpkgs#hello | nix derivation add error: Expected JSON object to contain key 'name' but it doesn't $ nix derivation show nixpkgs#hello | jq '.[] | .name = 5' | nix derivation add error: Expected JSON value to be of type 'string' but it is of type 'number' $ nix derivation show nixpkgs#hello | jq '.[] | .outputs = { out: "/nix/store/8j3f8j-hello" }' | nix derivation add error: … while reading key 'outputs' error: Expected JSON value to be of type 'object' but it is of type 'string' ``` --- doc/manual/src/release-notes/rl-next.md | 2 ++ src/libstore/derivations.cc | 40 +++++++++++++++++-------- src/libutil/json-utils.cc | 24 +++++++++++++++ src/libutil/json-utils.hh | 22 ++++++++++++++ 4 files changed, 76 insertions(+), 12 deletions(-) diff --git a/doc/manual/src/release-notes/rl-next.md b/doc/manual/src/release-notes/rl-next.md index df30ce83d..526b64fde 100644 --- a/doc/manual/src/release-notes/rl-next.md +++ b/doc/manual/src/release-notes/rl-next.md @@ -8,3 +8,5 @@ These functions are useful for converting between flake references encoded as attribute sets and URLs. - [`builtins.toJSON`](@docroot@/language/builtins.md#builtins-parseFlakeRef) now prints [--show-trace](@docroot@/command-ref/conf-file.html#conf-show-trace) items for the path in which it finds an evaluation error. + +- Error messages regarding malformed input to [`derivation add`](@docroot@/command-ref/new-cli/nix3-derivation-add.md) are now clearer and more detailed. diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index b831b2fe5..8a84bb1c5 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -993,6 +993,7 @@ DerivationOutput DerivationOutput::fromJSON( const ExperimentalFeatureSettings & xpSettings) { std::set keys; + ensureType(_json, nlohmann::detail::value_t::object); auto json = (std::map) _json; for (const auto & [key, _] : json) @@ -1097,36 +1098,51 @@ Derivation Derivation::fromJSON( const Store & store, const nlohmann::json & json) { + using nlohmann::detail::value_t; + Derivation res; - res.name = json["name"]; + ensureType(json, value_t::object); - { - auto & outputsObj = json["outputs"]; + res.name = ensureType(valueAt(json, "name"), value_t::string); + + try { + auto & outputsObj = ensureType(valueAt(json, "outputs"), value_t::object); for (auto & [outputName, output] : outputsObj.items()) { res.outputs.insert_or_assign( outputName, DerivationOutput::fromJSON(store, res.name, outputName, output)); } + } catch (Error & e) { + e.addTrace({}, "while reading key 'outputs'"); + throw; } - { - auto & inputsList = json["inputSrcs"]; + try { + auto & inputsList = ensureType(valueAt(json, "inputSrcs"), value_t::array); for (auto & input : inputsList) res.inputSrcs.insert(store.parseStorePath(static_cast(input))); + } catch (Error & e) { + e.addTrace({}, "while reading key 'inputSrcs'"); + throw; } - { - auto & inputDrvsObj = json["inputDrvs"]; - for (auto & [inputDrvPath, inputOutputs] : inputDrvsObj.items()) + try { + auto & inputDrvsObj = ensureType(valueAt(json, "inputDrvs"), value_t::object); + for (auto & [inputDrvPath, inputOutputs] : inputDrvsObj.items()) { + ensureType(inputOutputs, value_t::array); res.inputDrvs[store.parseStorePath(inputDrvPath)] = static_cast(inputOutputs); + } + } catch (Error & e) { + e.addTrace({}, "while reading key 'inputDrvs'"); + throw; } - res.platform = json["system"]; - res.builder = json["builder"]; - res.args = json["args"]; - res.env = json["env"]; + res.platform = ensureType(valueAt(json, "system"), value_t::string); + res.builder = ensureType(valueAt(json, "builder"), value_t::string); + res.args = ensureType(valueAt(json, "args"), value_t::array); + res.env = ensureType(valueAt(json, "env"), value_t::object); return res; } diff --git a/src/libutil/json-utils.cc b/src/libutil/json-utils.cc index d7220e71d..61cef743d 100644 --- a/src/libutil/json-utils.cc +++ b/src/libutil/json-utils.cc @@ -1,4 +1,5 @@ #include "json-utils.hh" +#include "error.hh" namespace nix { @@ -16,4 +17,27 @@ nlohmann::json * get(nlohmann::json & map, const std::string & key) return &*i; } +const nlohmann::json & valueAt( + const nlohmann::json & map, + const std::string & key) +{ + if (!map.contains(key)) + throw Error("Expected JSON object to contain key '%s' but it doesn't", key); + + return map[key]; +} + +const nlohmann::json & ensureType( + const nlohmann::json & value, + nlohmann::json::value_type expectedType + ) +{ + if (value.type() != expectedType) + throw Error( + "Expected JSON value to be of type '%s' but it is of type '%s'", + nlohmann::json(expectedType).type_name(), + value.type_name()); + + return value; +} } diff --git a/src/libutil/json-utils.hh b/src/libutil/json-utils.hh index 5e63c1af4..77c63595c 100644 --- a/src/libutil/json-utils.hh +++ b/src/libutil/json-utils.hh @@ -10,6 +10,28 @@ const nlohmann::json * get(const nlohmann::json & map, const std::string & key); nlohmann::json * get(nlohmann::json & map, const std::string & key); +/** + * Get the value of a json object at a key safely, failing + * with a Nix Error if the key does not exist. + * + * Use instead of nlohmann::json::at() to avoid ugly exceptions. + * + * _Does not check whether `map` is an object_, use `ensureType` for that. + */ +const nlohmann::json & valueAt( + const nlohmann::json & map, + const std::string & key); + +/** + * Ensure the type of a json object is what you expect, failing + * with a Nix Error if it isn't. + * + * Use before type conversions and element access to avoid ugly exceptions. + */ +const nlohmann::json & ensureType( + const nlohmann::json & value, + nlohmann::json::value_type expectedType); + /** * For `adl_serializer>` below, we need to track what * types are not already using `null`. Only for them can we use `null` From ad410abbe0d66a72927bd715af355d88a4e939d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Fri, 12 May 2023 09:56:39 +0200 Subject: [PATCH 5/5] Stabilize `discard-references` It has been there for a few releases now (landed in 2.14.0), doesn't seem to cause any major issue and is wanted in a few places (https://github.com/NixOS/nix/pull/7087#issuecomment-1544471346). --- doc/manual/src/language/advanced-attributes.md | 10 ---------- doc/manual/src/release-notes/rl-next.md | 6 ++++++ src/libstore/build/local-derivation-goal.cc | 1 - src/libutil/experimental-features.cc | 11 +---------- src/libutil/experimental-features.hh | 1 - tests/check-refs.sh | 6 ++++-- 6 files changed, 11 insertions(+), 24 deletions(-) diff --git a/doc/manual/src/language/advanced-attributes.md b/doc/manual/src/language/advanced-attributes.md index 307971434..5e8aaeba0 100644 --- a/doc/manual/src/language/advanced-attributes.md +++ b/doc/manual/src/language/advanced-attributes.md @@ -320,16 +320,6 @@ Derivations can declare some infrequently used optional attributes. ``` - [`unsafeDiscardReferences`]{#adv-attr-unsafeDiscardReferences}\ - > **Warning** - > This attribute is part of an [experimental feature](@docroot@/contributing/experimental-features.md). - > - > To use this attribute, you must enable the - > [`discard-references`](@docroot@/contributing/experimental-features.md#xp-feature-discard-references) experimental feature. - > For example, in [nix.conf](../command-ref/conf-file.md) you could add: - > - > ``` - > extra-experimental-features = discard-references - > ``` When using [structured attributes](#adv-attr-structuredAttrs), the attribute `unsafeDiscardReferences` is an attribute set with a boolean value for each output name. diff --git a/doc/manual/src/release-notes/rl-next.md b/doc/manual/src/release-notes/rl-next.md index 526b64fde..6516a2663 100644 --- a/doc/manual/src/release-notes/rl-next.md +++ b/doc/manual/src/release-notes/rl-next.md @@ -10,3 +10,9 @@ - [`builtins.toJSON`](@docroot@/language/builtins.md#builtins-parseFlakeRef) now prints [--show-trace](@docroot@/command-ref/conf-file.html#conf-show-trace) items for the path in which it finds an evaluation error. - Error messages regarding malformed input to [`derivation add`](@docroot@/command-ref/new-cli/nix3-derivation-add.md) are now clearer and more detailed. + +- The `discard-references` feature has been stabilized. + This means that the + [unsafeDiscardReferences](@docroot@/contributing/experimental-features.md#xp-feature-discard-references) + attribute is no longer guarded by an experimental flag and can be used + freely. diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index b7a27490c..8b7fbdcda 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2307,7 +2307,6 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() bool discardReferences = false; if (auto structuredAttrs = parsedDrv->getStructuredAttrs()) { if (auto udr = get(*structuredAttrs, "unsafeDiscardReferences")) { - experimentalFeatureSettings.require(Xp::DiscardReferences); if (auto output = get(*udr, outputName)) { if (!output->is_boolean()) throw Error("attribute 'unsafeDiscardReferences.\"%s\"' of derivation '%s' must be a Boolean", outputName, drvPath.to_string()); diff --git a/src/libutil/experimental-features.cc b/src/libutil/experimental-features.cc index 7c4112d32..782331283 100644 --- a/src/libutil/experimental-features.cc +++ b/src/libutil/experimental-features.cc @@ -12,7 +12,7 @@ struct ExperimentalFeatureDetails std::string_view description; }; -constexpr std::array xpFeatureDetails = {{ +constexpr std::array xpFeatureDetails = {{ { .tag = Xp::CaDerivations, .name = "ca-derivations", @@ -182,15 +182,6 @@ constexpr std::array xpFeatureDetails = {{ the [`use-cgroups`](#conf-use-cgroups) setting for details. )", }, - { - .tag = Xp::DiscardReferences, - .name = "discard-references", - .description = R"( - Allow the use of the [`unsafeDiscardReferences`](@docroot@/language/advanced-attributes.html#adv-attr-unsafeDiscardReferences) attribute in derivations - that use [structured attributes](@docroot@/language/advanced-attributes.html#adv-attr-structuredAttrs). This disables scanning of outputs for - runtime dependencies. - )", - }, { .tag = Xp::DaemonTrustOverride, .name = "daemon-trust-override", diff --git a/src/libutil/experimental-features.hh b/src/libutil/experimental-features.hh index faf2e9398..add592ae6 100644 --- a/src/libutil/experimental-features.hh +++ b/src/libutil/experimental-features.hh @@ -27,7 +27,6 @@ enum struct ExperimentalFeature ReplFlake, AutoAllocateUids, Cgroups, - DiscardReferences, DaemonTrustOverride, DynamicDerivations, ParseTomlTimestamps, diff --git a/tests/check-refs.sh b/tests/check-refs.sh index 2778e491d..3b587d1e5 100644 --- a/tests/check-refs.sh +++ b/tests/check-refs.sh @@ -42,8 +42,10 @@ nix-build -o $RESULT check-refs.nix -A test7 nix-build -o $RESULT check-refs.nix -A test10 if isDaemonNewer 2.12pre20230103; then - enableFeatures discard-references - restartDaemon + if ! isDaemonNewer 2.16.0; then + enableFeatures discard-references + restartDaemon + fi # test11 should succeed. test11=$(nix-build -o $RESULT check-refs.nix -A test11)