From b2748c6e99239ff6803ba0da76c362790c8be192 Mon Sep 17 00:00:00 2001 From: Lucas Franceschino Date: Mon, 25 May 2020 19:07:38 +0200 Subject: [PATCH 01/42] Make `functionArgs` primitive accept primops --- src/libexpr/primops.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 0a4236da47a0..5875457acf97 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1346,6 +1346,10 @@ static void prim_catAttrs(EvalState & state, const Pos & pos, Value * * args, Va static void prim_functionArgs(EvalState & state, const Pos & pos, Value * * args, Value & v) { state.forceValue(*args[0], pos); + if (args[0]->type == tPrimOpApp || args[0]->type == tPrimOp) { + state.mkAttrs(v, 0); + return; + } if (args[0]->type != tLambda) throw TypeError(format("'functionArgs' requires a function, at %1%") % pos); From a303c0b6dc71b1e0d6a57986c3f7a9b61361cd92 Mon Sep 17 00:00:00 2001 From: Greg Hale Date: Wed, 17 Jun 2020 15:08:59 -0400 Subject: [PATCH 02/42] Fetch commits from github/gitlab using Auth header `nix flake info` calls the github 'commits' API, which requires authorization when the repository is private. Currently this request fails with a 404. This commit adds an authorization header when calling the 'commits' API. It also changes the way that the 'tarball' API authenticates, moving the user's token from a query parameter into the Authorization header. The query parameter method is recently deprecated and will be disallowed in November 2020. Using them today triggers a warning email. --- src/libexpr/common-eval-args.cc | 2 +- src/libexpr/parser.y | 2 +- src/libexpr/primops/fetchTree.cc | 4 +- src/libfetchers/fetchers.hh | 2 + src/libfetchers/github.cc | 76 +++++++++++++++++++++++--------- src/libfetchers/registry.cc | 2 +- src/libfetchers/tarball.cc | 9 ++-- src/libstore/filetransfer.cc | 3 ++ src/libstore/filetransfer.hh | 4 ++ src/libstore/globals.hh | 3 ++ src/libutil/types.hh | 2 + src/nix-channel/nix-channel.cc | 6 +-- 12 files changed, 84 insertions(+), 31 deletions(-) diff --git a/src/libexpr/common-eval-args.cc b/src/libexpr/common-eval-args.cc index 10c1a697509a..d71aa22f1987 100644 --- a/src/libexpr/common-eval-args.cc +++ b/src/libexpr/common-eval-args.cc @@ -76,7 +76,7 @@ Path lookupFileArg(EvalState & state, string s) if (isUri(s)) { return state.store->toRealPath( fetchers::downloadTarball( - state.store, resolveUri(s), "source", false).first.storePath); + state.store, resolveUri(s), Headers {}, "source", false).first.storePath); } else if (s.size() > 2 && s.at(0) == '<' && s.at(s.size() - 1) == '>') { Path p = s.substr(1, s.size() - 2); return state.findFile(p); diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 24b21f7da332..28e31f46b51e 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -719,7 +719,7 @@ std::pair EvalState::resolveSearchPathElem(const SearchPathEl if (isUri(elem.second)) { try { res = { true, store->toRealPath(fetchers::downloadTarball( - store, resolveUri(elem.second), "source", false).first.storePath) }; + store, resolveUri(elem.second), Headers {}, "source", false).first.storePath) }; } catch (FileTransferError & e) { logWarning({ .name = "Entry download", diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index 06e8304b8bc5..3001957b4e42 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -201,8 +201,8 @@ static void fetch(EvalState & state, const Pos & pos, Value * * args, Value & v, auto storePath = unpack - ? fetchers::downloadTarball(state.store, *url, name, (bool) expectedHash).first.storePath - : fetchers::downloadFile(state.store, *url, name, (bool) expectedHash).storePath; + ? fetchers::downloadTarball(state.store, *url, Headers {}, name, (bool) expectedHash).first.storePath + : fetchers::downloadFile(state.store, *url, Headers{}, name, (bool) expectedHash).storePath; auto path = state.store->toRealPath(storePath); diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index 89b1e6e7dda1..62807e53bf12 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -118,12 +118,14 @@ struct DownloadFileResult DownloadFileResult downloadFile( ref store, const std::string & url, + const Headers & headers, const std::string & name, bool immutable); std::pair downloadTarball( ref store, const std::string & url, + const Headers & headers, const std::string & name, bool immutable); diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index 1cc0c5e2e7cf..d8d0351b9c2c 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -3,11 +3,24 @@ #include "fetchers.hh" #include "globals.hh" #include "store-api.hh" +#include "types.hh" #include namespace nix::fetchers { +struct DownloadUrl +{ + std::string url; + std::optional> access_token_header; + + DownloadUrl(const std::string & url) + : url(url) { } + + DownloadUrl(const std::string & url, const std::pair & access_token_header) + : url(url), access_token_header(access_token_header) { } +}; + // A github or gitlab url const static std::string urlRegexS = "[a-zA-Z0-9.]*"; // FIXME: check std::regex urlRegex(urlRegexS, std::regex::ECMAScript); @@ -16,6 +29,8 @@ struct GitArchiveInputScheme : InputScheme { virtual std::string type() = 0; + virtual std::pair accessHeaderFromToken(const std::string & token) const = 0; + std::optional inputFromURL(const ParsedURL & url) override { if (url.scheme != type()) return {}; @@ -131,7 +146,7 @@ struct GitArchiveInputScheme : InputScheme virtual Hash getRevFromRef(nix::ref store, const Input & input) const = 0; - virtual std::string getDownloadUrl(const Input & input) const = 0; + virtual DownloadUrl getDownloadUrl(const Input & input) const = 0; std::pair fetch(ref store, const Input & _input) override { @@ -160,7 +175,12 @@ struct GitArchiveInputScheme : InputScheme auto url = getDownloadUrl(input); - auto [tree, lastModified] = downloadTarball(store, url, "source", true); + Headers headers; + if (url.access_token_header) { + headers.push_back(*url.access_token_header); + } + + auto [tree, lastModified] = downloadTarball(store, url.url, headers, "source", true); input.attrs.insert_or_assign("lastModified", lastModified); @@ -182,11 +202,8 @@ struct GitHubInputScheme : GitArchiveInputScheme { std::string type() override { return "github"; } - void addAccessToken(std::string & url) const - { - std::string accessToken = settings.githubAccessToken.get(); - if (accessToken != "") - url += "?access_token=" + accessToken; + std::pair accessHeaderFromToken(const std::string & token) const { + return std::pair("Authorization", fmt("token %s", token)); } Hash getRevFromRef(nix::ref store, const Input & input) const override @@ -195,18 +212,21 @@ struct GitHubInputScheme : GitArchiveInputScheme auto url = fmt("https://api.%s/repos/%s/%s/commits/%s", // FIXME: check host_url, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"), *input.getRef()); - addAccessToken(url); + Headers headers; + std::string accessToken = settings.githubAccessToken.get(); + if (accessToken != "") + headers.push_back(accessHeaderFromToken(accessToken)); auto json = nlohmann::json::parse( readFile( store->toRealPath( - downloadFile(store, url, "source", false).storePath))); + downloadFile(store, url, headers, "source", false).storePath))); auto rev = Hash::parseAny(std::string { json["sha"] }, htSHA1); debug("HEAD revision for '%s' is %s", url, rev.gitRev()); return rev; } - std::string getDownloadUrl(const Input & input) const override + DownloadUrl getDownloadUrl(const Input & input) const override { // FIXME: use regular /archive URLs instead? api.github.com // might have stricter rate limits. @@ -215,9 +235,13 @@ struct GitHubInputScheme : GitArchiveInputScheme host_url, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"), input.getRev()->to_string(Base16, false)); - addAccessToken(url); - - return url; + std::string accessToken = settings.githubAccessToken.get(); + if (accessToken != "") { + auto auth_header = accessHeaderFromToken(accessToken); + return DownloadUrl(url, auth_header); + } else { + return DownloadUrl(url); + } } void clone(const Input & input, const Path & destDir) override @@ -234,21 +258,31 @@ struct GitLabInputScheme : GitArchiveInputScheme { std::string type() override { return "gitlab"; } + std::pair accessHeaderFromToken(const std::string & token) const { + return std::pair("Authorization", fmt("Bearer %s", token)); + } + Hash getRevFromRef(nix::ref store, const Input & input) const override { auto host_url = maybeGetStrAttr(input.attrs, "url").value_or("gitlab.com"); auto url = fmt("https://%s/api/v4/projects/%s%%2F%s/repository/commits?ref_name=%s", host_url, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"), *input.getRef()); + + Headers headers; + std::string accessToken = settings.gitlabAccessToken.get(); + if (accessToken != "") + headers.push_back(accessHeaderFromToken(accessToken)); + auto json = nlohmann::json::parse( readFile( store->toRealPath( - downloadFile(store, url, "source", false).storePath))); + downloadFile(store, url, headers, "source", false).storePath))); auto rev = Hash::parseAny(std::string(json[0]["id"]), htSHA1); debug("HEAD revision for '%s' is %s", url, rev.gitRev()); return rev; } - std::string getDownloadUrl(const Input & input) const override + DownloadUrl getDownloadUrl(const Input & input) const override { // FIXME: This endpoint has a rate limit threshold of 5 requests per minute auto host_url = maybeGetStrAttr(input.attrs, "url").value_or("gitlab.com"); @@ -256,12 +290,14 @@ struct GitLabInputScheme : GitArchiveInputScheme host_url, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"), input.getRev()->to_string(Base16, false)); - /* # FIXME: add privat token auth (`curl --header "PRIVATE-TOKEN: "`) - std::string accessToken = settings.githubAccessToken.get(); - if (accessToken != "") - url += "?access_token=" + accessToken;*/ + std::string accessToken = settings.gitlabAccessToken.get(); + if (accessToken != "") { + auto auth_header = accessHeaderFromToken(accessToken); + return DownloadUrl(url, auth_header); + } else { + return DownloadUrl(url); + } - return url; } void clone(const Input & input, const Path & destDir) override diff --git a/src/libfetchers/registry.cc b/src/libfetchers/registry.cc index 4367ee8103b2..551e7684acac 100644 --- a/src/libfetchers/registry.cc +++ b/src/libfetchers/registry.cc @@ -145,7 +145,7 @@ static std::shared_ptr getGlobalRegistry(ref store) auto path = settings.flakeRegistry.get(); if (!hasPrefix(path, "/")) { - auto storePath = downloadFile(store, path, "flake-registry.json", false).storePath; + auto storePath = downloadFile(store, path, Headers {}, "flake-registry.json", false).storePath; if (auto store2 = store.dynamic_pointer_cast()) store2->addPermRoot(storePath, getCacheDir() + "/nix/flake-registry.json"); path = store->toRealPath(storePath); diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index a2d16365e061..cf6d6e3d2499 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -5,12 +5,14 @@ #include "store-api.hh" #include "archive.hh" #include "tarfile.hh" +#include "types.hh" namespace nix::fetchers { DownloadFileResult downloadFile( ref store, const std::string & url, + const Headers & headers, const std::string & name, bool immutable) { @@ -36,7 +38,7 @@ DownloadFileResult downloadFile( if (cached && !cached->expired) return useCached(); - FileTransferRequest request(url); + FileTransferRequest request(url, headers); if (cached) request.expectedETag = getStrAttr(cached->infoAttrs, "etag"); FileTransferResult res; @@ -110,6 +112,7 @@ DownloadFileResult downloadFile( std::pair downloadTarball( ref store, const std::string & url, + const Headers & headers, const std::string & name, bool immutable) { @@ -127,7 +130,7 @@ std::pair downloadTarball( getIntAttr(cached->infoAttrs, "lastModified") }; - auto res = downloadFile(store, url, name, immutable); + auto res = downloadFile(store, url, headers, name, immutable); std::optional unpackedStorePath; time_t lastModified; @@ -222,7 +225,7 @@ struct TarballInputScheme : InputScheme std::pair fetch(ref store, const Input & input) override { - auto tree = downloadTarball(store, getStrAttr(input.attrs, "url"), "source", false).first; + auto tree = downloadTarball(store, getStrAttr(input.attrs, "url"), Headers {}, "source", false).first; return {std::move(tree), input}; } }; diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 4149f81550d6..13ed429fa876 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -112,6 +112,9 @@ struct curlFileTransfer : public FileTransfer requestHeaders = curl_slist_append(requestHeaders, ("If-None-Match: " + request.expectedETag).c_str()); if (!request.mimeType.empty()) requestHeaders = curl_slist_append(requestHeaders, ("Content-Type: " + request.mimeType).c_str()); + for (auto it = request.headers.begin(); it != request.headers.end(); ++it){ + requestHeaders = curl_slist_append(requestHeaders, fmt("%s: %s", it->first, it->second).c_str()); + } } ~TransferItem() diff --git a/src/libstore/filetransfer.hh b/src/libstore/filetransfer.hh index 0d608c8d8986..7e302ff39c7b 100644 --- a/src/libstore/filetransfer.hh +++ b/src/libstore/filetransfer.hh @@ -51,6 +51,7 @@ extern FileTransferSettings fileTransferSettings; struct FileTransferRequest { std::string uri; + Headers headers; std::string expectedETag; bool verifyTLS = true; bool head = false; @@ -65,6 +66,9 @@ struct FileTransferRequest FileTransferRequest(const std::string & uri) : uri(uri), parentAct(getCurActivity()) { } + FileTransferRequest(const std::string & uri, Headers headers) + : uri(uri), headers(headers) { } + std::string verb() { return data ? "upload" : "download"; diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 02721285ae30..b2e7610ee6d5 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -863,6 +863,9 @@ public: Setting githubAccessToken{this, "", "github-access-token", "GitHub access token to get access to GitHub data through the GitHub API for `github:<..>` flakes."}; + Setting gitlabAccessToken{this, "", "gitlab-access-token", + "GitLab access token to get access to GitLab data through the GitLab API for gitlab:<..> flakes."}; + Setting experimentalFeatures{this, {}, "experimental-features", "Experimental Nix features to enable."}; diff --git a/src/libutil/types.hh b/src/libutil/types.hh index 3af485fa0feb..55d02bcf9a6f 100644 --- a/src/libutil/types.hh +++ b/src/libutil/types.hh @@ -24,6 +24,8 @@ typedef string Path; typedef list Paths; typedef set PathSet; +typedef vector> Headers; + /* Helper class to run code at startup. */ template struct OnStartup diff --git a/src/nix-channel/nix-channel.cc b/src/nix-channel/nix-channel.cc index 3ccf620c9464..94d33a75c78d 100755 --- a/src/nix-channel/nix-channel.cc +++ b/src/nix-channel/nix-channel.cc @@ -87,7 +87,7 @@ static void update(const StringSet & channelNames) // We want to download the url to a file to see if it's a tarball while also checking if we // got redirected in the process, so that we can grab the various parts of a nix channel // definition from a consistent location if the redirect changes mid-download. - auto result = fetchers::downloadFile(store, url, std::string(baseNameOf(url)), false); + auto result = fetchers::downloadFile(store, url, Headers {}, std::string(baseNameOf(url)), false); auto filename = store->toRealPath(result.storePath); url = result.effectiveUrl; @@ -112,9 +112,9 @@ static void update(const StringSet & channelNames) if (!unpacked) { // Download the channel tarball. try { - filename = store->toRealPath(fetchers::downloadFile(store, url + "/nixexprs.tar.xz", "nixexprs.tar.xz", false).storePath); + filename = store->toRealPath(fetchers::downloadFile(store, url + "/nixexprs.tar.xz", Headers {}, "nixexprs.tar.xz", false).storePath); } catch (FileTransferError & e) { - filename = store->toRealPath(fetchers::downloadFile(store, url + "/nixexprs.tar.bz2", "nixexprs.tar.bz2", false).storePath); + filename = store->toRealPath(fetchers::downloadFile(store, url + "/nixexprs.tar.bz2", Headers {}, "nixexprs.tar.bz2", false).storePath); } } From 993229cdaf2e2347a204c876ecd660fc94048101 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sat, 22 Aug 2020 20:44:47 +0000 Subject: [PATCH 03/42] Deduplicate basic derivation goals too See comments for security concerns. Also optimize goal creation by not traversing map twice. --- src/libstore/build.cc | 88 +++++++++++++++++++++++++++------------ src/libstore/daemon.cc | 14 +++++++ src/libstore/store-api.hh | 36 ++++++++++++++-- 3 files changed, 109 insertions(+), 29 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 07c5bceb2327..8b206e819661 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -296,9 +296,21 @@ class Worker ~Worker(); /* Make a goal (with caching). */ - GoalPtr makeDerivationGoal(const StorePath & drvPath, const StringSet & wantedOutputs, BuildMode buildMode = bmNormal); - std::shared_ptr makeBasicDerivationGoal(const StorePath & drvPath, - const BasicDerivation & drv, BuildMode buildMode = bmNormal); + + /* derivation goal */ +private: + std::shared_ptr makeDerivationGoalCommon( + const StorePath & drvPath, const StringSet & wantedOutputs, + std::function()> mkDrvGoal); +public: + std::shared_ptr makeDerivationGoal( + const StorePath & drvPath, + const StringSet & wantedOutputs, BuildMode buildMode = bmNormal); + std::shared_ptr makeBasicDerivationGoal( + const StorePath & drvPath, const BasicDerivation & drv, + const StringSet & wantedOutputs, BuildMode buildMode = bmNormal); + + /* substitution goal */ GoalPtr makeSubstitutionGoal(const StorePath & storePath, RepairFlag repair = NoRepair, std::optional ca = std::nullopt); /* Remove a dead goal. */ @@ -949,10 +961,12 @@ class DerivationGoal : public Goal friend struct RestrictedStore; public: - DerivationGoal(const StorePath & drvPath, const StringSet & wantedOutputs, - Worker & worker, BuildMode buildMode = bmNormal); + DerivationGoal(const StorePath & drvPath, + const StringSet & wantedOutputs, Worker & worker, + BuildMode buildMode = bmNormal); DerivationGoal(const StorePath & drvPath, const BasicDerivation & drv, - Worker & worker, BuildMode buildMode = bmNormal); + const StringSet & wantedOutputs, Worker & worker, + BuildMode buildMode = bmNormal); ~DerivationGoal(); /* Whether we need to perform hash rewriting if there are valid output paths. */ @@ -1085,8 +1099,8 @@ class DerivationGoal : public Goal const Path DerivationGoal::homeDir = "/homeless-shelter"; -DerivationGoal::DerivationGoal(const StorePath & drvPath, const StringSet & wantedOutputs, - Worker & worker, BuildMode buildMode) +DerivationGoal::DerivationGoal(const StorePath & drvPath, + const StringSet & wantedOutputs, Worker & worker, BuildMode buildMode) : Goal(worker) , useDerivation(true) , drvPath(drvPath) @@ -1094,7 +1108,9 @@ DerivationGoal::DerivationGoal(const StorePath & drvPath, const StringSet & want , buildMode(buildMode) { state = &DerivationGoal::getDerivation; - name = fmt("building of '%s'", worker.store.printStorePath(this->drvPath)); + name = fmt( + "building of '%s' from .drv file", + StorePathWithOutputs { drvPath, wantedOutputs }.to_string(worker.store)); trace("created"); mcExpectedBuilds = std::make_unique>(worker.expectedBuilds); @@ -1103,15 +1119,18 @@ DerivationGoal::DerivationGoal(const StorePath & drvPath, const StringSet & want DerivationGoal::DerivationGoal(const StorePath & drvPath, const BasicDerivation & drv, - Worker & worker, BuildMode buildMode) + const StringSet & wantedOutputs, Worker & worker, BuildMode buildMode) : Goal(worker) , useDerivation(false) , drvPath(drvPath) + , wantedOutputs(wantedOutputs) , buildMode(buildMode) { this->drv = std::make_unique(BasicDerivation(drv)); state = &DerivationGoal::haveDerivation; - name = fmt("building of %s", StorePathWithOutputs { drvPath, drv.outputNames() }.to_string(worker.store)); + name = fmt( + "building of '%s' from in-memory derivation", + StorePathWithOutputs { drvPath, drv.outputNames() }.to_string(worker.store)); trace("created"); mcExpectedBuilds = std::make_unique>(worker.expectedBuilds); @@ -5060,35 +5079,52 @@ Worker::~Worker() } -GoalPtr Worker::makeDerivationGoal(const StorePath & path, - const StringSet & wantedOutputs, BuildMode buildMode) +std::shared_ptr Worker::makeDerivationGoalCommon( + const StorePath & drvPath, + const StringSet & wantedOutputs, + std::function()> mkDrvGoal) { - GoalPtr goal = derivationGoals[path].lock(); // FIXME - if (!goal) { - goal = std::make_shared(path, wantedOutputs, *this, buildMode); - derivationGoals.insert_or_assign(path, goal); + WeakGoalPtr & abstract_goal_weak = derivationGoals[drvPath]; + GoalPtr abstract_goal = abstract_goal_weak.lock(); // FIXME + std::shared_ptr goal; + if (!abstract_goal) { + goal = mkDrvGoal(); + abstract_goal_weak = goal; wakeUp(goal); - } else - (dynamic_cast(goal.get()))->addWantedOutputs(wantedOutputs); + } else { + goal = std::dynamic_pointer_cast(abstract_goal); + assert(goal); + goal->addWantedOutputs(wantedOutputs); + } return goal; } +std::shared_ptr Worker::makeDerivationGoal(const StorePath & drvPath, + const StringSet & wantedOutputs, BuildMode buildMode) +{ + return makeDerivationGoalCommon(drvPath, wantedOutputs, [&]() { + return std::make_shared(drvPath, wantedOutputs, *this, buildMode); + }); +} + + std::shared_ptr Worker::makeBasicDerivationGoal(const StorePath & drvPath, - const BasicDerivation & drv, BuildMode buildMode) + const BasicDerivation & drv, const StringSet & wantedOutputs, BuildMode buildMode) { - auto goal = std::make_shared(drvPath, drv, *this, buildMode); - wakeUp(goal); - return goal; + return makeDerivationGoalCommon(drvPath, wantedOutputs, [&]() { + return std::make_shared(drvPath, drv, wantedOutputs, *this, buildMode); + }); } GoalPtr Worker::makeSubstitutionGoal(const StorePath & path, RepairFlag repair, std::optional ca) { - GoalPtr goal = substitutionGoals[path].lock(); // FIXME + WeakGoalPtr & goal_weak = substitutionGoals[path]; + GoalPtr goal = goal_weak.lock(); // FIXME if (!goal) { goal = std::make_shared(path, *this, repair, ca); - substitutionGoals.insert_or_assign(path, goal); + goal_weak = goal; wakeUp(goal); } return goal; @@ -5519,7 +5555,7 @@ BuildResult LocalStore::buildDerivation(const StorePath & drvPath, const BasicDe BuildMode buildMode) { Worker worker(*this); - auto goal = worker.makeBasicDerivationGoal(drvPath, drv, buildMode); + auto goal = worker.makeBasicDerivationGoal(drvPath, drv, {}, buildMode); BuildResult result; diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 83f8968b0716..ec3391a6db52 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -546,6 +546,20 @@ static void performOp(TunnelLogger * logger, ref store, are in fact content-addressed if we don't trust them. */ assert(derivationIsCA(drv.type()) || trusted); + /* Recompute the derivation path when we cannot trust the original. */ + if (!trusted) { + /* Recomputing the derivation path for input-address derivations + makes it harder to audit them after the fact, since we need the + original not-necessarily-resolved derivation to verify the drv + derivation as adequate claim to the input-addressed output + paths. */ + assert(derivationIsCA(drv.type())); + + Derivation drv2; + static_cast(drv2) = drv; + drvPath = writeDerivation(*store, Derivation { drv2 }); + } + auto res = store->buildDerivation(drvPath, drv, buildMode); logger->stopWork(); to << res.status << res.errorMsg; diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 591140874ca9..3ccee4f75f30 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -479,8 +479,38 @@ public: BuildMode buildMode = bmNormal); /* Build a single non-materialized derivation (i.e. not from an - on-disk .drv file). Note that ‘drvPath’ is only used for - informational purposes. */ + on-disk .drv file). + + ‘drvPath’ is used to deduplicate worker goals so it is imperative that + is correct. That said, it doesn't literally need to be store path that + would be calculated from writing this derivation to the store: it is OK + if it instead is that of a Derivation which would resolve to this (by + taking the outputs of it's input derivations and adding them as input + sources) such that the build time referenceable-paths are the same. + + In the input-addressed case, we usually *do* use an "original" + unresolved derivations's path, as that is what will be used in the + `buildPaths` case. Also, the input-addressed output paths are verified + only by that contents of that specific unresolved derivation, so it is + nice to keep that information around so if the original derivation is + ever obtained later, it can be verified whether the trusted user in fact + used the proper output path. + + In the content-addressed case, we want to always use the + resolved drv path calculated from the provided derivation. This serves + two purposes: + + - It keeps the operation trustless, by ruling out a maliciously + invalid drv path corresponding to a non-resolution-equivalent + derivation. + + - For the floating case in particular, it ensures that the derivation + to output mapping respects the resolution equivalence relation, so + one cannot choose different resolution-equivalent derivations to + subvert dependency coherence (i.e. the property that one doesn't end + up with multiple different versions of dependencies without + explicitly choosing to allow it). + */ virtual BuildResult buildDerivation(const StorePath & drvPath, const BasicDerivation & drv, BuildMode buildMode = bmNormal) = 0; @@ -517,7 +547,7 @@ public: - The collector isn't running, or it's just started but hasn't acquired the GC lock yet. In that case we get and release the lock right away, then exit. The collector scans the - permanent root and sees our's. + permanent root and sees ours. In either case the permanent root is seen by the collector. */ virtual void syncWithGC() { }; From a2f5c921d41c941315ce783f66469bfb20e2c1b3 Mon Sep 17 00:00:00 2001 From: ujjwal Date: Tue, 22 Sep 2020 23:37:06 +0530 Subject: [PATCH 04/42] fixed typo --- src/libexpr/flake/flake.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index 01f464859e5f..783b98ef013e 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -367,7 +367,7 @@ LockedFlake lockFlake( /* If we have an --update-input flag for an input of this input, then we must fetch the flake to - to update it. */ + update it. */ auto lb = lockFlags.inputUpdates.lower_bound(inputPath); auto hasChildUpdate = From 8a2e10827f2f57c3b9a0829357363d5f09af65e2 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 23 Sep 2020 14:08:23 +0200 Subject: [PATCH 05/42] Remove unused Flake::vOutputs field --- src/libexpr/flake/flake.cc | 5 ++--- src/libexpr/flake/flake.hh | 1 - 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index 783b98ef013e..460eea5eaf6b 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -215,10 +215,9 @@ static Flake getFlake( if (auto outputs = vInfo.attrs->get(sOutputs)) { expectType(state, tLambda, *outputs->value, *outputs->pos); - flake.vOutputs = allocRootValue(outputs->value); - if ((*flake.vOutputs)->lambda.fun->matchAttrs) { - for (auto & formal : (*flake.vOutputs)->lambda.fun->formals->formals) { + if (outputs->value->lambda.fun->matchAttrs) { + for (auto & formal : outputs->value->lambda.fun->formals->formals) { if (formal.name != state.sSelf) flake.inputs.emplace(formal.name, FlakeInput { .ref = parseFlakeRef(formal.name) diff --git a/src/libexpr/flake/flake.hh b/src/libexpr/flake/flake.hh index c2bb2888b6fb..69c779af8c53 100644 --- a/src/libexpr/flake/flake.hh +++ b/src/libexpr/flake/flake.hh @@ -34,7 +34,6 @@ struct Flake std::optional description; std::shared_ptr sourceInfo; FlakeInputs inputs; - RootValue vOutputs; ~Flake(); }; From 21639b2d179e84805d5ab0949c7bdd74c9e2b7ae Mon Sep 17 00:00:00 2001 From: regnat Date: Wed, 23 Sep 2020 16:05:47 +0200 Subject: [PATCH 06/42] Use gold as the linker on Linux Saves ~7s in the linking phase --- flake.nix | 1 + 1 file changed, 1 insertion(+) diff --git a/flake.nix b/flake.nix index 0304557e8355..200417c3e7bb 100644 --- a/flake.nix +++ b/flake.nix @@ -58,6 +58,7 @@ configureFlags = lib.optionals stdenv.isLinux [ "--with-sandbox-shell=${sh}/bin/busybox" + "LDFLAGS=-fuse-ld=gold" ]; buildDeps = From e8f0b1e996e95e4a1025976d6cfb7ece734129a2 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 23 Sep 2020 15:34:11 +0200 Subject: [PATCH 07/42] DerivationGoal::registerOutputs(): Fix bad format string --- src/libstore/build.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 07c5bceb2327..2e23dd979e22 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -3858,7 +3858,7 @@ void DerivationGoal::registerOutputs() something like that. */ canonicalisePathMetaData(actualPath, buildUser ? buildUser->getUID() : -1, inodesSeen); - debug("scanning for references for output %1 in temp location '%1%'", outputName, actualPath); + debug("scanning for references for output '%s' in temp location '%s'", outputName, actualPath); /* Pass blank Sink as we are not ready to hash data at this stage. */ NullSink blank; From d4f8163d104aee0f39e6d8b98160f8de12c18824 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 23 Sep 2020 15:47:12 +0200 Subject: [PATCH 08/42] canonicalisePathMetaData_(): Change assertion to error message --- src/libstore/local-store.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index c91f3fbf70cf..1f58977a5939 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -478,8 +478,7 @@ static void canonicalisePathMetaData_(const Path & path, uid_t fromUid, InodesSe ensure that we don't fail on hard links within the same build (i.e. "touch $out/foo; ln $out/foo $out/bar"). */ if (fromUid != (uid_t) -1 && st.st_uid != fromUid) { - assert(!S_ISDIR(st.st_mode)); - if (inodesSeen.find(Inode(st.st_dev, st.st_ino)) == inodesSeen.end()) + if (S_ISDIR(st.st_mode) || !inodesSeen.count(Inode(st.st_dev, st.st_ino))) throw BuildError("invalid ownership on file '%1%'", path); mode_t mode = st.st_mode & ~S_IFMT; assert(S_ISLNK(st.st_mode) || (st.st_uid == geteuid() && (mode == 0444 || mode == 0555) && st.st_mtime == mtimeStore)); From cec94738715275ec4761071cefe4a9ae1a565960 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 23 Sep 2020 15:47:30 +0200 Subject: [PATCH 09/42] DerivationGoal::registerOutputs(): Don't canonicalize twice Fixes #4021. --- src/libstore/build.cc | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 2e23dd979e22..2979ce010f48 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -3913,7 +3913,6 @@ void DerivationGoal::registerOutputs() outputRewrites[std::string { scratchPath.hashPart() }] = std::string { finalStorePath.hashPart() }; }; - bool rewritten = false; std::optional referencesOpt = std::visit(overloaded { [&](AlreadyRegistered skippedFinalPath) -> std::optional { finish(skippedFinalPath.path); @@ -3943,8 +3942,6 @@ void DerivationGoal::registerOutputs() sink.s = make_ref(rewriteStrings(*sink.s, outputRewrites)); StringSource source(*sink.s); restorePath(actualPath, source); - - rewritten = true; } }; @@ -4125,11 +4122,6 @@ void DerivationGoal::registerOutputs() } } - /* Get rid of all weird permissions. This also checks that - all files are owned by the build user, if applicable. */ - canonicalisePathMetaData(actualPath, - buildUser && !rewritten ? buildUser->getUID() : -1, inodesSeen); - if (buildMode == bmCheck) { if (!worker.store.isValidPath(newInfo.path)) continue; ValidPathInfo oldInfo(*worker.store.queryPathInfo(newInfo.path)); From 2548347bba2d4e6f9947dadf95ed24a16f8a1cdc Mon Sep 17 00:00:00 2001 From: Dominique Martinet Date: Wed, 23 Sep 2020 17:39:19 +0200 Subject: [PATCH 10/42] libutil/archive: add preallocate-contents option Make archive preallocation (fallocate) optional because some filesystems like btrfs do not behave as expected with fallocate. See #3550. --- src/libutil/archive.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libutil/archive.cc b/src/libutil/archive.cc index 14399dea30c8..cdf3d89ae6dd 100644 --- a/src/libutil/archive.cc +++ b/src/libutil/archive.cc @@ -27,6 +27,8 @@ struct ArchiveSettings : Config #endif "use-case-hack", "Whether to enable a Darwin-specific hack for dealing with file name collisions."}; + Setting preallocateContents{this, true, "preallocate-contents", + "Whether to preallocate files when writing objects with known size."}; }; static ArchiveSettings archiveSettings; @@ -325,6 +327,9 @@ struct RestoreSink : ParseSink void preallocateContents(uint64_t len) { + if (!archiveSettings.preallocateContents) + return; + #if HAVE_POSIX_FALLOCATE if (len) { errno = posix_fallocate(fd.get(), 0, len); From 31ab4c3816675b5bf2f7b29dfb10112cd8ec9ceb Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 23 Sep 2020 19:09:58 +0200 Subject: [PATCH 11/42] Test whether build/repair results are read-only --- tests/repair.sh | 13 +++++-------- tests/simple.sh | 4 +++- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/tests/repair.sh b/tests/repair.sh index ec7ad5dcaff4..ba019028dc9a 100644 --- a/tests/repair.sh +++ b/tests/repair.sh @@ -13,14 +13,14 @@ hash=$(nix-hash $path2) chmod u+w $path2 touch $path2/bad -if nix-store --verify --check-contents -v; then - echo "nix-store --verify succeeded unexpectedly" >&2 - exit 1 -fi +(! nix-store --verify --check-contents -v) # The path can be repaired by rebuilding the derivation. nix-store --verify --check-contents --repair +(! [ -e $path2/bad ]) +(! [ -w $path2 ]) + nix-store --verify-path $path2 # Re-corrupt and delete the deriver. Now --verify --repair should @@ -30,10 +30,7 @@ touch $path2/bad nix-store --delete $(nix-store -qd $path2) -if nix-store --verify --check-contents --repair; then - echo "nix-store --verify --repair succeeded unexpectedly" >&2 - exit 1 -fi +(! nix-store --verify --check-contents --repair) nix-build dependencies.nix -o $TEST_ROOT/result --repair diff --git a/tests/simple.sh b/tests/simple.sh index 37631b648c67..15bd2bd163ee 100644 --- a/tests/simple.sh +++ b/tests/simple.sh @@ -10,13 +10,15 @@ outPath=$(nix-store -rvv "$drvPath") echo "output path is $outPath" +(! [ -w $outPath ]) + text=$(cat "$outPath"/hello) if test "$text" != "Hello World!"; then exit 1; fi # Directed delete: $outPath is not reachable from a root, so it should # be deleteable. nix-store --delete $outPath -if test -e $outPath/hello; then false; fi +(! [ -e $outPath/hello ]) outPath="$(NIX_REMOTE=local?store=/foo\&real=$TEST_ROOT/real-store nix-instantiate --readonly-mode hash-check.nix)" if test "$outPath" != "/foo/lfy1s6ca46rm5r6w4gg9hc0axiakjcnm-dependencies.drv"; then From 688bd4fb500c70cda4ffe6864bbf59dbc4da49a2 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 23 Sep 2020 19:10:16 +0200 Subject: [PATCH 12/42] After rewriting a path, make it read-only --- src/libstore/build.cc | 47 ++++++++++++++----------------------------- 1 file changed, 15 insertions(+), 32 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 2979ce010f48..cf04fbe56786 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -3735,15 +3735,12 @@ void DerivationGoal::runChild() } -static void moveCheckToStore(const Path & src, const Path & dst) +/* Move/rename path 'src' to 'dst'. Temporarily make 'src' writable if + it's a directory and we're not root (to be able to update the + directory's parent link ".."). */ +static void movePath(const Path & src, const Path & dst) { - /* For the rename of directory to succeed, we must be running as root or - the directory must be made temporarily writable (to update the - directory's parent link ".."). */ - struct stat st; - if (lstat(src.c_str(), &st) == -1) { - throw SysError("getting attributes of path '%1%'", src); - } + auto st = lstat(src); bool changePerm = (geteuid() && S_ISDIR(st.st_mode) && !(st.st_mode & S_IWUSR)); @@ -3942,6 +3939,10 @@ void DerivationGoal::registerOutputs() sink.s = make_ref(rewriteStrings(*sink.s, outputRewrites)); StringSource source(*sink.s); restorePath(actualPath, source); + + /* FIXME: set proper permissions in restorePath() so + we don't have to do another traversal. */ + canonicalisePathMetaData(actualPath, -1, inodesSeen); } }; @@ -4024,7 +4025,7 @@ void DerivationGoal::registerOutputs() [&](DerivationOutputInputAddressed output) { /* input-addressed case */ auto requiredFinalPath = output.path; - /* Preemtively add rewrite rule for final hash, as that is + /* Preemptively add rewrite rule for final hash, as that is what the NAR hash will use rather than normalized-self references */ if (scratchPath != requiredFinalPath) outputRewrites.insert_or_assign( @@ -4098,27 +4099,9 @@ void DerivationGoal::registerOutputs() else. No moving needed. */ assert(newInfo.ca); } else { - /* Temporarily add write perm so we can move, will be fixed - later. */ - { - struct stat st; - auto & mode = st.st_mode; - if (lstat(actualPath.c_str(), &st)) - throw SysError("getting attributes of path '%1%'", actualPath); - mode |= 0200; - /* Try to change the perms, but only if the file isn't a - symlink as symlinks permissions are mostly ignored and - calling `chmod` on it will just forward the call to the - target of the link. */ - if (!S_ISLNK(st.st_mode)) - if (chmod(actualPath.c_str(), mode) == -1) - throw SysError("changing mode of '%1%' to %2$o", actualPath, mode); - } - if (rename( - actualPath.c_str(), - worker.store.toRealPath(finalDestPath).c_str()) == -1) - throw SysError("moving build output '%1%' from it's temporary location to the Nix store", finalDestPath); - actualPath = worker.store.toRealPath(finalDestPath); + auto destPath = worker.store.toRealPath(finalDestPath); + movePath(actualPath, destPath); + actualPath = destPath; } } @@ -4128,9 +4111,9 @@ void DerivationGoal::registerOutputs() if (newInfo.narHash != oldInfo.narHash) { worker.checkMismatch = true; if (settings.runDiffHook || settings.keepFailed) { - Path dst = worker.store.toRealPath(finalDestPath + checkSuffix); + auto dst = worker.store.toRealPath(finalDestPath + checkSuffix); deletePath(dst); - moveCheckToStore(actualPath, dst); + movePath(actualPath, dst); handleDiffHook( buildUser ? buildUser->getUID() : getuid(), From 236d9ee7f72ca4238f5f44c244fd2b885691c6ad Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 23 Sep 2020 19:17:28 +0200 Subject: [PATCH 13/42] lstat() cleanup --- src/libexpr/parser.y | 3 +-- src/libstore/build.cc | 9 ++------- src/libstore/gc.cc | 4 +--- src/libstore/local-store.cc | 18 +++++------------- src/libstore/optimise-store.cc | 12 +++--------- src/libstore/profiles.cc | 5 +---- src/libutil/archive.cc | 4 +--- .../resolve-system-dependencies.cc | 6 +----- 8 files changed, 15 insertions(+), 46 deletions(-) diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 24b21f7da332..a4c84c52651d 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -614,8 +614,7 @@ Path resolveExprPath(Path path) // Basic cycle/depth limit to avoid infinite loops. if (++followCount >= maxFollow) throw Error("too many symbolic links encountered while traversing the path '%s'", path); - if (lstat(path.c_str(), &st)) - throw SysError("getting status of '%s'", path); + st = lstat(path); if (!S_ISLNK(st.st_mode)) break; path = absPath(readLink(path), dirOf(path)); } diff --git a/src/libstore/build.cc b/src/libstore/build.cc index cf04fbe56786..6b53f529a501 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2367,10 +2367,7 @@ void DerivationGoal::startBuilder() for (auto & i : inputPaths) { auto p = worker.store.printStorePath(i); Path r = worker.store.toRealPath(p); - struct stat st; - if (lstat(r.c_str(), &st)) - throw SysError("getting attributes of path '%s'", p); - if (S_ISDIR(st.st_mode)) + if (S_ISDIR(lstat(r).st_mode)) dirsInChroot.insert_or_assign(p, r); else linkOrCopy(r, chrootRootDir + p); @@ -3144,9 +3141,7 @@ void DerivationGoal::addDependency(const StorePath & path) if (pathExists(target)) throw Error("store path '%s' already exists in the sandbox", worker.store.printStorePath(path)); - struct stat st; - if (lstat(source.c_str(), &st)) - throw SysError("getting attributes of path '%s'", source); + auto st = lstat(source); if (S_ISDIR(st.st_mode)) { diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 08b53c702f74..518a357ef783 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -663,9 +663,7 @@ void LocalStore::removeUnusedLinks(const GCState & state) if (name == "." || name == "..") continue; Path path = linksDir + "/" + name; - struct stat st; - if (lstat(path.c_str(), &st) == -1) - throw SysError("statting '%1%'", path); + auto st = lstat(path); if (st.st_nlink != 1) { actualSize += st.st_size; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 1f58977a5939..ee997ef3abb1 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -114,8 +114,7 @@ LocalStore::LocalStore(const Params & params) Path path = realStoreDir; struct stat st; while (path != "/") { - if (lstat(path.c_str(), &st)) - throw SysError("getting status of '%1%'", path); + st = lstat(path); if (S_ISLNK(st.st_mode)) throw Error( "the path '%1%' is a symlink; " @@ -419,10 +418,7 @@ static void canonicaliseTimestampAndPermissions(const Path & path, const struct void canonicaliseTimestampAndPermissions(const Path & path) { - struct stat st; - if (lstat(path.c_str(), &st)) - throw SysError("getting attributes of path '%1%'", path); - canonicaliseTimestampAndPermissions(path, st); + canonicaliseTimestampAndPermissions(path, lstat(path)); } @@ -440,9 +436,7 @@ static void canonicalisePathMetaData_(const Path & path, uid_t fromUid, InodesSe } #endif - struct stat st; - if (lstat(path.c_str(), &st)) - throw SysError("getting attributes of path '%1%'", path); + auto st = lstat(path); /* Really make sure that the path is of a supported type. */ if (!(S_ISREG(st.st_mode) || S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))) @@ -521,9 +515,7 @@ void canonicalisePathMetaData(const Path & path, uid_t fromUid, InodesSeen & ino /* On platforms that don't have lchown(), the top-level path can't be a symlink, since we can't change its ownership. */ - struct stat st; - if (lstat(path.c_str(), &st)) - throw SysError("getting attributes of path '%1%'", path); + auto st = lstat(path); if (st.st_uid != geteuid()) { assert(S_ISLNK(st.st_mode)); @@ -1454,7 +1446,7 @@ static void makeMutable(const Path & path) { checkInterrupt(); - struct stat st = lstat(path); + auto st = lstat(path); if (!S_ISDIR(st.st_mode) && !S_ISREG(st.st_mode)) return; diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index e4b4b621370d..c032a5e2229b 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -17,9 +17,7 @@ namespace nix { static void makeWritable(const Path & path) { - struct stat st; - if (lstat(path.c_str(), &st)) - throw SysError("getting attributes of path '%1%'", path); + auto st = lstat(path); if (chmod(path.c_str(), st.st_mode | S_IWUSR) == -1) throw SysError("changing writability of '%1%'", path); } @@ -94,9 +92,7 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, { checkInterrupt(); - struct stat st; - if (lstat(path.c_str(), &st)) - throw SysError("getting attributes of path '%1%'", path); + auto st = lstat(path); #if __APPLE__ /* HFS/macOS has some undocumented security feature disabling hardlinking for @@ -187,9 +183,7 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, /* Yes! We've seen a file with the same contents. Replace the current file with a hard link to that file. */ - struct stat stLink; - if (lstat(linkPath.c_str(), &stLink)) - throw SysError("getting attributes of path '%1%'", linkPath); + auto stLink = lstat(linkPath); if (st.st_ino == stLink.st_ino) { debug(format("'%1%' is already linked to '%2%'") % path % linkPath); diff --git a/src/libstore/profiles.cc b/src/libstore/profiles.cc index c20386e2bab2..c3809bad797f 100644 --- a/src/libstore/profiles.cc +++ b/src/libstore/profiles.cc @@ -39,13 +39,10 @@ std::pair> findGenerations(Path pro for (auto & i : readDirectory(profileDir)) { if (auto n = parseName(profileName, i.name)) { auto path = profileDir + "/" + i.name; - struct stat st; - if (lstat(path.c_str(), &st) != 0) - throw SysError("statting '%1%'", path); gens.push_back({ .number = *n, .path = path, - .creationTime = st.st_mtime + .creationTime = lstat(path).st_mtime }); } } diff --git a/src/libutil/archive.cc b/src/libutil/archive.cc index 14399dea30c8..4f59c8ed52ac 100644 --- a/src/libutil/archive.cc +++ b/src/libutil/archive.cc @@ -66,9 +66,7 @@ static void dump(const Path & path, Sink & sink, PathFilter & filter) { checkInterrupt(); - struct stat st; - if (lstat(path.c_str(), &st)) - throw SysError("getting attributes of path '%1%'", path); + auto st = lstat(path); sink << "("; diff --git a/src/resolve-system-dependencies/resolve-system-dependencies.cc b/src/resolve-system-dependencies/resolve-system-dependencies.cc index 434ad80a6628..d30227e4ee7a 100644 --- a/src/resolve-system-dependencies/resolve-system-dependencies.cc +++ b/src/resolve-system-dependencies/resolve-system-dependencies.cc @@ -111,11 +111,7 @@ std::set runResolver(const Path & filename) bool isSymlink(const Path & path) { - struct stat st; - if (lstat(path.c_str(), &st) == -1) - throw SysError("getting attributes of path '%1%'", path); - - return S_ISLNK(st.st_mode); + return S_ISLNK(lstat(path).st_mode); } Path resolveSymlink(const Path & path) From 9a24ece122eb19f3b69f072f6ce3c39c5ae4d0ce Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 23 Sep 2020 20:21:08 +0200 Subject: [PATCH 14/42] Fix exception --- src/libstore/build.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 6b53f529a501..f0820e711dbb 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1675,7 +1675,7 @@ void DerivationGoal::tryLocalBuild() { } -void replaceValidPath(const Path & storePath, const Path tmpPath) +void replaceValidPath(const Path & storePath, const Path & tmpPath) { /* We can't atomically replace storePath (the original) with tmpPath (the replacement), so we have to move it out of the @@ -1685,8 +1685,9 @@ void replaceValidPath(const Path & storePath, const Path tmpPath) if (pathExists(storePath)) rename(storePath.c_str(), oldPath.c_str()); if (rename(tmpPath.c_str(), storePath.c_str()) == -1) { + auto ex = SysError("moving '%s' to '%s'", tmpPath, storePath); rename(oldPath.c_str(), storePath.c_str()); // attempt to recover - throw SysError("moving '%s' to '%s'", tmpPath, storePath); + throw ex; } deletePath(oldPath); } From 4ce8a3ed452f06b18a40cffefc37d47c916927a8 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 23 Sep 2020 21:29:10 +0200 Subject: [PATCH 15/42] Hopefully fix EPERM on macOS --- src/libstore/build.cc | 72 ++++++++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 32 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index f0820e711dbb..db7dbc17ee82 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1675,6 +1675,33 @@ void DerivationGoal::tryLocalBuild() { } +static void chmod_(const Path & path, mode_t mode) +{ + if (chmod(path.c_str(), mode) == -1) + throw SysError("setting permissions on '%s'", path); +} + + +/* Move/rename path 'src' to 'dst'. Temporarily make 'src' writable if + it's a directory and we're not root (to be able to update the + directory's parent link ".."). */ +static void movePath(const Path & src, const Path & dst) +{ + auto st = lstat(src); + + bool changePerm = (geteuid() && S_ISDIR(st.st_mode) && !(st.st_mode & S_IWUSR)); + + if (changePerm) + chmod_(src, st.st_mode | S_IWUSR); + + if (rename(src.c_str(), dst.c_str())) + throw SysError("renaming '%1%' to '%2%'", src, dst); + + if (changePerm) + chmod_(dst, st.st_mode); +} + + void replaceValidPath(const Path & storePath, const Path & tmpPath) { /* We can't atomically replace storePath (the original) with @@ -1683,12 +1710,20 @@ void replaceValidPath(const Path & storePath, const Path & tmpPath) we're repairing (say) Glibc, we end up with a broken system. */ Path oldPath = (format("%1%.old-%2%-%3%") % storePath % getpid() % random()).str(); if (pathExists(storePath)) - rename(storePath.c_str(), oldPath.c_str()); - if (rename(tmpPath.c_str(), storePath.c_str()) == -1) { - auto ex = SysError("moving '%s' to '%s'", tmpPath, storePath); - rename(oldPath.c_str(), storePath.c_str()); // attempt to recover - throw ex; + movePath(storePath, oldPath); + + try { + movePath(tmpPath, storePath); + } catch (...) { + try { + // attempt to recover + movePath(oldPath, storePath); + } catch (...) { + ignoreException(); + } + throw; } + deletePath(oldPath); } @@ -2006,13 +2041,6 @@ HookReply DerivationGoal::tryBuildHook() } -static void chmod_(const Path & path, mode_t mode) -{ - if (chmod(path.c_str(), mode) == -1) - throw SysError("setting permissions on '%s'", path); -} - - int childEntry(void * arg) { ((DerivationGoal *) arg)->runChild(); @@ -3731,26 +3759,6 @@ void DerivationGoal::runChild() } -/* Move/rename path 'src' to 'dst'. Temporarily make 'src' writable if - it's a directory and we're not root (to be able to update the - directory's parent link ".."). */ -static void movePath(const Path & src, const Path & dst) -{ - auto st = lstat(src); - - bool changePerm = (geteuid() && S_ISDIR(st.st_mode) && !(st.st_mode & S_IWUSR)); - - if (changePerm) - chmod_(src, st.st_mode | S_IWUSR); - - if (rename(src.c_str(), dst.c_str())) - throw SysError("renaming '%1%' to '%2%'", src, dst); - - if (changePerm) - chmod_(dst, st.st_mode); -} - - void DerivationGoal::registerOutputs() { /* When using a build hook, the build hook can register the output From bd5f3dbe118d569ffb201ce14394572ac5fc412c Mon Sep 17 00:00:00 2001 From: Kevin Quick Date: Thu, 24 Sep 2020 12:30:03 -0700 Subject: [PATCH 16/42] Fixes fall-through to report correct description of hash-file command. --- src/nix/hash.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/nix/hash.cc b/src/nix/hash.cc index 0eca4f8eaeb0..494f00a20b49 100644 --- a/src/nix/hash.cc +++ b/src/nix/hash.cc @@ -44,6 +44,7 @@ struct CmdHash : Command switch (mode) { case FileIngestionMethod::Flat: d = "print cryptographic hash of a regular file"; + break; case FileIngestionMethod::Recursive: d = "print cryptographic hash of the NAR serialisation of a path"; }; From ed218e1d6cf755fc3011c0954eb7031f95d3d732 Mon Sep 17 00:00:00 2001 From: Alexander Bantyev Date: Fri, 25 Sep 2020 00:07:42 +0300 Subject: [PATCH 17/42] Fix max-jobs option After 0ed946aa616bbf7ffe7f90d3309abdd27d875b10, max-jobs setting (-j/--max-jobs) stopped working. The reason was that nrLocalBuilds (which compared to maxBuildJobs to figure out whether the limit is reached or not) is not incremented yet when tryBuild is started; So, the solution is to move the check to tryLocalBuild. Closes https://github.com/nixos/nix/issues/3763 --- src/libstore/build.cc | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index db7dbc17ee82..3fc24b2218c2 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1612,6 +1612,13 @@ void DerivationGoal::tryToBuild() actLock.reset(); + state = &DerivationGoal::tryLocalBuild; + worker.wakeUp(shared_from_this()); +} + +void DerivationGoal::tryLocalBuild() { + bool buildLocally = buildMode != bmNormal || parsedDrv->willBuildLocally(worker.store); + /* Make sure that we are allowed to start a build. If this derivation prefers to be done locally, do it even if maxBuildJobs is 0. */ @@ -1622,12 +1629,6 @@ void DerivationGoal::tryToBuild() return; } - state = &DerivationGoal::tryLocalBuild; - worker.wakeUp(shared_from_this()); -} - -void DerivationGoal::tryLocalBuild() { - /* If `build-users-group' is not empty, then we have to build as one of the members of that group. */ if (settings.buildUsersGroup != "" && getuid() == 0) { From 4d863a9fcb9460a9e4978466e03d2982d32e39f0 Mon Sep 17 00:00:00 2001 From: Paul Opiyo <59094545+paulopiyo777@users.noreply.github.com> Date: Thu, 24 Sep 2020 18:05:47 -0500 Subject: [PATCH 18/42] Remove redundant value checks std::optional had redundant checks for whether it had a value. An object is emplaced either way so it can be dereferenced without repeating a value check --- src/libexpr/flake/flake.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index 460eea5eaf6b..760ed1a6e432 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -48,17 +48,17 @@ static std::tuple fetchOrSubstituteTree( resolvedRef = originalRef.resolve(state.store); auto fetchedResolved = lookupInFlakeCache(flakeCache, originalRef); if (!fetchedResolved) fetchedResolved.emplace(resolvedRef.fetchTree(state.store)); - flakeCache.push_back({resolvedRef, fetchedResolved.value()}); - fetched.emplace(fetchedResolved.value()); + flakeCache.push_back({resolvedRef, *fetchedResolved}); + fetched.emplace(*fetchedResolved); } else { throw Error("'%s' is an indirect flake reference, but registry lookups are not allowed", originalRef); } } - flakeCache.push_back({originalRef, fetched.value()}); + flakeCache.push_back({originalRef, *fetched}); } - auto [tree, lockedRef] = fetched.value(); + auto [tree, lockedRef] = *fetched; debug("got tree '%s' from '%s'", state.store->printStorePath(tree.storePath), lockedRef); From 83fec38fc93922192ada7c0409fec76578ef8dfb Mon Sep 17 00:00:00 2001 From: Kevin Quick Date: Thu, 24 Sep 2020 22:41:24 -0700 Subject: [PATCH 19/42] Update document generation for empty json object values. --- doc/manual/generate-options.nix | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/doc/manual/generate-options.nix b/doc/manual/generate-options.nix index 7afe279c3738..3c31a4eec37d 100644 --- a/doc/manual/generate-options.nix +++ b/doc/manual/generate-options.nix @@ -13,7 +13,12 @@ concatStrings (map then "*empty*" else if isBool option.value then (if option.value then "`true`" else "`false`") - else "`" + toString option.value + "`") + "\n\n" + else + # n.b. a StringMap value type is specified as a string, but + # this shows the value type. The empty stringmap is "null" in + # JSON, but that converts to "{ }" here. + (if isAttrs option.value then "`\"\"`" + else "`" + toString option.value + "`")) + "\n\n" + (if option.aliases != [] then " **Deprecated alias:** " + (concatStringsSep ", " (map (s: "`${s}`") option.aliases)) + "\n\n" else "") From a439e9488df6c13d0e44dd4816df98487d69f4c6 Mon Sep 17 00:00:00 2001 From: Kevin Quick Date: Thu, 24 Sep 2020 22:42:59 -0700 Subject: [PATCH 20/42] Support StringMap configuration settings. Allows Configuration values that are space-separated key=value pairs. --- src/libutil/config.cc | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/libutil/config.cc b/src/libutil/config.cc index 309d23b40a5d..b14feb10dbb8 100644 --- a/src/libutil/config.cc +++ b/src/libutil/config.cc @@ -268,6 +268,26 @@ template<> std::string BaseSetting::to_string() const return concatStringsSep(" ", value); } +template<> void BaseSetting::set(const std::string & str) +{ + auto kvpairs = tokenizeString(str); + for (auto & s : kvpairs) + { + auto eq = s.find_first_of('='); + if (std::string::npos != eq) + value.emplace(std::string(s, 0, eq), std::string(s, eq + 1)); + // else ignored + } +} + +template<> std::string BaseSetting::to_string() const +{ + Strings kvstrs; + std::transform(value.begin(), value.end(), back_inserter(kvstrs), + [&](auto kvpair){ return kvpair.first + "=" + kvpair.second; }); + return concatStringsSep(" ", kvstrs); +} + template class BaseSetting; template class BaseSetting; template class BaseSetting; @@ -278,6 +298,7 @@ template class BaseSetting; template class BaseSetting; template class BaseSetting; template class BaseSetting; +template class BaseSetting; void PathSetting::set(const std::string & str) { From c2f48cfcee501dd15690245d481d154444456f66 Mon Sep 17 00:00:00 2001 From: Kevin Quick Date: Thu, 24 Sep 2020 22:46:03 -0700 Subject: [PATCH 21/42] Complete conversion of "url" to "host" with associated variable renaming. Completes the change begun in commit 56f1e0d to consistently use the "host" attribute for "github" and "gitlab" inputs instead of a "url" attribute. --- src/libfetchers/github.cc | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index d8d0351b9c2c..eb758bc5fc0b 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -65,9 +65,9 @@ struct GitArchiveInputScheme : InputScheme throw BadURL("URL '%s' contains multiple branch/tag names", url.url); ref = value; } - else if (name == "url") { + else if (name == "host") { if (!std::regex_match(value, urlRegex)) - throw BadURL("URL '%s' contains an invalid instance url", url.url); + throw BadURL("URL '%s' contains an invalid instance host", url.url); host_url = value; } // FIXME: barf on unsupported attributes @@ -82,7 +82,7 @@ struct GitArchiveInputScheme : InputScheme 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("url", *host_url); + if (host_url) input.attrs.insert_or_assign("host", *host_url); return input; } @@ -92,7 +92,7 @@ struct GitArchiveInputScheme : InputScheme if (maybeGetStrAttr(attrs, "type") != type()) return {}; for (auto & [name, value] : attrs) - if (name != "type" && name != "owner" && name != "repo" && name != "ref" && name != "rev" && name != "narHash" && name != "lastModified") + if (name != "type" && name != "owner" && name != "repo" && name != "ref" && name != "rev" && name != "narHash" && name != "lastModified" && name != "host") throw Error("unsupported input attribute '%s'", name); getStrAttr(attrs, "owner"); @@ -208,9 +208,9 @@ struct GitHubInputScheme : GitArchiveInputScheme Hash getRevFromRef(nix::ref store, const Input & input) const override { - auto host_url = maybeGetStrAttr(input.attrs, "url").value_or("github.com"); + auto host = maybeGetStrAttr(input.attrs, "host").value_or("github.com"); auto url = fmt("https://api.%s/repos/%s/%s/commits/%s", // FIXME: check - host_url, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"), *input.getRef()); + host, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"), *input.getRef()); Headers headers; std::string accessToken = settings.githubAccessToken.get(); @@ -230,9 +230,9 @@ struct GitHubInputScheme : GitArchiveInputScheme { // FIXME: use regular /archive URLs instead? api.github.com // might have stricter rate limits. - auto host_url = maybeGetStrAttr(input.attrs, "url").value_or("github.com"); + auto host = maybeGetStrAttr(input.attrs, "host").value_or("github.com"); auto url = fmt("https://api.%s/repos/%s/%s/tarball/%s", // FIXME: check if this is correct for self hosted instances - host_url, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"), + host, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"), input.getRev()->to_string(Base16, false)); std::string accessToken = settings.githubAccessToken.get(); @@ -246,9 +246,9 @@ struct GitHubInputScheme : GitArchiveInputScheme void clone(const Input & input, const Path & destDir) override { - auto host_url = maybeGetStrAttr(input.attrs, "url").value_or("github.com"); + auto host = maybeGetStrAttr(input.attrs, "host").value_or("github.com"); Input::fromURL(fmt("git+ssh://git@%s/%s/%s.git", - host_url, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"))) + host, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"))) .applyOverrides(input.getRef().value_or("HEAD"), input.getRev()) .clone(destDir); } @@ -284,10 +284,14 @@ struct GitLabInputScheme : GitArchiveInputScheme DownloadUrl getDownloadUrl(const Input & input) const override { - // FIXME: This endpoint has a rate limit threshold of 5 requests per minute - auto host_url = maybeGetStrAttr(input.attrs, "url").value_or("gitlab.com"); + // This endpoint has a rate limit threshold that may be + // server-specific and vary based whether the user is + // authenticated via an accessToken or not, but the usual rate + // is 10 reqs/sec/ip-addr. See + // https://docs.gitlab.com/ee/user/gitlab_com/index.html#gitlabcom-specific-rate-limits + auto host = maybeGetStrAttr(input.attrs, "host").value_or("gitlab.com"); auto url = fmt("https://%s/api/v4/projects/%s%%2F%s/repository/archive.tar.gz?sha=%s", - host_url, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"), + host, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"), input.getRev()->to_string(Base16, false)); std::string accessToken = settings.gitlabAccessToken.get(); @@ -302,10 +306,10 @@ struct GitLabInputScheme : GitArchiveInputScheme void clone(const Input & input, const Path & destDir) override { - auto host_url = maybeGetStrAttr(input.attrs, "url").value_or("gitlab.com"); + auto host = maybeGetStrAttr(input.attrs, "host").value_or("gitlab.com"); // FIXME: get username somewhere Input::fromURL(fmt("git+ssh://git@%s/%s/%s.git", - host_url, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"))) + host, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"))) .applyOverrides(input.getRef().value_or("HEAD"), input.getRev()) .clone(destDir); } From 8fba2a8b54283ea1cf56ae75faf4ced5f3e8e4a1 Mon Sep 17 00:00:00 2001 From: Kevin Quick Date: Thu, 24 Sep 2020 22:49:44 -0700 Subject: [PATCH 22/42] Update to use access-tokens configuration for github/gitlab access. This change provides support for using access tokens with other instances of GitHub and GitLab beyond just github.com and gitlab.com (especially company-specific or foundation-specific instances). This change also provides the ability to specify the type of access token being used, where different types may have different handling, based on the forge type. --- src/libfetchers/github.cc | 100 ++++++++++++++++++++++---------------- src/libstore/globals.hh | 50 ++++++++++++++++++- 2 files changed, 107 insertions(+), 43 deletions(-) diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index eb758bc5fc0b..0e0655367ff7 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -5,6 +5,7 @@ #include "store-api.hh" #include "types.hh" +#include #include namespace nix::fetchers { @@ -12,13 +13,10 @@ namespace nix::fetchers { struct DownloadUrl { std::string url; - std::optional> access_token_header; + Headers headers; - DownloadUrl(const std::string & url) - : url(url) { } - - DownloadUrl(const std::string & url, const std::pair & access_token_header) - : url(url), access_token_header(access_token_header) { } + DownloadUrl(const std::string & url, const Headers & headers) + : url(url), headers(headers) { } }; // A github or gitlab url @@ -29,7 +27,7 @@ struct GitArchiveInputScheme : InputScheme { virtual std::string type() = 0; - virtual std::pair accessHeaderFromToken(const std::string & token) const = 0; + virtual std::optional > accessHeaderFromToken(const std::string & token) const = 0; std::optional inputFromURL(const ParsedURL & url) override { @@ -144,6 +142,27 @@ struct GitArchiveInputScheme : InputScheme return input; } + std::optional getAccessToken(const std::string &host) const { + auto tokens = settings.accessTokens.get(); + auto pat = tokens.find(host); + if (pat == tokens.end()) + return std::nullopt; + return pat->second; + } + + Headers makeHeadersWithAuthTokens(const std::string & host) const { + Headers headers; + auto accessToken = getAccessToken(host); + if (accessToken) { + auto hdr = accessHeaderFromToken(*accessToken); + if (hdr) + headers.push_back(*hdr); + else + warn("Unrecognized access token for host '%s'", host); + } + return headers; + } + virtual Hash getRevFromRef(nix::ref store, const Input & input) const = 0; virtual DownloadUrl getDownloadUrl(const Input & input) const = 0; @@ -175,12 +194,7 @@ struct GitArchiveInputScheme : InputScheme auto url = getDownloadUrl(input); - Headers headers; - if (url.access_token_header) { - headers.push_back(*url.access_token_header); - } - - auto [tree, lastModified] = downloadTarball(store, url.url, headers, "source", true); + auto [tree, lastModified] = downloadTarball(store, url.url, url.headers, "source", true); input.attrs.insert_or_assign("lastModified", lastModified); @@ -202,7 +216,13 @@ struct GitHubInputScheme : GitArchiveInputScheme { std::string type() override { return "github"; } - std::pair accessHeaderFromToken(const std::string & token) const { + std::optional > accessHeaderFromToken(const std::string & token) const { + // Github supports PAT/OAuth2 tokens and HTTP Basic + // Authentication. The former simply specifies the token, the + // latter can use the token as the password. Only the first + // is used here. See + // https://developer.github.com/v3/#authentication and + // https://docs.github.com/en/developers/apps/authorizing-oath-apps return std::pair("Authorization", fmt("token %s", token)); } @@ -212,10 +232,7 @@ struct GitHubInputScheme : GitArchiveInputScheme auto url = fmt("https://api.%s/repos/%s/%s/commits/%s", // FIXME: check host, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"), *input.getRef()); - Headers headers; - std::string accessToken = settings.githubAccessToken.get(); - if (accessToken != "") - headers.push_back(accessHeaderFromToken(accessToken)); + Headers headers = makeHeadersWithAuthTokens(host); auto json = nlohmann::json::parse( readFile( @@ -235,13 +252,8 @@ struct GitHubInputScheme : GitArchiveInputScheme host, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"), input.getRev()->to_string(Base16, false)); - std::string accessToken = settings.githubAccessToken.get(); - if (accessToken != "") { - auto auth_header = accessHeaderFromToken(accessToken); - return DownloadUrl(url, auth_header); - } else { - return DownloadUrl(url); - } + Headers headers = makeHeadersWithAuthTokens(host); + return DownloadUrl(url, headers); } void clone(const Input & input, const Path & destDir) override @@ -258,20 +270,32 @@ struct GitLabInputScheme : GitArchiveInputScheme { std::string type() override { return "gitlab"; } - std::pair accessHeaderFromToken(const std::string & token) const { - return std::pair("Authorization", fmt("Bearer %s", token)); + std::optional > accessHeaderFromToken(const std::string & token) const { + // Gitlab supports 4 kinds of authorization, two of which are + // relevant here: OAuth2 and PAT (Private Access Token). The + // user can indicate which token is used by specifying the + // token as :, where type is "OAuth2" or "PAT". + // If the is unrecognized, this will fall back to + // treating this simply has :. See + // https://docs.gitlab.com/12.10/ee/api/README.html#authentication + auto fldsplit = token.find_first_of(':'); + // n.b. C++20 would allow: if (token.starts_with("OAuth2:")) ... + if ("OAuth2" == token.substr(0, fldsplit)) + return std::make_pair("Authorization", fmt("Bearer %s", token.substr(fldsplit+1))); + if ("PAT" == token.substr(0, fldsplit)) + return std::make_pair("Private-token", token.substr(fldsplit+1)); + warn("Unrecognized GitLab token type %s", token.substr(0, fldsplit)); + return std::nullopt; } Hash getRevFromRef(nix::ref store, const Input & input) const override { - auto host_url = maybeGetStrAttr(input.attrs, "url").value_or("gitlab.com"); + auto host = maybeGetStrAttr(input.attrs, "host").value_or("gitlab.com"); + // See rate limiting note below auto url = fmt("https://%s/api/v4/projects/%s%%2F%s/repository/commits?ref_name=%s", - host_url, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"), *input.getRef()); + host, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"), *input.getRef()); - Headers headers; - std::string accessToken = settings.gitlabAccessToken.get(); - if (accessToken != "") - headers.push_back(accessHeaderFromToken(accessToken)); + Headers headers = makeHeadersWithAuthTokens(host); auto json = nlohmann::json::parse( readFile( @@ -294,14 +318,8 @@ struct GitLabInputScheme : GitArchiveInputScheme host, getStrAttr(input.attrs, "owner"), getStrAttr(input.attrs, "repo"), input.getRev()->to_string(Base16, false)); - std::string accessToken = settings.gitlabAccessToken.get(); - if (accessToken != "") { - auto auth_header = accessHeaderFromToken(accessToken); - return DownloadUrl(url, auth_header); - } else { - return DownloadUrl(url); - } - + Headers headers = makeHeadersWithAuthTokens(host); + return DownloadUrl(url, headers); } void clone(const Input & input, const Path & destDir) override diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index b2e7610ee6d5..6464223995a1 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -863,8 +863,54 @@ public: Setting githubAccessToken{this, "", "github-access-token", "GitHub access token to get access to GitHub data through the GitHub API for `github:<..>` flakes."}; - Setting gitlabAccessToken{this, "", "gitlab-access-token", - "GitLab access token to get access to GitLab data through the GitLab API for gitlab:<..> flakes."}; + Setting accessTokens{this, {}, "access-tokens", + R"( + Access tokens used to access protected GitHub, GitLab, or + other locations requiring token-based authentication. + + Access tokens are specified as a string made up of + space-separated `host=token` values. The specific token + used is selected by matching the `host` portion against the + "host" specification of the input. The actual use of the + `token` value is determined by the type of resource being + accessed: + + * Github: the token value is the OAUTH-TOKEN string obtained + as the Personal Access Token from the Github server (see + https://docs.github.com/en/developers/apps/authorizing-oath-apps). + + * Gitlab: the token value is either the OAuth2 token or the + Personal Access Token (these are different types tokens + for gitlab, see + https://docs.gitlab.com/12.10/ee/api/README.html#authentication). + The `token` value should be `type:tokenstring` where + `type` is either `OAuth2` or `PAT` to indicate which type + of token is being specified. + + Example `~/.config/nix/nix.conf`: + + ``` + personal-access-tokens = "github.com=23ac...b289 gitlab.mycompany.com=PAT:A123Bp_Cd..EfG gitlab.com=OAuth2:1jklw3jk" + ``` + + Example `~/code/flake.nix`: + + ```nix + input.foo = { + type="gitlab"; + host="gitlab.mycompany.com"; + owner="mycompany"; + repo="pro"; + }; + ``` + + This example specifies three tokens, one each for accessing + github.com, gitlab.mycompany.com, and sourceforge.net. + + The `input.foo` uses the "gitlab" fetcher, which might + requires specifying the token type along with the token + value. + )"}; Setting experimentalFeatures{this, {}, "experimental-features", "Experimental Nix features to enable."}; From 7b2ae472ff05a39cd635ac10dbbce3cd17b60c93 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 25 Sep 2020 10:27:40 +0200 Subject: [PATCH 23/42] expectArg(): Respect the 'optional' flag --- src/libutil/args.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/args.hh b/src/libutil/args.hh index 3c1f87f7efe8..f41242e17795 100644 --- a/src/libutil/args.hh +++ b/src/libutil/args.hh @@ -192,7 +192,7 @@ public: { expectArgs({ .label = label, - .optional = true, + .optional = optional, .handler = {dest} }); } From ef2a14be190f7162e85e9bdd44dd45bd9ddfe391 Mon Sep 17 00:00:00 2001 From: Kevin Quick Date: Fri, 25 Sep 2020 08:08:27 -0700 Subject: [PATCH 24/42] Fix reference to older name for access-tokens config value. --- src/libstore/globals.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 6464223995a1..959ebe360cff 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -890,7 +890,7 @@ public: Example `~/.config/nix/nix.conf`: ``` - personal-access-tokens = "github.com=23ac...b289 gitlab.mycompany.com=PAT:A123Bp_Cd..EfG gitlab.com=OAuth2:1jklw3jk" + access-tokens = "github.com=23ac...b289 gitlab.mycompany.com=PAT:A123Bp_Cd..EfG gitlab.com=OAuth2:1jklw3jk" ``` Example `~/code/flake.nix`: From 5a35cc29bffc88b88f883dfcdd1bb251eab53ecd Mon Sep 17 00:00:00 2001 From: Kevin Quick Date: Fri, 25 Sep 2020 08:09:56 -0700 Subject: [PATCH 25/42] Re-add support for github-access-token, but mark as deprecated. --- src/libfetchers/github.cc | 10 ++++++++++ src/libstore/globals.hh | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index 0e0655367ff7..4436446396d7 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -146,7 +146,17 @@ struct GitArchiveInputScheme : InputScheme auto tokens = settings.accessTokens.get(); auto pat = tokens.find(host); if (pat == tokens.end()) + { + if ("github.com" == host) + { + auto oldcfg = settings.githubAccessToken.get(); + if (!oldcfg.empty()) { + warn("using deprecated 'github-access-token' config value; please use 'access-tokens' instead"); + return oldcfg; + } + } return std::nullopt; + } return pat->second; } diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 959ebe360cff..bd36ffc1733d 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -861,7 +861,7 @@ public: )"}; Setting githubAccessToken{this, "", "github-access-token", - "GitHub access token to get access to GitHub data through the GitHub API for `github:<..>` flakes."}; + "GitHub access token to get access to GitHub data through the GitHub API for `github:<..>` flakes (deprecated, please use 'access-tokens' instead)."}; Setting accessTokens{this, {}, "access-tokens", R"( From cb186f1e7536c9448455bfbf8dec16ad6600e88e Mon Sep 17 00:00:00 2001 From: Kevin Quick Date: Fri, 25 Sep 2020 09:36:18 -0700 Subject: [PATCH 26/42] Use "?dir=..." portion of "registry add" local path specification. The registry targets generally follow a URL formatting schema with support for a query parameter of "?dir=subpath" to specify a sub-path location below the URL root. Alternatively, an absolute path can be specified. This specification mode accepts the query parameter but ignores/drops it. It would probably be better to either (a) disallow the query parameter for the path form, or (b) recognize the query parameter and add to the path. This patch implements (b) for consistency, and to make it easier for tooling that might switch between a remote git reference and a local path reference. See also issue #4050. --- src/libexpr/flake/flakeref.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libexpr/flake/flakeref.cc b/src/libexpr/flake/flakeref.cc index d5c2ffe66c88..762e27e1ff40 100644 --- a/src/libexpr/flake/flakeref.cc +++ b/src/libexpr/flake/flakeref.cc @@ -157,7 +157,8 @@ std::pair parseFlakeRefWithFragment( } else { if (!hasPrefix(path, "/")) throw BadURL("flake reference '%s' is not an absolute path", url); - path = canonPath(path); + auto query = decodeQuery(match[2]); + path = canonPath(path + "/" + get(query, "dir").value_or("")); } fetchers::Attrs attrs; From 8b4a542d1767e0df7b3c0902b766f34352cb0958 Mon Sep 17 00:00:00 2001 From: Mateusz Piotrowski <0mp@FreeBSD.org> Date: Sat, 26 Sep 2020 13:33:04 +0200 Subject: [PATCH 27/42] Fix a typo (#4073) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 3cf4e44fadf3..11fe5f932342 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ for more details. ## Installation -On Linux and macOS the easiest way to Install Nix is to run the following shell command +On Linux and macOS the easiest way to install Nix is to run the following shell command (as a user other than root): ```console From a76fb07314d3c5dea06ac2c1a36f8af1e76c2dde Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Sat, 26 Sep 2020 17:38:11 +0200 Subject: [PATCH 28/42] libmain/progress-bar: don't trim whitespace on the left When running `nix build -L` it can be fairly hard to read the output if the build program intentionally renders whitespace on the left. A typical example is `g++` displaying compilation errors. With this patch, the whitespace on the left is retained to make the log more readable: ``` foo> no configure script, doing nothing foo> building foo> foobar.cc: In function 'int main()': foo> foobar.cc:5:5: error: 'wrong_func' was not declared in this scope foo> 5 | wrong_func(1); foo> | ^~~~~~~~~~ error: --- Error ------------------------------------------------------------------------------------- nix error: --- Error --- nix-daemon builder for '/nix/store/i1q76cw6cyh91raaqg5p5isd1l2x6rx2-foo-1.0.drv' failed with exit code 1 ``` --- src/libmain/progress-bar.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc index be3c06a38c0f..07b45b3b547d 100644 --- a/src/libmain/progress-bar.cc +++ b/src/libmain/progress-bar.cc @@ -256,7 +256,7 @@ class ProgressBar : public Logger } else if (type == resBuildLogLine || type == resPostBuildLogLine) { - auto lastLine = trim(getS(fields, 0)); + auto lastLine = chomp(getS(fields, 0)); if (!lastLine.empty()) { auto i = state->its.find(act); assert(i != state->its.end()); From 5885b0cfd878b4b60556c5b03bbe52244d04191a Mon Sep 17 00:00:00 2001 From: Kevin Quick Date: Sun, 27 Sep 2020 13:04:06 -0700 Subject: [PATCH 29/42] Miscellaneous spelling fixes in comments. (#4071) --- src/libexpr/flake/flake.cc | 2 +- src/libfetchers/fetchers.hh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index 760ed1a6e432..b4ede542c191 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -247,7 +247,7 @@ Flake getFlake(EvalState & state, const FlakeRef & originalRef, bool allowLookup } /* Compute an in-memory lock file for the specified top-level flake, - and optionally write it to file, it the flake is writable. */ + and optionally write it to file, if the flake is writable. */ LockedFlake lockFlake( EvalState & state, const FlakeRef & topRef, diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index 89b1e6e7dda1..191e91978fe9 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -73,7 +73,7 @@ public: StorePath computeStorePath(Store & store) const; - // Convience functions for common attributes. + // Convenience functions for common attributes. std::string getType() const; std::optional getNarHash() const; std::optional getRef() const; From 3655875483306fa893ec2b01295151819a00ccaa Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Sun, 27 Sep 2020 22:35:03 +0200 Subject: [PATCH 30/42] doc/manual: update hacking docs (#4078) * By default, build artifacts should be installed into `outputs/` rather than `inst/`[1]. * Add instructions on how to run unit-tests. [1] 733d2e9402807e54d503c3113e854bfddb3d44e0 --- doc/manual/src/hacking.md | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/doc/manual/src/hacking.md b/doc/manual/src/hacking.md index 5bd884ce8a1d..2a1e55e5b391 100644 --- a/doc/manual/src/hacking.md +++ b/doc/manual/src/hacking.md @@ -39,17 +39,17 @@ To build Nix itself in this shell: ```console [nix-shell]$ ./bootstrap.sh -[nix-shell]$ ./configure $configureFlags --prefix=$(pwd)/inst +[nix-shell]$ ./configure $configureFlags --prefix=$(pwd)/outputs/out [nix-shell]$ make -j $NIX_BUILD_CORES ``` -To install it in `$(pwd)/inst` and test it: +To install it in `$(pwd)/outputs` and test it: ```console [nix-shell]$ make install -[nix-shell]$ make installcheck -[nix-shell]$ ./inst/bin/nix --version -nix (Nix) 2.4 +[nix-shell]$ make installcheck -j $NIX_BUILD_CORES +[nix-shell]$ ./outputs/out/bin/nix --version +nix (Nix) 3.0 ``` To run a functional test: @@ -58,6 +58,12 @@ To run a functional test: make tests/test-name-should-auto-complete.sh.test ``` +To run the unit-tests for C++ code: + +``` +make check +``` + If you have a flakes-enabled Nix you can replace: ```console From 095a91f55a89d856e51ff099686e2bbe6fb9a384 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 28 Sep 2020 05:37:07 +0000 Subject: [PATCH 31/42] Bump cachix/install-nix-action from v10 to v11 Bumps [cachix/install-nix-action](https://github.com/cachix/install-nix-action) from v10 to v11. - [Release notes](https://github.com/cachix/install-nix-action/releases) - [Commits](https://github.com/cachix/install-nix-action/compare/v10...95a8068e317b8def9482980abe762f36c77ccc99) Signed-off-by: dependabot[bot] --- .github/workflows/test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1f504a8eab81..2f5c548d90f8 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -12,7 +12,7 @@ jobs: - uses: actions/checkout@v2 with: fetch-depth: 0 - - uses: cachix/install-nix-action@v10 + - uses: cachix/install-nix-action@v11 with: skip_adding_nixpkgs_channel: true #- run: nix flake check From ed66d010659a710646f5f1015c97a58614a713b3 Mon Sep 17 00:00:00 2001 From: Mateusz Piotrowski <0mp@FreeBSD.org> Date: Mon, 28 Sep 2020 15:23:21 +0200 Subject: [PATCH 32/42] Fix tar invocation on FreeBSD tar(1) on FreeBSD does not use standard output or input when the -f flag is not provided. Instead, it defaults to /dev/sa0 on FreeBSD. Make this tar invocation a bit more robust and explicitly tell tar(1) to use standard output. This is one of the issues discovered while porting Nix to FreeBSD. It has been tested and committed locally to FreeBSD ports: https://svnweb.freebsd.org/ports/head/sysutils/nix/Makefile?revision=550026&view=markup#l108 --- tests/tarball.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tarball.sh b/tests/tarball.sh index 88a1a07a0eef..fe65a22e4f80 100644 --- a/tests/tarball.sh +++ b/tests/tarball.sh @@ -17,7 +17,7 @@ test_tarball() { local compressor="$2" tarball=$TEST_ROOT/tarball.tar$ext - (cd $TEST_ROOT && tar c tarball) | $compressor > $tarball + (cd $TEST_ROOT && tar cf - tarball) | $compressor > $tarball nix-env -f file://$tarball -qa --out-path | grep -q dependencies From 80e335bb5837d7bfbf1f14d4f3d39525013c4f4d Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 28 Sep 2020 15:43:56 +0000 Subject: [PATCH 33/42] Use `drvPath2` and give it a better name --- src/libstore/build.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 0a6f2ccb51ef..0499273a4475 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -4315,19 +4315,19 @@ void DerivationGoal::registerOutputs() but it's fine to do in all cases. */ bool isCaFloating = drv->type() == DerivationType::CAFloating; - auto drvPath2 = drvPath; + auto drvPathResolved = drvPath; if (!useDerivation && isCaFloating) { /* Once a floating CA derivations reaches this point, it must already be resolved, so we don't bother trying to downcast drv to get would would just be an empty inputDrvs field. */ Derivation drv2 { *drv }; - drvPath2 = writeDerivation(worker.store, drv2); + drvPathResolved = writeDerivation(worker.store, drv2); } if (useDerivation || isCaFloating) for (auto & [outputName, newInfo] : infos) - worker.store.linkDeriverToPath(drvPath, outputName, newInfo.path); + worker.store.linkDeriverToPath(drvPathResolved, outputName, newInfo.path); } From c89fa3f644eec0652e97c11d9246f9580e5929fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Domen=20Ko=C5=BEar?= Date: Mon, 28 Sep 2020 21:08:14 +0300 Subject: [PATCH 34/42] Update .github/workflows/test.yml --- .github/workflows/test.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 2f5c548d90f8..adc56e223a3a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,7 +13,6 @@ jobs: with: fetch-depth: 0 - uses: cachix/install-nix-action@v11 - with: skip_adding_nixpkgs_channel: true #- run: nix flake check - run: nix-build -A checks.$(if [[ `uname` = Linux ]]; then echo x86_64-linux; else echo x86_64-darwin; fi) From f1428484be248c7ba3f96379d8bf256b8df1a706 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Domen=20Ko=C5=BEar?= Date: Mon, 28 Sep 2020 21:08:24 +0300 Subject: [PATCH 35/42] Update .github/workflows/test.yml --- .github/workflows/test.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index adc56e223a3a..829111b67727 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,6 +13,5 @@ jobs: with: fetch-depth: 0 - uses: cachix/install-nix-action@v11 - skip_adding_nixpkgs_channel: true #- run: nix flake check - run: nix-build -A checks.$(if [[ `uname` = Linux ]]; then echo x86_64-linux; else echo x86_64-darwin; fi) From 00135e13f49e9b20e3ef03f2516e7cc277c40ca9 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 28 Sep 2020 18:19:10 +0000 Subject: [PATCH 36/42] Clarify comment a bit --- src/libstore/derivations.hh | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index 74601134e375..d48266774e9b 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -128,11 +128,12 @@ struct Derivation : BasicDerivation std::string unparse(const Store & store, bool maskOutputs, std::map * actualInputs = nullptr) const; - /* Return the underlying basic derivation but with + /* Return the underlying basic derivation but with these changes: - 1. input drv outputs moved to input sources. + 1. Input drvs are emptied, but the outputs of them that were used are + added directly to input sources. - 2. placeholders replaced with realized input store paths. */ + 2. Input placeholders are replaced with realized input store paths. */ std::optional tryResolve(Store & store); Derivation() = default; From de86abbf3f9f0d18f7506b1d50072597786332ea Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 29 Sep 2020 12:55:06 +0200 Subject: [PATCH 37/42] Cleanup --- src/libfetchers/github.cc | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index 7bf155c69555..3734c427864d 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -14,12 +14,6 @@ struct DownloadUrl { std::string url; std::optional> access_token_header; - - DownloadUrl(const std::string & url) - : url(url) { } - - DownloadUrl(const std::string & url, const std::pair & access_token_header) - : url(url), access_token_header(access_token_header) { } }; // A github or gitlab host @@ -239,9 +233,9 @@ struct GitHubInputScheme : GitArchiveInputScheme std::string accessToken = settings.githubAccessToken.get(); if (accessToken != "") { auto auth_header = accessHeaderFromToken(accessToken); - return DownloadUrl(url, auth_header); + return DownloadUrl { url, auth_header }; } else { - return DownloadUrl(url); + return DownloadUrl { url }; } } @@ -294,9 +288,9 @@ struct GitLabInputScheme : GitArchiveInputScheme std::string accessToken = settings.gitlabAccessToken.get(); if (accessToken != "") { auto auth_header = accessHeaderFromToken(accessToken); - return DownloadUrl(url, auth_header); + return DownloadUrl { url, auth_header }; } else { - return DownloadUrl(url); + return DownloadUrl { url }; } } From 5999978a053b3ec16f448c40b54a1a62ceb82c90 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 29 Sep 2020 13:05:19 +0200 Subject: [PATCH 38/42] Make Headers an optional argument --- src/libexpr/common-eval-args.cc | 2 +- src/libexpr/parser.y | 2 +- src/libexpr/primops/fetchTree.cc | 4 ++-- src/libfetchers/fetchers.hh | 8 ++++---- src/libfetchers/github.cc | 6 +++--- src/libfetchers/registry.cc | 2 +- src/libfetchers/tarball.cc | 15 ++++++++------- src/libstore/filetransfer.hh | 3 --- src/nix-channel/nix-channel.cc | 6 +++--- 9 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/libexpr/common-eval-args.cc b/src/libexpr/common-eval-args.cc index d71aa22f1987..10c1a697509a 100644 --- a/src/libexpr/common-eval-args.cc +++ b/src/libexpr/common-eval-args.cc @@ -76,7 +76,7 @@ Path lookupFileArg(EvalState & state, string s) if (isUri(s)) { return state.store->toRealPath( fetchers::downloadTarball( - state.store, resolveUri(s), Headers {}, "source", false).first.storePath); + state.store, resolveUri(s), "source", false).first.storePath); } else if (s.size() > 2 && s.at(0) == '<' && s.at(s.size() - 1) == '>') { Path p = s.substr(1, s.size() - 2); return state.findFile(p); diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index e879280c0948..a4c84c52651d 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -718,7 +718,7 @@ std::pair EvalState::resolveSearchPathElem(const SearchPathEl if (isUri(elem.second)) { try { res = { true, store->toRealPath(fetchers::downloadTarball( - store, resolveUri(elem.second), Headers {}, "source", false).first.storePath) }; + store, resolveUri(elem.second), "source", false).first.storePath) }; } catch (FileTransferError & e) { logWarning({ .name = "Entry download", diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index 3001957b4e42..06e8304b8bc5 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -201,8 +201,8 @@ static void fetch(EvalState & state, const Pos & pos, Value * * args, Value & v, auto storePath = unpack - ? fetchers::downloadTarball(state.store, *url, Headers {}, name, (bool) expectedHash).first.storePath - : fetchers::downloadFile(state.store, *url, Headers{}, name, (bool) expectedHash).storePath; + ? fetchers::downloadTarball(state.store, *url, name, (bool) expectedHash).first.storePath + : fetchers::downloadFile(state.store, *url, name, (bool) expectedHash).storePath; auto path = state.store->toRealPath(storePath); diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index 0adc2c9f5c0e..36d44f6e153f 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -118,15 +118,15 @@ struct DownloadFileResult DownloadFileResult downloadFile( ref store, const std::string & url, - const Headers & headers, const std::string & name, - bool immutable); + bool immutable, + const Headers & headers = {}); std::pair downloadTarball( ref store, const std::string & url, - const Headers & headers, const std::string & name, - bool immutable); + bool immutable, + const Headers & headers = {}); } diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index 3734c427864d..ec99481e12dc 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -175,7 +175,7 @@ struct GitArchiveInputScheme : InputScheme headers.push_back(*url.access_token_header); } - auto [tree, lastModified] = downloadTarball(store, url.url, headers, "source", true); + auto [tree, lastModified] = downloadTarball(store, url.url, "source", true, headers); input.attrs.insert_or_assign("lastModified", lastModified); @@ -215,7 +215,7 @@ struct GitHubInputScheme : GitArchiveInputScheme auto json = nlohmann::json::parse( readFile( store->toRealPath( - downloadFile(store, url, headers, "source", false).storePath))); + downloadFile(store, url, "source", false, headers).storePath))); auto rev = Hash::parseAny(std::string { json["sha"] }, htSHA1); debug("HEAD revision for '%s' is %s", url, rev.gitRev()); return rev; @@ -271,7 +271,7 @@ struct GitLabInputScheme : GitArchiveInputScheme auto json = nlohmann::json::parse( readFile( store->toRealPath( - downloadFile(store, url, headers, "source", false).storePath))); + downloadFile(store, url, "source", false, headers).storePath))); auto rev = Hash::parseAny(std::string(json[0]["id"]), htSHA1); debug("HEAD revision for '%s' is %s", url, rev.gitRev()); return rev; diff --git a/src/libfetchers/registry.cc b/src/libfetchers/registry.cc index 551e7684acac..4367ee8103b2 100644 --- a/src/libfetchers/registry.cc +++ b/src/libfetchers/registry.cc @@ -145,7 +145,7 @@ static std::shared_ptr getGlobalRegistry(ref store) auto path = settings.flakeRegistry.get(); if (!hasPrefix(path, "/")) { - auto storePath = downloadFile(store, path, Headers {}, "flake-registry.json", false).storePath; + auto storePath = downloadFile(store, path, "flake-registry.json", false).storePath; if (auto store2 = store.dynamic_pointer_cast()) store2->addPermRoot(storePath, getCacheDir() + "/nix/flake-registry.json"); path = store->toRealPath(storePath); diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index cf6d6e3d2499..ca49482a9629 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -12,9 +12,9 @@ namespace nix::fetchers { DownloadFileResult downloadFile( ref store, const std::string & url, - const Headers & headers, const std::string & name, - bool immutable) + bool immutable, + const Headers & headers) { // FIXME: check store @@ -38,7 +38,8 @@ DownloadFileResult downloadFile( if (cached && !cached->expired) return useCached(); - FileTransferRequest request(url, headers); + FileTransferRequest request(url); + request.headers = headers; if (cached) request.expectedETag = getStrAttr(cached->infoAttrs, "etag"); FileTransferResult res; @@ -112,9 +113,9 @@ DownloadFileResult downloadFile( std::pair downloadTarball( ref store, const std::string & url, - const Headers & headers, const std::string & name, - bool immutable) + bool immutable, + const Headers & headers) { Attrs inAttrs({ {"type", "tarball"}, @@ -130,7 +131,7 @@ std::pair downloadTarball( getIntAttr(cached->infoAttrs, "lastModified") }; - auto res = downloadFile(store, url, headers, name, immutable); + auto res = downloadFile(store, url, name, immutable, headers); std::optional unpackedStorePath; time_t lastModified; @@ -225,7 +226,7 @@ struct TarballInputScheme : InputScheme std::pair fetch(ref store, const Input & input) override { - auto tree = downloadTarball(store, getStrAttr(input.attrs, "url"), Headers {}, "source", false).first; + auto tree = downloadTarball(store, getStrAttr(input.attrs, "url"), "source", false).first; return {std::move(tree), input}; } }; diff --git a/src/libstore/filetransfer.hh b/src/libstore/filetransfer.hh index 7e302ff39c7b..c89c51a21ed7 100644 --- a/src/libstore/filetransfer.hh +++ b/src/libstore/filetransfer.hh @@ -66,9 +66,6 @@ struct FileTransferRequest FileTransferRequest(const std::string & uri) : uri(uri), parentAct(getCurActivity()) { } - FileTransferRequest(const std::string & uri, Headers headers) - : uri(uri), headers(headers) { } - std::string verb() { return data ? "upload" : "download"; diff --git a/src/nix-channel/nix-channel.cc b/src/nix-channel/nix-channel.cc index 760bbea86c0a..e48f7af9a77a 100755 --- a/src/nix-channel/nix-channel.cc +++ b/src/nix-channel/nix-channel.cc @@ -94,7 +94,7 @@ static void update(const StringSet & channelNames) // We want to download the url to a file to see if it's a tarball while also checking if we // got redirected in the process, so that we can grab the various parts of a nix channel // definition from a consistent location if the redirect changes mid-download. - auto result = fetchers::downloadFile(store, url, Headers {}, std::string(baseNameOf(url)), false); + auto result = fetchers::downloadFile(store, url, std::string(baseNameOf(url)), false); auto filename = store->toRealPath(result.storePath); url = result.effectiveUrl; @@ -119,9 +119,9 @@ static void update(const StringSet & channelNames) if (!unpacked) { // Download the channel tarball. try { - filename = store->toRealPath(fetchers::downloadFile(store, url + "/nixexprs.tar.xz", Headers {}, "nixexprs.tar.xz", false).storePath); + filename = store->toRealPath(fetchers::downloadFile(store, url + "/nixexprs.tar.xz", "nixexprs.tar.xz", false).storePath); } catch (FileTransferError & e) { - filename = store->toRealPath(fetchers::downloadFile(store, url + "/nixexprs.tar.bz2", Headers {}, "nixexprs.tar.bz2", false).storePath); + filename = store->toRealPath(fetchers::downloadFile(store, url + "/nixexprs.tar.bz2", "nixexprs.tar.bz2", false).storePath); } } From 64e9b3c83b7cf7f3c7348426666ccca2ca395d28 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 29 Sep 2020 23:33:16 +0200 Subject: [PATCH 39/42] nix registry list: Show 'dir' attribute Issue #4050. --- src/libexpr/flake/flakeref.cc | 6 +++--- src/libfetchers/fetchers.cc | 8 ++++++++ src/libfetchers/fetchers.hh | 2 ++ src/nix/registry.cc | 4 ++-- 4 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/libexpr/flake/flakeref.cc b/src/libexpr/flake/flakeref.cc index d5c2ffe66c88..87b202643640 100644 --- a/src/libexpr/flake/flakeref.cc +++ b/src/libexpr/flake/flakeref.cc @@ -16,10 +16,10 @@ const static std::string subDirRegex = subDirElemRegex + "(?:/" + subDirElemRege std::string FlakeRef::to_string() const { - auto url = input.toURL(); + std::map extraQuery; if (subdir != "") - url.query.insert_or_assign("dir", subdir); - return url.to_string(); + extraQuery.insert_or_assign("dir", subdir); + return input.toURLString(extraQuery); } fetchers::Attrs FlakeRef::toAttrs() const diff --git a/src/libfetchers/fetchers.cc b/src/libfetchers/fetchers.cc index eaa6355953e1..49851f7bc520 100644 --- a/src/libfetchers/fetchers.cc +++ b/src/libfetchers/fetchers.cc @@ -69,6 +69,14 @@ ParsedURL Input::toURL() const return scheme->toURL(*this); } +std::string Input::toURLString(const std::map & extraQuery) const +{ + auto url = toURL(); + for (auto & attr : extraQuery) + url.query.insert(attr); + return url.to_string(); +} + std::string Input::to_string() const { return toURL().to_string(); diff --git a/src/libfetchers/fetchers.hh b/src/libfetchers/fetchers.hh index 36d44f6e153f..cc31a31b9709 100644 --- a/src/libfetchers/fetchers.hh +++ b/src/libfetchers/fetchers.hh @@ -39,6 +39,8 @@ public: ParsedURL toURL() const; + std::string toURLString(const std::map & extraQuery = {}) const; + std::string to_string() const; Attrs toAttrs() const; diff --git a/src/nix/registry.cc b/src/nix/registry.cc index 367268683d66..cb11ec1955f4 100644 --- a/src/nix/registry.cc +++ b/src/nix/registry.cc @@ -31,8 +31,8 @@ struct CmdRegistryList : StoreCommand registry->type == Registry::User ? "user " : registry->type == Registry::System ? "system" : "global", - entry.from.to_string(), - entry.to.to_string()); + entry.from.toURLString(), + entry.to.toURLString(attrsToQuery(entry.extraAttrs))); } } } From 5e7838512e2b8de3c8fe271b8beae5ca9e1efaf9 Mon Sep 17 00:00:00 2001 From: Kevin Quick Date: Tue, 29 Sep 2020 16:20:54 -0700 Subject: [PATCH 40/42] Remove github-access-token in favor of access-token. --- src/libfetchers/github.cc | 10 ---------- src/libstore/globals.hh | 3 --- 2 files changed, 13 deletions(-) diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index 142b8b87c7b1..8286edf75a0f 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -147,17 +147,7 @@ struct GitArchiveInputScheme : InputScheme auto tokens = settings.accessTokens.get(); auto pat = tokens.find(host); if (pat == tokens.end()) - { - if ("github.com" == host) - { - auto oldcfg = settings.githubAccessToken.get(); - if (!oldcfg.empty()) { - warn("using deprecated 'github-access-token' config value; please use 'access-tokens' instead"); - return oldcfg; - } - } return std::nullopt; - } return pat->second; } diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 3b8ccadf3fd8..0f0c0fe6fcc7 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -859,9 +859,6 @@ public: are loaded as plugins (non-recursively). )"}; - Setting githubAccessToken{this, "", "github-access-token", - "GitHub access token to get access to GitHub data through the GitHub API for `github:<..>` flakes (deprecated, please use 'access-tokens' instead)."}; - Setting accessTokens{this, {}, "access-tokens", R"( Access tokens used to access protected GitHub, GitLab, or From 274357eb6acd9a0812872f32dd487354d51f0f13 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 30 Sep 2020 12:09:18 +0200 Subject: [PATCH 41/42] Simplify --- src/libfetchers/github.cc | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index 8286edf75a0f..42eb346a5582 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -15,9 +15,6 @@ struct DownloadUrl { std::string url; Headers headers; - - DownloadUrl(const std::string & url, const Headers & headers) - : url(url), headers(headers) { } }; // A github or gitlab host @@ -254,7 +251,7 @@ struct GitHubInputScheme : GitArchiveInputScheme input.getRev()->to_string(Base16, false)); Headers headers = makeHeadersWithAuthTokens(host); - return DownloadUrl(url, headers); + return DownloadUrl { url, headers }; } void clone(const Input & input, const Path & destDir) override @@ -320,7 +317,7 @@ struct GitLabInputScheme : GitArchiveInputScheme input.getRev()->to_string(Base16, false)); Headers headers = makeHeadersWithAuthTokens(host); - return DownloadUrl(url, headers); + return DownloadUrl { url, headers }; } void clone(const Input & input, const Path & destDir) override From 20a1e20d9194527d725898c745d1243d3de16277 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 30 Sep 2020 12:11:22 +0200 Subject: [PATCH 42/42] Style --- src/libfetchers/github.cc | 19 +++++++++++-------- src/libstore/globals.hh | 8 ++++---- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index 42eb346a5582..8610fe44789d 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -140,15 +140,16 @@ struct GitArchiveInputScheme : InputScheme return input; } - std::optional getAccessToken(const std::string &host) const { + std::optional getAccessToken(const std::string & host) const + { auto tokens = settings.accessTokens.get(); - auto pat = tokens.find(host); - if (pat == tokens.end()) - return std::nullopt; - return pat->second; + if (auto token = get(tokens, host)) + return *token; + return {}; } - Headers makeHeadersWithAuthTokens(const std::string & host) const { + Headers makeHeadersWithAuthTokens(const std::string & host) const + { Headers headers; auto accessToken = getAccessToken(host); if (accessToken) { @@ -214,7 +215,8 @@ struct GitHubInputScheme : GitArchiveInputScheme { std::string type() override { return "github"; } - std::optional > accessHeaderFromToken(const std::string & token) const { + std::optional > accessHeaderFromToken(const std::string & token) const + { // Github supports PAT/OAuth2 tokens and HTTP Basic // Authentication. The former simply specifies the token, the // latter can use the token as the password. Only the first @@ -268,7 +270,8 @@ struct GitLabInputScheme : GitArchiveInputScheme { std::string type() override { return "gitlab"; } - std::optional > accessHeaderFromToken(const std::string & token) const { + std::optional > accessHeaderFromToken(const std::string & token) const + { // Gitlab supports 4 kinds of authorization, two of which are // relevant here: OAuth2 and PAT (Private Access Token). The // user can indicate which token is used by specifying the diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 0f0c0fe6fcc7..8c63c5b34829 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -893,10 +893,10 @@ public: ```nix input.foo = { - type="gitlab"; - host="gitlab.mycompany.com"; - owner="mycompany"; - repo="pro"; + type = "gitlab"; + host = "gitlab.mycompany.com"; + owner = "mycompany"; + repo = "pro"; }; ```