diff options
30 files changed, 636 insertions, 348 deletions
diff --git a/doc/manual/change-authors.yml b/doc/manual/change-authors.yml index 775149180..630af29ff 100644 --- a/doc/manual/change-authors.yml +++ b/doc/manual/change-authors.yml @@ -70,10 +70,17 @@ horrors: iFreilicht: github: iFreilicht +isabelroses: + forgejo: isabelroses + github: isabelroses + jade: forgejo: jade github: lf- +kjeremy: + github: kjeremy + kloenk: forgejo: kloenk github: kloenk diff --git a/doc/manual/rl-next/nix-flake-show-description.md b/doc/manual/rl-next/nix-flake-show-description.md new file mode 100644 index 000000000..e819a3a29 --- /dev/null +++ b/doc/manual/rl-next/nix-flake-show-description.md @@ -0,0 +1,37 @@ +--- +synopsis: "Lix will now show the package descriptions in when running `nix flake show`." +cls: [1540] +issues: [] +credits: [kjeremy, isabelroses] +category: Improvements +--- + +When running `nix flake show`, Lix will now show the package descriptions, if they exist. + +Before: + +```shell +$ nix flake show +path:/home/isabel/dev/lix-show?lastModified=1721736108&narHash=sha256-Zo8HP1ur7Q2b39hKUEG8EAh/opgq8xJ2jvwQ/htwO4Q%3D +└───packages + └───x86_64-linux + ├───aNoDescription: package 'simple' + ├───bOneLineDescription: package 'simple' + ├───cMultiLineDescription: package 'simple' + ├───dLongDescription: package 'simple' + └───eEmptyDescription: package 'simple' +``` + +After: + +```shell +$ nix flake show +path:/home/isabel/dev/lix-show?lastModified=1721736108&narHash=sha256-Zo8HP1ur7Q2b39hKUEG8EAh/opgq8xJ2jvwQ/htwO4Q%3D +└───packages + └───x86_64-linux + ├───aNoDescription: package 'simple' + ├───bOneLineDescription: package 'simple' - 'one line' + ├───cMultiLineDescription: package 'simple' - 'line one' + ├───dLongDescription: package 'simple' - 'abcdefghijklmnopqrstuvwxyz' + └───eEmptyDescription: package 'simple' +``` @@ -33,7 +33,7 @@ # This notice gets echoed as a dev shell hook, and can be turned off with # `touch .nocontribmsg` - sgr = ''[''; + sgr = builtins.fromJSON ''"\u001b["''; freezePage = "https://wiki.lix.systems/books/lix-contributors/page/freezes-and-recommended-contributions"; codebaseOverview = "https://wiki.lix.systems/books/lix-contributors/page/codebase-overview"; contribNotice = builtins.toFile "lix-contrib-notice" '' diff --git a/package.nix b/package.nix index 1b711585d..7ebe2721b 100644 --- a/package.nix +++ b/package.nix @@ -20,6 +20,7 @@ doxygen, editline-lix ? __forDefaults.editline-lix, editline, + expect, git, gtest, jq, @@ -87,13 +88,17 @@ let version = __forDefaults.versionJson.version + versionSuffix; - aws-sdk-cpp-nix = aws-sdk-cpp.override { - apis = [ - "s3" - "transfer" - ]; - customMemoryManagement = false; - }; + aws-sdk-cpp-nix = + if aws-sdk-cpp == null then + null + else + aws-sdk-cpp.override { + apis = [ + "s3" + "transfer" + ]; + customMemoryManagement = false; + }; # Reimplementation of Nixpkgs' Meson cross file, with some additions to make # it actually work. @@ -254,6 +259,8 @@ stdenv.mkDerivation (finalAttrs: { ++ lib.optional (hostPlatform.canExecute buildPlatform) aws-sdk-cpp-nix ++ lib.optionals (finalAttrs.dontBuild) maybePropagatedInputs; + nativeCheckInputs = [ expect ]; + checkInputs = [ gtest rapidcheck diff --git a/src/libexpr/flake/flakeref.cc b/src/libexpr/flake/flakeref.cc index a95df04ba..3be4ea550 100644 --- a/src/libexpr/flake/flakeref.cc +++ b/src/libexpr/flake/flakeref.cc @@ -169,14 +169,13 @@ std::pair<FlakeRef, std::string> parseFlakeRefWithFragment( if (subdir != "") { if (parsedURL.query.count("dir")) throw Error("flake URL '%s' has an inconsistent 'dir' parameter", url); - parsedURL.query.insert_or_assign("dir", subdir); } if (pathExists(flakeRoot + "/.git/shallow")) parsedURL.query.insert_or_assign("shallow", "1"); return std::make_pair( - FlakeRef(Input::fromURL(parsedURL, isFlake), getOr(parsedURL.query, "dir", "")), + FlakeRef(Input::fromURL(parsedURL, isFlake), subdir), fragment); } @@ -204,7 +203,13 @@ std::pair<FlakeRef, std::string> parseFlakeRefWithFragment( std::string fragment; std::swap(fragment, parsedURL.fragment); - auto input = Input::fromURL(parsedURL, isFlake); + // This has a special meaning for flakes and must not be passed to libfetchers. + // Of course this means that libfetchers cannot have fetchers + // expecting an argument `dir` 🫠 + ParsedURL urlForFetchers(parsedURL); + urlForFetchers.query.erase("dir"); + + auto input = Input::fromURL(urlForFetchers, isFlake); input.parent = baseDir; return std::make_pair( diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index 2bb4248be..40f2b6294 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -159,6 +159,37 @@ struct InputScheme std::optional<std::string> commitMsg) const; virtual std::pair<StorePath, Input> fetch(ref<Store> store, const Input & input) = 0; + +protected: + void emplaceURLQueryIntoAttrs( + const ParsedURL & parsedURL, + Attrs & attrs, + const StringSet & numericParams, + const StringSet & booleanParams) const + { + for (auto &[name, value] : parsedURL.query) { + if (name == "url") { + throw BadURL( + "URL '%s' must not override url via query param!", + parsedURL.to_string() + ); + } else if (numericParams.count(name) != 0) { + if (auto n = string2Int<uint64_t>(value)) { + attrs.insert_or_assign(name, *n); + } else { + throw BadURL( + "URL '%s' has non-numeric parameter '%s'", + parsedURL.to_string(), + name + ); + } + } else if (booleanParams.count(name) != 0) { + attrs.emplace(name, Explicit<bool> { value == "1" }); + } else { + attrs.emplace(name, value); + } + } + } }; void registerInputScheme(std::shared_ptr<InputScheme> && fetcher); diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 9fd8d7bbf..8e3165ff6 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -273,18 +273,15 @@ struct GitInputScheme : InputScheme Attrs attrs; attrs.emplace("type", "git"); - - for (auto & [name, value] : url.query) { - if (name == "rev" || name == "ref") - attrs.emplace(name, value); - else if (name == "shallow" || name == "submodules" || name == "allRefs") - attrs.emplace(name, Explicit<bool> { value == "1" }); - else - url2.query.emplace(name, value); - } - attrs.emplace("url", url2.to_string()); + emplaceURLQueryIntoAttrs( + url, + attrs, + {"lastModified", "revCount"}, + {"shallow", "submodules", "allRefs"} + ); + return inputFromAttrs(attrs); } diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index 60fefd1f3..b971781ae 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -1,3 +1,4 @@ +#include "attrs.hh" #include "filetransfer.hh" #include "cache.hh" #include "globals.hh" @@ -36,18 +37,11 @@ struct GitArchiveInputScheme : InputScheme auto path = tokenizeString<std::vector<std::string>>(url.path, "/"); - std::optional<Hash> rev; - std::optional<std::string> ref; - std::optional<std::string> host_url; + std::optional<std::string> refOrRev; auto size = path.size(); if (size == 3) { - if (std::regex_match(path[2], revRegex)) - rev = Hash::parseAny(path[2], htSHA1); - else if (std::regex_match(path[2], refRegex)) - ref = path[2]; - else - throw BadURL("in URL '%s', '%s' is not a commit hash or branch/tag name", url.url, path[2]); + refOrRev = path[2]; } else if (size > 3) { std::string rs; for (auto i = std::next(path.begin(), 2); i != path.end(); i++) { @@ -58,61 +52,91 @@ struct GitArchiveInputScheme : InputScheme } if (std::regex_match(rs, refRegex)) { - ref = rs; + refOrRev = rs; } else { throw BadURL("in URL '%s', '%s' is not a branch/tag name", url.url, rs); } } else if (size < 2) throw BadURL("URL '%s' is invalid", url.url); + Attrs attrs; + attrs.emplace("type", type()); + attrs.emplace("owner", path[0]); + attrs.emplace("repo", path[1]); + for (auto &[name, value] : url.query) { - if (name == "rev") { - if (rev) - throw BadURL("URL '%s' contains multiple commit hashes", url.url); - rev = Hash::parseAny(value, htSHA1); - } - else if (name == "ref") { - if (!std::regex_match(value, refRegex)) - throw BadURL("URL '%s' contains an invalid branch/tag name", url.url); - if (ref) - throw BadURL("URL '%s' contains multiple branch/tag names", url.url); - ref = value; - } - else if (name == "host") { - if (!std::regex_match(value, hostRegex)) - throw BadURL("URL '%s' contains an invalid instance host", url.url); - host_url = value; + if (name == "rev" || name == "ref") { + if (refOrRev) { + throw BadURL("URL '%s' already contains a ref or rev", url.url); + } else { + refOrRev = value; + } + } else if (name == "lastModified") { + if (auto n = string2Int<uint64_t>(value)) { + attrs.emplace(name, *n); + } else { + throw Error( + "Attribute 'lastModified' in URL '%s' must be an integer", + url.to_string() + ); + } + } else { + attrs.emplace(name, value); } - // FIXME: barf on unsupported attributes } - if (ref && rev) - throw BadURL("URL '%s' contains both a commit hash and a branch/tag name %s %s", url.url, *ref, rev->gitRev()); + if (refOrRev) attrs.emplace("refOrRev", *refOrRev); - Input input; - input.attrs.insert_or_assign("type", type()); - input.attrs.insert_or_assign("owner", path[0]); - input.attrs.insert_or_assign("repo", path[1]); - if (rev) input.attrs.insert_or_assign("rev", rev->gitRev()); - if (ref) input.attrs.insert_or_assign("ref", *ref); - if (host_url) input.attrs.insert_or_assign("host", *host_url); - - return input; + return inputFromAttrs(attrs); } std::optional<Input> inputFromAttrs(const Attrs & attrs) const override { - if (maybeGetStrAttr(attrs, "type") != type()) return {}; + // Attributes can contain refOrRev and it needs to be figured out + // which one it is (see inputFromURL for when that may happen). + // The correct one (ref or rev) will be written into finalAttrs and + // it needs to be mutable for that. + Attrs finalAttrs(attrs); + auto type_ = maybeGetStrAttr(finalAttrs, "type"); + if (type_ != type()) return {}; + + auto owner = getStrAttr(finalAttrs, "owner"); + auto repo = getStrAttr(finalAttrs, "repo"); + + auto url = fmt("%s:%s/%s", *type_, owner, repo); + if (auto host = maybeGetStrAttr(finalAttrs, "host")) { + if (!std::regex_match(*host, hostRegex)) { + throw BadURL("URL '%s' contains an invalid instance host", url); + } + } - for (auto & [name, value] : attrs) - if (name != "type" && name != "owner" && name != "repo" && name != "ref" && name != "rev" && name != "narHash" && name != "lastModified" && name != "host") - throw Error("unsupported input attribute '%s'", name); + if (auto refOrRev = maybeGetStrAttr(finalAttrs, "refOrRev")) { + finalAttrs.erase("refOrRev"); + if (std::regex_match(*refOrRev, revRegex)) { + finalAttrs.emplace("rev", *refOrRev); + } else if (std::regex_match(*refOrRev, refRegex)) { + finalAttrs.emplace("ref", *refOrRev); + } else { + throw Error( + "in URL '%s', '%s' is not a commit hash or a branch/tag name", + url, + *refOrRev + ); + } + } else if (auto ref = maybeGetStrAttr(finalAttrs, "ref")) { + if (!std::regex_match(*ref, refRegex)) { + throw BadURL("URL '%s' contains an invalid branch/tag name", url); + } + } - getStrAttr(attrs, "owner"); - getStrAttr(attrs, "repo"); + for (auto & [name, value] : finalAttrs) { + if (name != "type" && name != "owner" && name != "repo" && name != "ref" && name != "rev" && name != "narHash" && name != "lastModified" && name != "host") { + throw Error("unsupported input attribute '%s'", name); + } + } Input input; - input.attrs = attrs; + input.attrs = finalAttrs; return input; } diff --git a/src/libfetchers/indirect.cc b/src/libfetchers/indirect.cc index c73505b31..8c0176e84 100644 --- a/src/libfetchers/indirect.cc +++ b/src/libfetchers/indirect.cc @@ -17,6 +17,8 @@ struct IndirectInputScheme : InputScheme std::optional<Hash> rev; std::optional<std::string> ref; + Attrs attrs; + if (path.size() == 1) { } else if (path.size() == 2) { if (std::regex_match(path[1], revRegex)) @@ -26,29 +28,21 @@ struct IndirectInputScheme : InputScheme else throw BadURL("in flake URL '%s', '%s' is not a commit hash or branch/tag name", url.url, path[1]); } else if (path.size() == 3) { - if (!std::regex_match(path[1], refRegex)) - throw BadURL("in flake URL '%s', '%s' is not a branch/tag name", url.url, path[1]); ref = path[1]; - if (!std::regex_match(path[2], revRegex)) - throw BadURL("in flake URL '%s', '%s' is not a commit hash", url.url, path[2]); rev = Hash::parseAny(path[2], htSHA1); } else throw BadURL("GitHub URL '%s' is invalid", url.url); std::string id = path[0]; - if (!std::regex_match(id, flakeRegex)) - throw BadURL("'%s' is not a valid flake ID", id); - // FIXME: forbid query params? + attrs.emplace("type", "indirect"); + attrs.emplace("id", id); + if (rev) attrs.emplace("rev", rev->gitRev()); + if (ref) attrs.emplace("ref", *ref); - Input input; - input.direct = false; - input.attrs.insert_or_assign("type", "indirect"); - input.attrs.insert_or_assign("id", id); - if (rev) input.attrs.insert_or_assign("rev", rev->gitRev()); - if (ref) input.attrs.insert_or_assign("ref", *ref); + emplaceURLQueryIntoAttrs(url, attrs, {}, {}); - return input; + return inputFromAttrs(attrs); } std::optional<Input> inputFromAttrs(const Attrs & attrs) const override @@ -63,6 +57,18 @@ struct IndirectInputScheme : InputScheme if (!std::regex_match(id, flakeRegex)) throw BadURL("'%s' is not a valid flake ID", id); + // TODO come up with a nicer error message for those two. + if (auto rev = maybeGetStrAttr(attrs, "rev")) { + if (!std::regex_match(*rev, revRegex)) { + throw BadURL("in flake '%s', '%s' is not a commit hash", id, *rev); + } + } + if (auto ref = maybeGetStrAttr(attrs, "ref")) { + if (!std::regex_match(*ref, refRegex)) { + throw BadURL("in flake '%s', '%s' is not a valid branch/tag name", id, *ref); + } + } + Input input; input.direct = false; input.attrs = attrs; diff --git a/src/libfetchers/mercurial.cc b/src/libfetchers/mercurial.cc index 23cf7b51d..b4150e9df 100644 --- a/src/libfetchers/mercurial.cc +++ b/src/libfetchers/mercurial.cc @@ -56,12 +56,7 @@ struct MercurialInputScheme : InputScheme Attrs attrs; attrs.emplace("type", "hg"); - for (auto &[name, value] : url.query) { - if (name == "rev" || name == "ref") - attrs.emplace(name, value); - else - url2.query.emplace(name, value); - } + emplaceURLQueryIntoAttrs(url, attrs, {"revCount"}, {}); attrs.emplace("url", url2.to_string()); diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index 6ce35aeb2..b11665805 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -201,29 +201,17 @@ struct CurlInputScheme : InputScheme if (!isValidURL(_url, requireTree)) return std::nullopt; - Input input; - auto url = _url; - url.scheme = parseUrlScheme(url.scheme).transport; - - auto narHash = url.query.find("narHash"); - if (narHash != url.query.end()) - input.attrs.insert_or_assign("narHash", narHash->second); + Attrs attrs; + attrs.emplace("type", inputType()); - if (auto i = get(url.query, "rev")) - input.attrs.insert_or_assign("rev", *i); - - if (auto i = get(url.query, "revCount")) - if (auto n = string2Int<uint64_t>(*i)) - input.attrs.insert_or_assign("revCount", *n); + url.scheme = parseUrlScheme(url.scheme).transport; - url.query.erase("rev"); - url.query.erase("revCount"); + emplaceURLQueryIntoAttrs(url, attrs, {"revCount"}, {}); - input.attrs.insert_or_assign("type", inputType()); - input.attrs.insert_or_assign("url", url.to_string()); - return input; + attrs.emplace("url", url.to_string()); + return inputFromAttrs(attrs); } std::optional<Input> inputFromAttrs(const Attrs & attrs) const override @@ -235,7 +223,7 @@ struct CurlInputScheme : InputScheme std::set<std::string> allowedNames = {"type", "url", "narHash", "name", "unpack", "rev", "revCount", "lastModified"}; for (auto & [name, value] : attrs) if (!allowedNames.count(name)) - throw Error("unsupported %s input attribute '%s'", *type, name); + throw Error("unsupported %s input attribute '%s'. If you wanted to fetch a tarball with a query parameter, please use '{ type = \"tarball\"; url = \"...\"; }'", *type, name); Input input; input.attrs = attrs; diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 38c54e854..ab7b2b88c 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -130,16 +130,16 @@ void DerivationGoal::killChild() } -void DerivationGoal::timedOut(Error && ex) +Goal::Finished DerivationGoal::timedOut(Error && ex) { killChild(); - done(BuildResult::TimedOut, {}, std::move(ex)); + return done(BuildResult::TimedOut, {}, std::move(ex)); } -void DerivationGoal::work() +Goal::WorkResult DerivationGoal::work() { - (this->*state)(); + return (this->*state)(); } void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs) @@ -163,7 +163,7 @@ void DerivationGoal::addWantedOutputs(const OutputsSpec & outputs) } -void DerivationGoal::getDerivation() +Goal::WorkResult DerivationGoal::getDerivation() { trace("init"); @@ -171,23 +171,22 @@ void DerivationGoal::getDerivation() exists. If it doesn't, it may be created through a substitute. */ if (buildMode == bmNormal && worker.evalStore.isValidPath(drvPath)) { - loadDerivation(); - return; + return loadDerivation(); } addWaitee(worker.makePathSubstitutionGoal(drvPath)); state = &DerivationGoal::loadDerivation; + return StillAlive{}; } -void DerivationGoal::loadDerivation() +Goal::WorkResult DerivationGoal::loadDerivation() { trace("loading derivation"); if (nrFailed != 0) { - done(BuildResult::MiscFailure, {}, Error("cannot build missing derivation '%s'", worker.store.printStorePath(drvPath))); - return; + return done(BuildResult::MiscFailure, {}, Error("cannot build missing derivation '%s'", worker.store.printStorePath(drvPath))); } /* `drvPath' should already be a root, but let's be on the safe @@ -209,11 +208,11 @@ void DerivationGoal::loadDerivation() } assert(drv); - haveDerivation(); + return haveDerivation(); } -void DerivationGoal::haveDerivation() +Goal::WorkResult DerivationGoal::haveDerivation() { trace("have derivation"); @@ -241,8 +240,7 @@ void DerivationGoal::haveDerivation() }); } - gaveUpOnSubstitution(); - return; + return gaveUpOnSubstitution(); } for (auto & i : drv->outputsAndOptPaths(worker.store)) @@ -264,8 +262,7 @@ void DerivationGoal::haveDerivation() /* If they are all valid, then we're done. */ if (allValid && buildMode == bmNormal) { - done(BuildResult::AlreadyValid, std::move(validOutputs)); - return; + return done(BuildResult::AlreadyValid, std::move(validOutputs)); } /* We are first going to try to create the invalid output paths @@ -290,24 +287,24 @@ void DerivationGoal::haveDerivation() } } - if (waitees.empty()) /* to prevent hang (no wake-up event) */ - outputsSubstitutionTried(); - else + if (waitees.empty()) { /* to prevent hang (no wake-up event) */ + return outputsSubstitutionTried(); + } else { state = &DerivationGoal::outputsSubstitutionTried; + return StillAlive{}; + } } - -void DerivationGoal::outputsSubstitutionTried() +Goal::WorkResult DerivationGoal::outputsSubstitutionTried() { trace("all outputs substituted (maybe)"); assert(drv->type().isPure()); if (nrFailed > 0 && nrFailed > nrNoSubstituters + nrIncompleteClosure && !settings.tryFallback) { - done(BuildResult::TransientFailure, {}, + return done(BuildResult::TransientFailure, {}, Error("some substitutes for the outputs of derivation '%s' failed (usually happens due to networking issues); try '--fallback' to build derivation from source ", worker.store.printStorePath(drvPath))); - return; } /* If the substitutes form an incomplete closure, then we should @@ -341,32 +338,29 @@ void DerivationGoal::outputsSubstitutionTried() if (needRestart == NeedRestartForMoreOutputs::OutputsAddedDoNeed) { needRestart = NeedRestartForMoreOutputs::OutputsUnmodifedDontNeed; - haveDerivation(); - return; + return haveDerivation(); } auto [allValid, validOutputs] = checkPathValidity(); if (buildMode == bmNormal && allValid) { - done(BuildResult::Substituted, std::move(validOutputs)); - return; + return done(BuildResult::Substituted, std::move(validOutputs)); } if (buildMode == bmRepair && allValid) { - repairClosure(); - return; + return repairClosure(); } if (buildMode == bmCheck && !allValid) throw Error("some outputs of '%s' are not valid, so checking is not possible", worker.store.printStorePath(drvPath)); /* Nothing to wait for; tail call */ - gaveUpOnSubstitution(); + return gaveUpOnSubstitution(); } /* At least one of the output paths could not be produced using a substitute. So we have to build instead. */ -void DerivationGoal::gaveUpOnSubstitution() +Goal::WorkResult DerivationGoal::gaveUpOnSubstitution() { /* At this point we are building all outputs, so if more are wanted there is no need to restart. */ @@ -427,14 +421,16 @@ void DerivationGoal::gaveUpOnSubstitution() addWaitee(worker.makePathSubstitutionGoal(i)); } - if (waitees.empty()) /* to prevent hang (no wake-up event) */ - inputsRealised(); - else + if (waitees.empty()) {/* to prevent hang (no wake-up event) */ + return inputsRealised(); + } else { state = &DerivationGoal::inputsRealised; + return StillAlive{}; + } } -void DerivationGoal::repairClosure() +Goal::WorkResult DerivationGoal::repairClosure() { assert(drv->type().isPure()); @@ -488,41 +484,39 @@ void DerivationGoal::repairClosure() } if (waitees.empty()) { - done(BuildResult::AlreadyValid, assertPathValidity()); - return; + return done(BuildResult::AlreadyValid, assertPathValidity()); } state = &DerivationGoal::closureRepaired; + return StillAlive{}; } -void DerivationGoal::closureRepaired() +Goal::WorkResult DerivationGoal::closureRepaired() { trace("closure repaired"); if (nrFailed > 0) throw Error("some paths in the output closure of derivation '%s' could not be repaired", worker.store.printStorePath(drvPath)); - done(BuildResult::AlreadyValid, assertPathValidity()); + return done(BuildResult::AlreadyValid, assertPathValidity()); } -void DerivationGoal::inputsRealised() +Goal::WorkResult DerivationGoal::inputsRealised() { trace("all inputs realised"); if (nrFailed != 0) { if (!useDerivation) throw Error("some dependencies of '%s' are missing", worker.store.printStorePath(drvPath)); - done(BuildResult::DependencyFailed, {}, Error( + return done(BuildResult::DependencyFailed, {}, Error( "%s dependencies of derivation '%s' failed to build", nrFailed, worker.store.printStorePath(drvPath))); - return; } if (retrySubstitution == RetrySubstitution::YesNeed) { retrySubstitution = RetrySubstitution::AlreadyRetried; - haveDerivation(); - return; + return haveDerivation(); } /* Gather information necessary for computing the closure and/or @@ -589,7 +583,7 @@ void DerivationGoal::inputsRealised() addWaitee(resolvedDrvGoal); state = &DerivationGoal::resolvedFinished; - return; + return StillAlive{}; } std::function<void(const StorePath &, const DerivedPathMap<StringSet>::ChildNode &)> accumInputPaths; @@ -655,9 +649,10 @@ void DerivationGoal::inputsRealised() build hook. */ state = &DerivationGoal::tryToBuild; worker.wakeUp(shared_from_this()); + return StillAlive{}; } -void DerivationGoal::started() +Goal::WorkResult DerivationGoal::started() { auto msg = fmt( buildMode == bmRepair ? "repairing outputs of '%s'" : @@ -668,9 +663,10 @@ void DerivationGoal::started() act = std::make_unique<Activity>(*logger, lvlInfo, actBuild, msg, Logger::Fields{worker.store.printStorePath(drvPath), hook ? machineName : "", 1, 1}); mcRunningBuilds = std::make_unique<MaintainCount<uint64_t>>(worker.runningBuilds); + return StillAlive{}; } -void DerivationGoal::tryToBuild() +Goal::WorkResult DerivationGoal::tryToBuild() { trace("trying to build"); @@ -706,7 +702,7 @@ void DerivationGoal::tryToBuild() actLock = std::make_unique<Activity>(*logger, lvlWarn, actBuildWaiting, fmt("waiting for lock on %s", Magenta(showPaths(lockFiles)))); worker.waitForAWhile(shared_from_this()); - return; + return StillAlive{}; } actLock.reset(); @@ -723,8 +719,7 @@ void DerivationGoal::tryToBuild() if (buildMode != bmCheck && allValid) { debug("skipping build of derivation '%s', someone beat us to it", worker.store.printStorePath(drvPath)); outputLocks.setDeletion(true); - done(BuildResult::AlreadyValid, std::move(validOutputs)); - return; + return done(BuildResult::AlreadyValid, std::move(validOutputs)); } /* If any of the outputs already exist but are not valid, delete @@ -751,8 +746,7 @@ void DerivationGoal::tryToBuild() actLock.reset(); buildResult.startTime = time(0); // inexact state = &DerivationGoal::buildDone; - started(); - return; + return started(); case rpPostpone: /* Not now; wait until at least one child finishes or the wake-up timeout expires. */ @@ -761,7 +755,7 @@ void DerivationGoal::tryToBuild() fmt("waiting for a machine to build '%s'", Magenta(worker.store.printStorePath(drvPath)))); worker.waitForAWhile(shared_from_this()); outputLocks.unlock(); - return; + return StillAlive{}; case rpDecline: /* We should do it ourselves. */ break; @@ -772,9 +766,10 @@ void DerivationGoal::tryToBuild() state = &DerivationGoal::tryLocalBuild; worker.wakeUp(shared_from_this()); + return StillAlive{}; } -void DerivationGoal::tryLocalBuild() { +Goal::WorkResult DerivationGoal::tryLocalBuild() { throw Error( "unable to build with a primary store that isn't a local store; " "either pass a different '--store' or enable remote builds." @@ -932,7 +927,7 @@ void runPostBuildHook( proc.getStdout()->drainInto(sink); } -void DerivationGoal::buildDone() +Goal::WorkResult DerivationGoal::buildDone() { trace("build done"); @@ -1027,8 +1022,7 @@ void DerivationGoal::buildDone() outputLocks.setDeletion(true); outputLocks.unlock(); - done(BuildResult::Built, std::move(builtOutputs)); - + return done(BuildResult::Built, std::move(builtOutputs)); } catch (BuildError & e) { outputLocks.unlock(); @@ -1049,12 +1043,11 @@ void DerivationGoal::buildDone() BuildResult::PermanentFailure; } - done(st, {}, std::move(e)); - return; + return done(st, {}, std::move(e)); } } -void DerivationGoal::resolvedFinished() +Goal::WorkResult DerivationGoal::resolvedFinished() { trace("resolved derivation finished"); @@ -1122,26 +1115,26 @@ void DerivationGoal::resolvedFinished() if (status == BuildResult::AlreadyValid) status = BuildResult::ResolvesToAlreadyValid; - done(status, std::move(builtOutputs)); + return done(status, std::move(builtOutputs)); } HookReply DerivationGoal::tryBuildHook() { - if (!worker.tryBuildHook || !useDerivation) return rpDecline; + if (!worker.hook.available || !useDerivation) return rpDecline; - if (!worker.hook) - worker.hook = std::make_unique<HookInstance>(); + if (!worker.hook.instance) + worker.hook.instance = std::make_unique<HookInstance>(); try { /* Send the request to the hook. */ - worker.hook->sink + worker.hook.instance->sink << "try" << (worker.getNrLocalBuilds() < settings.maxBuildJobs ? 1 : 0) << drv->platform << worker.store.printStorePath(drvPath) << parsedDrv->getRequiredSystemFeatures(); - worker.hook->sink.flush(); + worker.hook.instance->sink.flush(); /* Read the first line of input, which should be a word indicating whether the hook wishes to perform the build. */ @@ -1149,13 +1142,13 @@ HookReply DerivationGoal::tryBuildHook() while (true) { auto s = [&]() { try { - return readLine(worker.hook->fromHook.readSide.get()); + return readLine(worker.hook.instance->fromHook.readSide.get()); } catch (Error & e) { e.addTrace({}, "while reading the response from the build hook"); throw; } }(); - if (handleJSONLogMessage(s, worker.act, worker.hook->activities, true)) + if (handleJSONLogMessage(s, worker.act, worker.hook.instance->activities, true)) ; else if (s.substr(0, 2) == "# ") { reply = s.substr(2); @@ -1172,8 +1165,8 @@ HookReply DerivationGoal::tryBuildHook() if (reply == "decline") return rpDecline; else if (reply == "decline-permanently") { - worker.tryBuildHook = false; - worker.hook = 0; + worker.hook.available = false; + worker.hook.instance.reset(); return rpDecline; } else if (reply == "postpone") @@ -1185,14 +1178,14 @@ HookReply DerivationGoal::tryBuildHook() if (e.errNo == EPIPE) { printError( "build hook died unexpectedly: %s", - chomp(drainFD(worker.hook->fromHook.readSide.get()))); - worker.hook = 0; + chomp(drainFD(worker.hook.instance->fromHook.readSide.get()))); + worker.hook.instance.reset(); return rpDecline; } else throw; } - hook = std::move(worker.hook); + hook = std::move(worker.hook.instance); try { machineName = readLine(hook->fromHook.readSide.get()); @@ -1293,7 +1286,7 @@ bool DerivationGoal::isReadDesc(int fd) return fd == hook->builderOut.readSide.get(); } -void DerivationGoal::handleChildOutput(int fd, std::string_view data) +Goal::WorkResult DerivationGoal::handleChildOutput(int fd, std::string_view data) { // local & `ssh://`-builds are dealt with here. auto isWrittenToLog = isReadDesc(fd); @@ -1302,11 +1295,10 @@ void DerivationGoal::handleChildOutput(int fd, std::string_view data) logSize += data.size(); if (settings.maxLogSize && logSize > settings.maxLogSize) { killChild(); - done( + return done( BuildResult::LogLimitExceeded, {}, Error("%s killed after writing more than %d bytes of log output", getName(), settings.maxLogSize)); - return; } for (auto c : data) @@ -1357,6 +1349,8 @@ void DerivationGoal::handleChildOutput(int fd, std::string_view data) } else currentHookLine += c; } + + return StillAlive{}; } @@ -1505,7 +1499,7 @@ SingleDrvOutputs DerivationGoal::assertPathValidity() } -void DerivationGoal::done( +Goal::Finished DerivationGoal::done( BuildResult::Status status, SingleDrvOutputs builtOutputs, std::optional<Error> ex) @@ -1540,7 +1534,10 @@ void DerivationGoal::done( fs << worker.store.printStorePath(drvPath) << "\t" << buildResult.toString() << std::endl; } - amDone(buildResult.success() ? ecSuccess : ecFailed, std::move(ex)); + return Finished{ + .result = buildResult.success() ? ecSuccess : ecFailed, + .ex = ex ? std::make_unique<Error>(std::move(*ex)) : nullptr, + }; } diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 28aa6afbe..268b717dd 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -188,7 +188,7 @@ struct DerivationGoal : public Goal */ std::optional<DerivationType> derivationType; - typedef void (DerivationGoal::*GoalState)(); + typedef WorkResult (DerivationGoal::*GoalState)(); GoalState state; BuildMode buildMode; @@ -217,11 +217,11 @@ struct DerivationGoal : public Goal BuildMode buildMode = bmNormal); virtual ~DerivationGoal() noexcept(false); - void timedOut(Error && ex) override; + Finished timedOut(Error && ex) override; std::string key() override; - void work() override; + WorkResult work() override; /** * Add wanted outputs to an already existing derivation goal. @@ -231,18 +231,18 @@ struct DerivationGoal : public Goal /** * The states. */ - void getDerivation(); - void loadDerivation(); - void haveDerivation(); - void outputsSubstitutionTried(); - void gaveUpOnSubstitution(); - void closureRepaired(); - void inputsRealised(); - void tryToBuild(); - virtual void tryLocalBuild(); - void buildDone(); + WorkResult getDerivation(); + WorkResult loadDerivation(); + WorkResult haveDerivation(); + WorkResult outputsSubstitutionTried(); + WorkResult gaveUpOnSubstitution(); + WorkResult closureRepaired(); + WorkResult inputsRealised(); + WorkResult tryToBuild(); + virtual WorkResult tryLocalBuild(); + WorkResult buildDone(); - void resolvedFinished(); + WorkResult resolvedFinished(); /** * Is the build hook willing to perform the build? @@ -292,7 +292,7 @@ struct DerivationGoal : public Goal /** * Callback used by the worker to write to the log. */ - void handleChildOutput(int fd, std::string_view data) override; + WorkResult handleChildOutput(int fd, std::string_view data) override; void handleEOF(int fd) override; void flushLine(); @@ -323,11 +323,11 @@ struct DerivationGoal : public Goal */ virtual void killChild(); - void repairClosure(); + WorkResult repairClosure(); - void started(); + WorkResult started(); - void done( + Finished done( BuildResult::Status status, SingleDrvOutputs builtOutputs = {}, std::optional<Error> ex = {}); diff --git a/src/libstore/build/drv-output-substitution-goal.cc b/src/libstore/build/drv-output-substitution-goal.cc index 94c9206a3..62e86e1cc 100644 --- a/src/libstore/build/drv-output-substitution-goal.cc +++ b/src/libstore/build/drv-output-substitution-goal.cc @@ -20,21 +20,20 @@ DrvOutputSubstitutionGoal::DrvOutputSubstitutionGoal( } -void DrvOutputSubstitutionGoal::init() +Goal::WorkResult DrvOutputSubstitutionGoal::init() { trace("init"); /* If the derivation already exists, we’re done */ if (worker.store.queryRealisation(id)) { - amDone(ecSuccess); - return; + return Finished{ecSuccess}; } subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list<ref<Store>>(); - tryNext(); + return tryNext(); } -void DrvOutputSubstitutionGoal::tryNext() +Goal::WorkResult DrvOutputSubstitutionGoal::tryNext() { trace("trying next substituter"); @@ -43,7 +42,7 @@ void DrvOutputSubstitutionGoal::tryNext() prevents infinite waiting. */ if (worker.runningSubstitutions >= std::max(1U, settings.maxSubstitutionJobs.get())) { worker.waitForBuildSlot(shared_from_this()); - return; + return StillAlive{}; } maintainRunningSubstitutions = @@ -54,16 +53,14 @@ void DrvOutputSubstitutionGoal::tryNext() with it. */ debug("derivation output '%s' is required, but there is no substituter that can provide it", id.to_string()); - /* Hack: don't indicate failure if there were no substituters. - In that case the calling derivation should just do a - build. */ - amDone(substituterFailed ? ecFailed : ecNoSubstituters); - if (substituterFailed) { worker.failedSubstitutions++; } - return; + /* Hack: don't indicate failure if there were no substituters. + In that case the calling derivation should just do a + build. */ + return Finished{substituterFailed ? ecFailed : ecNoSubstituters}; } sub = subs.front(); @@ -85,9 +82,10 @@ void DrvOutputSubstitutionGoal::tryNext() worker.childStarted(shared_from_this(), {downloadState->outPipe.readSide.get()}, true, false); state = &DrvOutputSubstitutionGoal::realisationFetched; + return StillAlive{}; } -void DrvOutputSubstitutionGoal::realisationFetched() +Goal::WorkResult DrvOutputSubstitutionGoal::realisationFetched() { worker.childTerminated(this); maintainRunningSubstitutions.reset(); @@ -116,8 +114,7 @@ void DrvOutputSubstitutionGoal::realisationFetched() worker.store.printStorePath(localOutputInfo->outPath), worker.store.printStorePath(depPath) ); - tryNext(); - return; + return tryNext(); } addWaitee(worker.makeDrvOutputSubstitutionGoal(depId)); } @@ -125,29 +122,34 @@ void DrvOutputSubstitutionGoal::realisationFetched() addWaitee(worker.makePathSubstitutionGoal(outputInfo->outPath)); - if (waitees.empty()) outPathValid(); - else state = &DrvOutputSubstitutionGoal::outPathValid; + if (waitees.empty()) { + return outPathValid(); + } else { + state = &DrvOutputSubstitutionGoal::outPathValid; + return StillAlive{}; + } } -void DrvOutputSubstitutionGoal::outPathValid() +Goal::WorkResult DrvOutputSubstitutionGoal::outPathValid() { assert(outputInfo); trace("output path substituted"); if (nrFailed > 0) { debug("The output path of the derivation output '%s' could not be substituted", id.to_string()); - amDone(nrNoSubstituters > 0 || nrIncompleteClosure > 0 ? ecIncompleteClosure : ecFailed); - return; + return Finished{ + nrNoSubstituters > 0 || nrIncompleteClosure > 0 ? ecIncompleteClosure : ecFailed + }; } worker.store.registerDrvOutput(*outputInfo); - finished(); + return finished(); } -void DrvOutputSubstitutionGoal::finished() +Goal::WorkResult DrvOutputSubstitutionGoal::finished() { trace("finished"); - amDone(ecSuccess); + return Finished{ecSuccess}; } std::string DrvOutputSubstitutionGoal::key() @@ -157,9 +159,9 @@ std::string DrvOutputSubstitutionGoal::key() return "a$" + std::string(id.to_string()); } -void DrvOutputSubstitutionGoal::work() +Goal::WorkResult DrvOutputSubstitutionGoal::work() { - (this->*state)(); + return (this->*state)(); } diff --git a/src/libstore/build/drv-output-substitution-goal.hh b/src/libstore/build/drv-output-substitution-goal.hh index 598b119dc..a28347f15 100644 --- a/src/libstore/build/drv-output-substitution-goal.hh +++ b/src/libstore/build/drv-output-substitution-goal.hh @@ -58,20 +58,20 @@ class DrvOutputSubstitutionGoal : public Goal { public: DrvOutputSubstitutionGoal(const DrvOutput& id, Worker & worker, RepairFlag repair = NoRepair, std::optional<ContentAddress> ca = std::nullopt); - typedef void (DrvOutputSubstitutionGoal::*GoalState)(); + typedef WorkResult (DrvOutputSubstitutionGoal::*GoalState)(); GoalState state; - void init(); - void tryNext(); - void realisationFetched(); - void outPathValid(); - void finished(); + WorkResult init(); + WorkResult tryNext(); + WorkResult realisationFetched(); + WorkResult outPathValid(); + WorkResult finished(); - void timedOut(Error && ex) override { abort(); }; + Finished timedOut(Error && ex) override { abort(); }; std::string key() override; - void work() override; + WorkResult work() override; JobCategory jobCategory() const override { return JobCategory::Substitution; diff --git a/src/libstore/build/goal.cc b/src/libstore/build/goal.cc index 4db6af6e6..40a3bae8d 100644 --- a/src/libstore/build/goal.cc +++ b/src/libstore/build/goal.cc @@ -45,30 +45,6 @@ void Goal::waiteeDone(GoalPtr waitee, ExitCode result) } -void Goal::amDone(ExitCode result, std::optional<Error> ex) -{ - trace("done"); - assert(!exitCode.has_value()); - exitCode = result; - - if (ex) { - if (!waiters.empty()) - logError(ex->info()); - else - this->ex = std::make_unique<Error>(std::move(*ex)); - } - - for (auto & i : waiters) { - GoalPtr goal = i.lock(); - if (goal) goal->waiteeDone(shared_from_this(), result); - } - waiters.clear(); - worker.removeGoal(shared_from_this()); - - cleanup(); -} - - void Goal::trace(std::string_view s) { debug("%1%: %2%", name, s); diff --git a/src/libstore/build/goal.hh b/src/libstore/build/goal.hh index 575621037..adb3ab94b 100644 --- a/src/libstore/build/goal.hh +++ b/src/libstore/build/goal.hh @@ -105,6 +105,18 @@ struct Goal : public std::enable_shared_from_this<Goal> public: + struct [[nodiscard]] StillAlive {}; + struct [[nodiscard]] Finished { + ExitCode result; + std::unique_ptr<Error> ex; + }; + + struct [[nodiscard]] WorkResult : std::variant<StillAlive, Finished> + { + WorkResult() = delete; + using variant::variant; + }; + /** * Exception containing an error message, if any. */ @@ -119,13 +131,13 @@ public: trace("goal destroyed"); } - virtual void work() = 0; + virtual WorkResult work() = 0; void addWaitee(GoalPtr waitee); virtual void waiteeDone(GoalPtr waitee, ExitCode result); - virtual void handleChildOutput(int fd, std::string_view data) + virtual WorkResult handleChildOutput(int fd, std::string_view data) { abort(); } @@ -146,12 +158,10 @@ public: * get rid of any running child processes that are being monitored * by the worker (important!), etc. */ - virtual void timedOut(Error && ex) = 0; + virtual Finished timedOut(Error && ex) = 0; virtual std::string key() = 0; - void amDone(ExitCode result, std::optional<Error> ex = {}); - virtual void cleanup() { } /** diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 2f1f338c1..fb5ccc6f1 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -161,7 +161,7 @@ void LocalDerivationGoal::killSandbox(bool getStats) } -void LocalDerivationGoal::tryLocalBuild() +Goal::WorkResult LocalDerivationGoal::tryLocalBuild() { #if __APPLE__ additionalSandboxProfile = parsedDrv->getStringAttr("__sandboxProfile").value_or(""); @@ -172,7 +172,7 @@ void LocalDerivationGoal::tryLocalBuild() state = &DerivationGoal::tryToBuild; worker.waitForBuildSlot(shared_from_this()); outputLocks.unlock(); - return; + return StillAlive{}; } assert(derivationType); @@ -215,7 +215,7 @@ void LocalDerivationGoal::tryLocalBuild() actLock = std::make_unique<Activity>(*logger, lvlWarn, actBuildWaiting, fmt("waiting for a free build user ID for '%s'", Magenta(worker.store.printStorePath(drvPath)))); worker.waitForAWhile(shared_from_this()); - return; + return StillAlive{}; } } @@ -250,15 +250,14 @@ void LocalDerivationGoal::tryLocalBuild() outputLocks.unlock(); buildUser.reset(); worker.permanentFailure = true; - done(BuildResult::InputRejected, {}, std::move(e)); - return; + return done(BuildResult::InputRejected, {}, std::move(e)); } /* This state will be reached when we get EOF on the child's log pipe. */ state = &DerivationGoal::buildDone; - started(); + return started(); } diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh index 4ac18b974..237417b42 100644 --- a/src/libstore/build/local-derivation-goal.hh +++ b/src/libstore/build/local-derivation-goal.hh @@ -211,7 +211,7 @@ struct LocalDerivationGoal : public DerivationGoal /** * The additional states. */ - void tryLocalBuild() override; + WorkResult tryLocalBuild() override; /** * Start building a derivation. diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index 027a7e161..fb2949fd0 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -25,7 +25,7 @@ PathSubstitutionGoal::~PathSubstitutionGoal() } -void PathSubstitutionGoal::done( +Goal::Finished PathSubstitutionGoal::done( ExitCode result, BuildResult::Status status, std::optional<std::string> errorMsg) @@ -35,17 +35,17 @@ void PathSubstitutionGoal::done( debug(*errorMsg); buildResult.errorMsg = *errorMsg; } - amDone(result); + return Finished{result}; } -void PathSubstitutionGoal::work() +Goal::WorkResult PathSubstitutionGoal::work() { - (this->*state)(); + return (this->*state)(); } -void PathSubstitutionGoal::init() +Goal::WorkResult PathSubstitutionGoal::init() { trace("init"); @@ -53,8 +53,7 @@ void PathSubstitutionGoal::init() /* If the path already exists we're done. */ if (!repair && worker.store.isValidPath(storePath)) { - done(ecSuccess, BuildResult::AlreadyValid); - return; + return done(ecSuccess, BuildResult::AlreadyValid); } if (settings.readOnlyMode) @@ -62,11 +61,11 @@ void PathSubstitutionGoal::init() subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list<ref<Store>>(); - tryNext(); + return tryNext(); } -void PathSubstitutionGoal::tryNext() +Goal::WorkResult PathSubstitutionGoal::tryNext() { trace("trying next substituter"); @@ -75,20 +74,17 @@ void PathSubstitutionGoal::tryNext() if (subs.size() == 0) { /* None left. Terminate this goal and let someone else deal with it. */ + if (substituterFailed) { + worker.failedSubstitutions++; + } /* Hack: don't indicate failure if there were no substituters. In that case the calling derivation should just do a build. */ - done( + return done( substituterFailed ? ecFailed : ecNoSubstituters, BuildResult::NoSubstituters, fmt("path '%s' is required, but there is no substituter that can build it", worker.store.printStorePath(storePath))); - - if (substituterFailed) { - worker.failedSubstitutions++; - } - - return; } sub = subs.front(); @@ -101,27 +97,23 @@ void PathSubstitutionGoal::tryNext() if (sub->storeDir == worker.store.storeDir) assert(subPath == storePath); } else if (sub->storeDir != worker.store.storeDir) { - tryNext(); - return; + return tryNext(); } try { // FIXME: make async info = sub->queryPathInfo(subPath ? *subPath : storePath); } catch (InvalidPath &) { - tryNext(); - return; + return tryNext(); } catch (SubstituterDisabled &) { if (settings.tryFallback) { - tryNext(); - return; + return tryNext(); } throw; } catch (Error & e) { if (settings.tryFallback) { logError(e.info()); - tryNext(); - return; + return tryNext(); } throw; } @@ -134,8 +126,7 @@ void PathSubstitutionGoal::tryNext() } else { printError("asked '%s' for '%s' but got '%s'", sub->getUri(), worker.store.printStorePath(storePath), sub->printStorePath(info->path)); - tryNext(); - return; + return tryNext(); } } @@ -156,8 +147,7 @@ void PathSubstitutionGoal::tryNext() { warn("ignoring substitute for '%s' from '%s', as it's not signed by any of the keys in 'trusted-public-keys'", worker.store.printStorePath(storePath), sub->getUri()); - tryNext(); - return; + return tryNext(); } /* To maintain the closure invariant, we first have to realise the @@ -166,23 +156,24 @@ void PathSubstitutionGoal::tryNext() if (i != storePath) /* ignore self-references */ addWaitee(worker.makePathSubstitutionGoal(i)); - if (waitees.empty()) /* to prevent hang (no wake-up event) */ - referencesValid(); - else + if (waitees.empty()) {/* to prevent hang (no wake-up event) */ + return referencesValid(); + } else { state = &PathSubstitutionGoal::referencesValid; + return StillAlive{}; + } } -void PathSubstitutionGoal::referencesValid() +Goal::WorkResult PathSubstitutionGoal::referencesValid() { trace("all references realised"); if (nrFailed > 0) { - done( + return done( nrNoSubstituters > 0 || nrIncompleteClosure > 0 ? ecIncompleteClosure : ecFailed, BuildResult::DependencyFailed, fmt("some references of path '%s' could not be realised", worker.store.printStorePath(storePath))); - return; } for (auto & i : info->references) @@ -191,10 +182,11 @@ void PathSubstitutionGoal::referencesValid() state = &PathSubstitutionGoal::tryToRun; worker.wakeUp(shared_from_this()); + return StillAlive{}; } -void PathSubstitutionGoal::tryToRun() +Goal::WorkResult PathSubstitutionGoal::tryToRun() { trace("trying to run"); @@ -203,7 +195,7 @@ void PathSubstitutionGoal::tryToRun() prevents infinite waiting. */ if (worker.getNrSubstitutions() >= std::max(1U, (unsigned int) settings.maxSubstitutionJobs)) { worker.waitForBuildSlot(shared_from_this()); - return; + return StillAlive{}; } maintainRunningSubstitutions = std::make_unique<MaintainCount<uint64_t>>(worker.runningSubstitutions); @@ -236,10 +228,11 @@ void PathSubstitutionGoal::tryToRun() worker.childStarted(shared_from_this(), {outPipe.readSide.get()}, true, false); state = &PathSubstitutionGoal::finished; + return StillAlive{}; } -void PathSubstitutionGoal::finished() +Goal::WorkResult PathSubstitutionGoal::finished() { trace("substitute finished"); @@ -264,7 +257,7 @@ void PathSubstitutionGoal::finished() /* Try the next substitute. */ state = &PathSubstitutionGoal::tryNext; worker.wakeUp(shared_from_this()); - return; + return StillAlive{}; } worker.markContentsGood(storePath); @@ -285,12 +278,13 @@ void PathSubstitutionGoal::finished() worker.doneNarSize += maintainExpectedNar->delta; maintainExpectedNar.reset(); - done(ecSuccess, BuildResult::Substituted); + return done(ecSuccess, BuildResult::Substituted); } -void PathSubstitutionGoal::handleChildOutput(int fd, std::string_view data) +Goal::WorkResult PathSubstitutionGoal::handleChildOutput(int fd, std::string_view data) { + return StillAlive{}; } diff --git a/src/libstore/build/substitution-goal.hh b/src/libstore/build/substitution-goal.hh index d85b3beb3..89cf3c759 100644 --- a/src/libstore/build/substitution-goal.hh +++ b/src/libstore/build/substitution-goal.hh @@ -66,7 +66,7 @@ struct PathSubstitutionGoal : public Goal std::unique_ptr<MaintainCount<uint64_t>> maintainExpectedSubstitutions, maintainRunningSubstitutions, maintainExpectedNar, maintainExpectedDownload; - typedef void (PathSubstitutionGoal::*GoalState)(); + typedef WorkResult (PathSubstitutionGoal::*GoalState)(); GoalState state; /** @@ -74,7 +74,7 @@ struct PathSubstitutionGoal : public Goal */ std::optional<ContentAddress> ca; - void done( + Finished done( ExitCode result, BuildResult::Status status, std::optional<std::string> errorMsg = {}); @@ -83,7 +83,7 @@ public: PathSubstitutionGoal(const StorePath & storePath, Worker & worker, RepairFlag repair = NoRepair, std::optional<ContentAddress> ca = std::nullopt); ~PathSubstitutionGoal(); - void timedOut(Error && ex) override { abort(); }; + Finished timedOut(Error && ex) override { abort(); }; /** * We prepend "a$" to the key name to ensure substitution goals @@ -94,22 +94,22 @@ public: return "a$" + std::string(storePath.name()) + "$" + worker.store.printStorePath(storePath); } - void work() override; + WorkResult work() override; /** * The states. */ - void init(); - void tryNext(); - void gotInfo(); - void referencesValid(); - void tryToRun(); - void finished(); + WorkResult init(); + WorkResult tryNext(); + WorkResult gotInfo(); + WorkResult referencesValid(); + WorkResult tryToRun(); + WorkResult finished(); /** * Callback used by the worker to write to the log. */ - void handleChildOutput(int fd, std::string_view data) override; + WorkResult handleChildOutput(int fd, std::string_view data) override; /* Called by destructor, can't be overridden */ void cleanup() override final; diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 5b2e36acb..f4c352b61 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -139,6 +139,40 @@ static void removeGoal(std::shared_ptr<G> goal, std::map<K, std::weak_ptr<G>> & } +void Worker::goalFinished(GoalPtr goal, Goal::Finished & f) +{ + goal->trace("done"); + assert(!goal->exitCode.has_value()); + goal->exitCode = f.result; + + if (f.ex) { + if (!goal->waiters.empty()) + logError(f.ex->info()); + else + goal->ex = std::move(f.ex); + } + + for (auto & i : goal->waiters) { + if (GoalPtr waiting = i.lock()) { + waiting->waiteeDone(goal, f.result); + } + } + goal->waiters.clear(); + removeGoal(goal); + goal->cleanup(); +} + +void Worker::handleWorkResult(GoalPtr goal, Goal::WorkResult how) +{ + std::visit( + overloaded{ + [&](Goal::StillAlive) {}, + [&](Goal::Finished & f) { goalFinished(goal, f); }, + }, + how + ); +} + void Worker::removeGoal(GoalPtr goal) { if (auto drvGoal = std::dynamic_pointer_cast<DerivationGoal>(goal)) @@ -299,7 +333,7 @@ void Worker::run(const Goals & _topGoals) awake.clear(); for (auto & goal : awake2) { checkInterrupt(); - goal->work(); + handleWorkResult(goal, goal->work()); actDerivations.progress( doneBuilds, expectedBuilds + doneBuilds, runningBuilds, failedBuilds @@ -429,9 +463,14 @@ void Worker::waitForInput() j->respectTimeouts && after - j->lastOutput >= std::chrono::seconds(settings.maxSilentTime)) { - goal->timedOut(Error( + handleWorkResult( + goal, + goal->timedOut(Error( "%1% timed out after %2% seconds of silence", - goal->getName(), settings.maxSilentTime)); + goal->getName(), + settings.maxSilentTime + )) + ); continue; } @@ -440,9 +479,12 @@ void Worker::waitForInput() j->respectTimeouts && after - j->timeStarted >= std::chrono::seconds(settings.buildTimeout)) { - goal->timedOut(Error( - "%1% timed out after %2% seconds", - goal->getName(), settings.buildTimeout)); + handleWorkResult( + goal, + goal->timedOut( + Error("%1% timed out after %2% seconds", goal->getName(), settings.buildTimeout) + ) + ); continue; } @@ -469,7 +511,7 @@ void Worker::waitForInput() goal->getName(), rd); std::string_view data((char *) buffer.data(), rd); j->lastOutput = after; - goal->handleChildOutput(k, data); + handleWorkResult(goal, goal->handleChildOutput(k, data)); } } } diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index 3984c9c1c..5af93b49e 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -105,6 +105,9 @@ private: */ std::map<StorePath, bool> pathContentsGoodCache; + void goalFinished(GoalPtr goal, Goal::Finished & f); + void handleWorkResult(GoalPtr goal, Goal::WorkResult how); + public: const Activity act; @@ -135,7 +138,17 @@ public: Store & store; Store & evalStore; - std::unique_ptr<HookInstance> hook; + struct HookState { + std::unique_ptr<HookInstance> instance; + + /** + * Whether to ask the build hook if it can build a derivation. If + * it answers with "decline-permanently", we don't try again. + */ + bool available = true; + }; + + HookState hook; uint64_t expectedBuilds = 0; uint64_t doneBuilds = 0; @@ -151,12 +164,6 @@ public: uint64_t expectedNarSize = 0; uint64_t doneNarSize = 0; - /** - * Whether to ask the build hook if it can build a derivation. If - * it answers with "decline-permanently", we don't try again. - */ - bool tryBuildHook = true; - Worker(Store & store, Store & evalStore); ~Worker(); diff --git a/src/libutil/processes.cc b/src/libutil/processes.cc index 05b1321e9..866ba9647 100644 --- a/src/libutil/processes.cc +++ b/src/libutil/processes.cc @@ -280,9 +280,14 @@ RunningProgram::~RunningProgram() void RunningProgram::wait() { - int status = pid.wait(); - if (status) - throw ExecError(status, "program '%1%' %2%", program, statusToString(status)); + if (std::uncaught_exceptions() == 0) { + int status = pid.wait(); + if (status) + throw ExecError(status, "program '%1%' %2%", program, statusToString(status)); + } else { + pid.kill(); + debug("killed subprocess %1% during exception handling", program); + } } RunningProgram runProgram2(const RunOptions & options) diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 63b250d13..9d18b81b8 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -15,6 +15,7 @@ #include "registry.hh" #include "eval-cache.hh" #include "markdown.hh" +#include "terminal.hh" #include <nlohmann/json.hpp> #include <queue> @@ -1225,25 +1226,42 @@ struct CmdFlakeShow : FlakeCommand, MixJSON auto showDerivation = [&]() { auto name = visitor.getAttr(state->sName)->getString(); + std::optional<std::string> description; + if (auto aMeta = visitor.maybeGetAttr(state->sMeta)) { + if (auto aDescription = aMeta->maybeGetAttr(state->sDescription)) + description = aDescription->getString(); + } + if (json) { - std::optional<std::string> description; - if (auto aMeta = visitor.maybeGetAttr(state->sMeta)) { - if (auto aDescription = aMeta->maybeGetAttr(state->sDescription)) - description = aDescription->getString(); - } j.emplace("type", "derivation"); j.emplace("name", name); if (description) j.emplace("description", *description); } else { - logger->cout("%s: %s '%s'", - headerPrefix, + auto type = attrPath.size() == 2 && attrPathS[0] == "devShell" ? "development environment" : attrPath.size() >= 2 && attrPathS[0] == "devShells" ? "development environment" : attrPath.size() == 3 && attrPathS[0] == "checks" ? "derivation" : attrPath.size() >= 1 && attrPathS[0] == "hydraJobs" ? "derivation" : - "package", - name); + "package"; + if (description && !description->empty()) { + // Trim the string and only display the first line of the description. + auto desc = nix::trim(*description); + auto firstLineDesc = desc.substr(0, desc.find('\n')); + + std::string output = fmt("%s: %s '%s' - '%s'", headerPrefix, type, name, firstLineDesc); + if (output.size() > getWindowSize().second) { + // we resize to 4 less then the window size to account for the "...'" we append to + // the final string, we also include the ' since that is removed when we truncate the string + output.resize(getWindowSize().second - 4); + output.append("...'"); + } + + logger->cout("%s", output.c_str()); + } + else { + logger->cout("%s: %s '%s'", headerPrefix, type, name); + } } }; diff --git a/tests/functional/fetchers.sh b/tests/functional/fetchers.sh new file mode 100644 index 000000000..0f888dc33 --- /dev/null +++ b/tests/functional/fetchers.sh @@ -0,0 +1,91 @@ +source common.sh + +requireGit + +clearStore + +testFetchTreeError() { + rawFetchTreeArg="${1?fetchTree arg missing}" + messageSubstring="${2?messageSubstring missing}" + + output="$(nix eval --impure --raw --expr "(builtins.fetchTree $rawFetchTreeArg).outPath" 2>&1)" && status=0 || status=$? + grepQuiet "$messageSubstring" <<<"$output" + test "$status" -ne 0 +} + +# github/gitlab/sourcehut fetcher input validation +for provider in github gitlab sourcehut; do + # ref/rev validation + testFetchTreeError \ + "{ type = \"$provider\"; owner = \"foo\"; repo = \"bar\"; ref = \",\"; }" \ + "URL '$provider:foo/bar' contains an invalid branch/tag name" + + testFetchTreeError \ + "\"$provider://host/foo/bar/,\"" \ + "URL '$provider:foo/bar', ',' is not a commit hash or a branch/tag name" + + testFetchTreeError \ + "\"$provider://host/foo/bar/f16d8f43dd0998cdb315a2cccf2e4d10027e7ca4?rev=abc\"" \ + "URL '$provider://host/foo/bar/f16d8f43dd0998cdb315a2cccf2e4d10027e7ca4?rev=abc' already contains a ref or rev" + + testFetchTreeError \ + "\"$provider://host/foo/bar/ref?ref=ref2\"" \ + "URL '$provider://host/foo/bar/ref?ref=ref2' already contains a ref or rev" + + # host validation + testFetchTreeError \ + "{ type = \"$provider\"; owner = \"foo\"; repo = \"bar\"; host = \"git_hub.com\"; }" \ + "URL '$provider:foo/bar' contains an invalid instance host" + + testFetchTreeError \ + "\"$provider://host/foo/bar/ref?host=git_hub.com\"" \ + "URL '$provider:foo/bar' contains an invalid instance host" + + # invalid attributes + testFetchTreeError \ + "{ type = \"$provider\"; owner = \"foo\"; repo = \"bar\"; wrong = true; }" \ + "unsupported input attribute 'wrong'" + + testFetchTreeError \ + "\"$provider://host/foo/bar/ref?wrong=1\"" \ + "unsupported input attribute 'wrong'" +done + +# unsupported attributes w/ tarball fetcher +testFetchTreeError \ + "\"https://host/foo?wrong=1\"" \ + "unsupported tarball input attribute 'wrong'. If you wanted to fetch a tarball with a query parameter, please use '{ type = \"tarball\"; url = \"...\"; }" + +# test for unsupported attributes / validation in git fetcher +testFetchTreeError \ + "\"git+https://github.com/owner/repo?invalid=1\"" \ + "unsupported Git input attribute 'invalid'" + +testFetchTreeError \ + "\"git+https://github.com/owner/repo?url=foo\"" \ + "URL 'git+https://github.com/owner/repo?url=foo' must not override url via query param!" + +testFetchTreeError \ + "\"git+https://github.com/owner/repo?ref=foo.lock\"" \ + "invalid Git branch/tag name 'foo.lock'" + +testFetchTreeError \ + "{ type = \"git\"; url =\"https://github.com/owner/repo\"; ref = \"foo.lock\"; }" \ + "invalid Git branch/tag name 'foo.lock'" + +# same for mercurial +testFetchTreeError \ + "\"hg+https://forge.tld/owner/repo?invalid=1\"" \ + "unsupported Mercurial input attribute 'invalid'" + +testFetchTreeError \ + "{ type = \"hg\"; url = \"https://forge.tld/owner/repo\"; invalid = 1; }" \ + "unsupported Mercurial input attribute 'invalid'" + +testFetchTreeError \ + "\"hg+https://forge.tld/owner/repo?ref=,\"" \ + "invalid Mercurial branch/tag name ','" + +testFetchTreeError \ + "{ type = \"hg\"; url = \"https://forge.tld/owner/repo\"; ref = \",\"; }" \ + "invalid Mercurial branch/tag name ','" diff --git a/tests/functional/flakes/show.sh b/tests/functional/flakes/show.sh index a3d300552..25f481575 100644 --- a/tests/functional/flakes/show.sh +++ b/tests/functional/flakes/show.sh @@ -85,3 +85,31 @@ assert show_output.legacyPackages.${builtins.currentSystem}.AAAAAASomeThingsFail assert show_output.legacyPackages.${builtins.currentSystem}.simple.name == "simple"; true ' + +cat >flake.nix<<EOF +{ + outputs = inputs: { + packages.$system = { + aNoDescription = import ./simple.nix; + bOneLineDescription = import ./simple.nix // { meta.description = "one line"; }; + cMultiLineDescription = import ./simple.nix // { meta.description = '' + line one + line two + ''; }; + dLongDescription = import ./simple.nix // { meta.description = '' + abcdefghijklmnopqrstuvwxyz + ''; }; + eEmptyDescription = import ./simple.nix // { meta.description = ""; }; + }; + }; +} +EOF +unbuffer sh -c ' + stty rows 20 cols 100 + nix flake show > show-output.txt +' +test "$(awk -F '[:] ' '/aNoDescription/{print $NF}' ./show-output.txt)" = "package 'simple'" +test "$(awk -F '[:] ' '/bOneLineDescription/{print $NF}' ./show-output.txt)" = "package 'simple' - 'one line'" +test "$(awk -F '[:] ' '/cMultiLineDescription/{print $NF}' ./show-output.txt)" = "package 'simple' - 'line one'" +test "$(awk -F '[:] ' '/dLongDescription/{print $NF}' ./show-output.txt)" = "package 'simple' - 'abcdefghijklmnopqrs...'" +test "$(awk -F '[:] ' '/eEmptyDescription/{print $NF}' ./show-output.txt)" = "package 'simple'" diff --git a/tests/functional/flakes/subdir-flake.sh b/tests/functional/flakes/subdir-flake.sh new file mode 100644 index 000000000..399518502 --- /dev/null +++ b/tests/functional/flakes/subdir-flake.sh @@ -0,0 +1,20 @@ +source common.sh +requireGit +clearStore + +container="$TEST_HOME"/flake-container +flake_dir="$container"/flake-dir + +createGitRepo "$container" +mkdir -p "$flake_dir" +writeSimpleFlake "$flake_dir" +git -C "$container" add flake-dir + +pushd "$flake_dir" &>/dev/null + info="$(nix flake info --json)" + [[ "$(jq -r '.resolvedUrl' <<<"$info")" == git+file://*/flake-container?dir=flake-dir ]] + [[ "$(jq -r '.url' <<<"$info")" == git+file://*/flake-container?dir=flake-dir ]] + + # Make sure we can actually access & build stuff in this flake. + nix build "path:$flake_dir#foo" -L +popd &>/dev/null diff --git a/tests/functional/meson.build b/tests/functional/meson.build index cbf6a1563..7a9c7182f 100644 --- a/tests/functional/meson.build +++ b/tests/functional/meson.build @@ -71,6 +71,7 @@ functional_tests_scripts = [ 'flakes/build-paths.sh', 'flakes/flake-registry.sh', 'flakes/flake-in-submodule.sh', + 'flakes/subdir-flake.sh', 'gc.sh', 'nix-collect-garbage-d.sh', 'nix-collect-garbage-dry-run.sh', @@ -94,6 +95,7 @@ functional_tests_scripts = [ 'fetchGitRefs.sh', 'gc-runtime.sh', 'tarball.sh', + 'fetchers.sh', 'fetchGit.sh', 'fetchurl.sh', 'fetchPath.sh', diff --git a/tests/nixos/tarball-flakes.nix b/tests/nixos/tarball-flakes.nix index ca7627bd1..5deba4a12 100644 --- a/tests/nixos/tarball-flakes.nix +++ b/tests/nixos/tarball-flakes.nix @@ -69,7 +69,7 @@ in # Check that we got redirected to the immutable URL. locked_url = info["locked"]["url"] - assert locked_url == "http://localhost/stable/${nixpkgs.rev}.tar.gz", f"{locked_url=} != http://localhost/stable/${nixpkgs.rev}.tar.gz" + assert locked_url == "http://localhost/stable/${nixpkgs.rev}.tar.gz?rev=${nixpkgs.rev}&revCount=1234", f"{locked_url=} != http://localhost/stable/${nixpkgs.rev}.tar.gz" # Check that we got the rev and revCount attributes. revision = info["revision"] |