From 225e62a56a7cebb030bebffb8d2bd7afe21cc64a Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 29 Mar 2020 01:04:55 -0400 Subject: [PATCH 01/58] Replace some `bool recursive` with a new `FileIngestionMethod` enum --- perl/lib/Nix/Store.xs | 4 ++-- src/libexpr/eval.cc | 2 +- src/libexpr/primops.cc | 16 ++++++++-------- src/libexpr/primops/fetchGit.cc | 2 +- src/libexpr/primops/fetchMercurial.cc | 2 +- src/libstore/binary-cache-store.cc | 4 ++-- src/libstore/binary-cache-store.hh | 2 +- src/libstore/build.cc | 12 ++++++------ src/libstore/daemon.cc | 24 ++++++++++++++++-------- src/libstore/derivations.cc | 6 +++--- src/libstore/derivations.hh | 2 +- src/libstore/download.cc | 9 +++++---- src/libstore/legacy-ssh-store.cc | 2 +- src/libstore/local-store.cc | 12 ++++++------ src/libstore/local-store.hh | 4 ++-- src/libstore/path.hh | 5 +++++ src/libstore/remote-store.cc | 12 +++++++----- src/libstore/remote-store.hh | 2 +- src/libstore/store-api.cc | 18 +++++++++--------- src/libstore/store-api.hh | 11 +++++------ src/nix-prefetch-url/nix-prefetch-url.cc | 9 ++++++--- src/nix-store/nix-store.cc | 8 ++++---- src/nix/add-to-store.cc | 4 ++-- src/nix/command.cc | 10 +++++----- src/nix/command.hh | 4 ++-- src/nix/copy.cc | 2 +- src/nix/make-content-addressable.cc | 4 ++-- 27 files changed, 105 insertions(+), 87 deletions(-) diff --git a/perl/lib/Nix/Store.xs b/perl/lib/Nix/Store.xs index 1ca734e75..eab8ccacb 100644 --- a/perl/lib/Nix/Store.xs +++ b/perl/lib/Nix/Store.xs @@ -274,7 +274,7 @@ int checkSignature(SV * publicKey_, SV * sig_, char * msg) SV * addToStore(char * srcPath, int recursive, char * algo) PPCODE: try { - auto path = store()->addToStore(std::string(baseNameOf(srcPath)), srcPath, recursive, parseHashType(algo)); + auto path = store()->addToStore(std::string(baseNameOf(srcPath)), srcPath, (FileIngestionMethod) recursive, parseHashType(algo)); XPUSHs(sv_2mortal(newSVpv(store()->printStorePath(path).c_str(), 0))); } catch (Error & e) { croak("%s", e.what()); @@ -285,7 +285,7 @@ SV * makeFixedOutputPath(int recursive, char * algo, char * hash, char * name) PPCODE: try { Hash h(hash, parseHashType(algo)); - auto path = store()->makeFixedOutputPath(recursive, h, name); + auto path = store()->makeFixedOutputPath((FileIngestionMethod) recursive, h, name); XPUSHs(sv_2mortal(newSVpv(store()->printStorePath(path).c_str(), 0))); } catch (Error & e) { croak("%s", e.what()); diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index dac32b6f5..88bbf3b32 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1661,7 +1661,7 @@ string EvalState::copyPathToStore(PathSet & context, const Path & path) else { auto p = settings.readOnlyMode ? store->computeStorePathForPath(std::string(baseNameOf(path)), checkSourcePath(path)).first - : store->addToStore(std::string(baseNameOf(path)), checkSourcePath(path), true, htSHA256, defaultPathFilter, repair); + : store->addToStore(std::string(baseNameOf(path)), checkSourcePath(path), FileIngestionMethod::Recursive, htSHA256, defaultPathFilter, repair); dstPath = store->printStorePath(p); srcToStore.insert_or_assign(path, std::move(p)); printMsg(lvlChatty, "copied source '%1%' -> '%2%'", path, dstPath); diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 8de234951..7d45733f4 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -563,7 +563,7 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * std::optional outputHash; std::string outputHashAlgo; - bool outputHashRecursive = false; + FileIngestionMethod outputHashRecursive = FileIngestionMethod::Flat; StringSet outputs; outputs.insert("out"); @@ -574,8 +574,8 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * vomit("processing attribute '%1%'", key); auto handleHashMode = [&](const std::string & s) { - if (s == "recursive") outputHashRecursive = true; - else if (s == "flat") outputHashRecursive = false; + if (s == "recursive") outputHashRecursive = FileIngestionMethod::Recursive; + else if (s == "flat") outputHashRecursive = FileIngestionMethod::Flat; else throw EvalError("invalid value '%s' for 'outputHashMode' attribute, at %s", s, posDrvName); }; @@ -725,7 +725,7 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * auto outPath = state.store->makeFixedOutputPath(outputHashRecursive, h, drvName); if (!jsonObject) drv.env["out"] = state.store->printStorePath(outPath); drv.outputs.insert_or_assign("out", DerivationOutput(std::move(outPath), - (outputHashRecursive ? "r:" : "") + printHashType(h.type), + (static_cast(outputHashRecursive) ? "r:" : "") + printHashType(h.type), h.to_string(Base16, false))); } @@ -1038,7 +1038,7 @@ static void prim_toFile(EvalState & state, const Pos & pos, Value * * args, Valu static void addPath(EvalState & state, const Pos & pos, const string & name, const Path & path_, - Value * filterFun, bool recursive, const Hash & expectedHash, Value & v) + Value * filterFun, FileIngestionMethod recursive, const Hash & expectedHash, Value & v) { const auto path = evalSettings.pureEval && expectedHash ? path_ : @@ -1095,7 +1095,7 @@ static void prim_filterSource(EvalState & state, const Pos & pos, Value * * args if (args[0]->type != tLambda) throw TypeError(format("first argument in call to 'filterSource' is not a function but %1%, at %2%") % showType(*args[0]) % pos); - addPath(state, pos, std::string(baseNameOf(path)), path, args[0], true, Hash(), v); + addPath(state, pos, std::string(baseNameOf(path)), path, args[0], FileIngestionMethod::Recursive, Hash(), v); } static void prim_path(EvalState & state, const Pos & pos, Value * * args, Value & v) @@ -1104,7 +1104,7 @@ static void prim_path(EvalState & state, const Pos & pos, Value * * args, Value Path path; string name; Value * filterFun = nullptr; - auto recursive = true; + auto recursive = FileIngestionMethod::Recursive; Hash expectedHash; for (auto & attr : *args[0]->attrs) { @@ -1120,7 +1120,7 @@ static void prim_path(EvalState & state, const Pos & pos, Value * * args, Value state.forceValue(*attr.value); filterFun = attr.value; } else if (n == "recursive") - recursive = state.forceBool(*attr.value, *attr.pos); + recursive = FileIngestionMethod { state.forceBool(*attr.value, *attr.pos) }; else if (n == "sha256") expectedHash = Hash(state.forceStringNoCtx(*attr.value, *attr.pos), htSHA256); else diff --git a/src/libexpr/primops/fetchGit.cc b/src/libexpr/primops/fetchGit.cc index 4aee1073e..9f2b673ad 100644 --- a/src/libexpr/primops/fetchGit.cc +++ b/src/libexpr/primops/fetchGit.cc @@ -69,7 +69,7 @@ GitInfo exportGit(ref store, const std::string & uri, return files.count(file); }; - gitInfo.storePath = store->printStorePath(store->addToStore("source", uri, true, htSHA256, filter)); + gitInfo.storePath = store->printStorePath(store->addToStore("source", uri, FileIngestionMethod::Recursive, htSHA256, filter)); return gitInfo; } diff --git a/src/libexpr/primops/fetchMercurial.cc b/src/libexpr/primops/fetchMercurial.cc index db274fa4f..548f4e392 100644 --- a/src/libexpr/primops/fetchMercurial.cc +++ b/src/libexpr/primops/fetchMercurial.cc @@ -63,7 +63,7 @@ HgInfo exportMercurial(ref store, const std::string & uri, return files.count(file); }; - hgInfo.storePath = store->printStorePath(store->addToStore("source", uri, true, htSHA256, filter)); + hgInfo.storePath = store->printStorePath(store->addToStore("source", uri, FileIngestionMethod::Recursive, htSHA256, filter)); return hgInfo; } diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index 3a2d84861..a61af4a00 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -327,7 +327,7 @@ void BinaryCacheStore::queryPathInfoUncached(const StorePath & storePath, } StorePath BinaryCacheStore::addToStore(const string & name, const Path & srcPath, - bool recursive, HashType hashAlgo, PathFilter & filter, RepairFlag repair) + FileIngestionMethod recursive, HashType hashAlgo, PathFilter & filter, RepairFlag repair) { // FIXME: some cut&paste from LocalStore::addToStore(). @@ -336,7 +336,7 @@ StorePath BinaryCacheStore::addToStore(const string & name, const Path & srcPath small files. */ StringSink sink; Hash h; - if (recursive) { + if (recursive == FileIngestionMethod::Recursive) { dumpPath(srcPath, sink, filter); h = hashString(hashAlgo, *sink.s); } else { diff --git a/src/libstore/binary-cache-store.hh b/src/libstore/binary-cache-store.hh index aa13c1cb4..1a1ea636c 100644 --- a/src/libstore/binary-cache-store.hh +++ b/src/libstore/binary-cache-store.hh @@ -79,7 +79,7 @@ public: std::shared_ptr accessor) override; StorePath addToStore(const string & name, const Path & srcPath, - bool recursive, HashType hashAlgo, + FileIngestionMethod recursive, HashType hashAlgo, PathFilter & filter, RepairFlag repair) override; StorePath addTextToStore(const string & name, const string & s, diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 0e3a23a4d..224633106 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2712,7 +2712,7 @@ struct RestrictedStore : public LocalFSStore { throw Error("queryPathFromHashPart"); } StorePath addToStore(const string & name, const Path & srcPath, - bool recursive = true, HashType hashAlgo = htSHA256, + FileIngestionMethod recursive = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, PathFilter & filter = defaultPathFilter, RepairFlag repair = NoRepair) override { throw Error("addToStore"); } @@ -2725,7 +2725,7 @@ struct RestrictedStore : public LocalFSStore } StorePath addToStoreFromDump(const string & dump, const string & name, - bool recursive = true, HashType hashAlgo = htSHA256, RepairFlag repair = NoRepair) override + FileIngestionMethod recursive = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, RepairFlag repair = NoRepair) override { auto path = next->addToStoreFromDump(dump, name, recursive, hashAlgo, repair); goal.addDependency(path); @@ -3647,10 +3647,10 @@ void DerivationGoal::registerOutputs() if (fixedOutput) { - bool recursive; Hash h; + FileIngestionMethod recursive; Hash h; i.second.parseHashInfo(recursive, h); - if (!recursive) { + if (!static_cast(recursive)) { /* The output path should be a regular file without execute permission. */ if (!S_ISREG(st.st_mode) || (st.st_mode & S_IXUSR) != 0) throw BuildError( @@ -3659,7 +3659,7 @@ void DerivationGoal::registerOutputs() /* Check the hash. In hash mode, move the path produced by the derivation to its content-addressed location. */ - Hash h2 = recursive ? hashPath(h.type, actualPath).first : hashFile(h.type, actualPath); + Hash h2 = static_cast(recursive) ? hashPath(h.type, actualPath).first : hashFile(h.type, actualPath); auto dest = worker.store.makeFixedOutputPath(recursive, h2, i.second.path.name()); @@ -3912,7 +3912,7 @@ void DerivationGoal::checkOutputs(const std::map & outputs) auto spec = parseReferenceSpecifiers(worker.store, *drv, *value); - auto used = recursive ? cloneStorePathSet(getClosure(info.path).first) : cloneStorePathSet(info.references); + auto used = static_cast(recursive) ? cloneStorePathSet(getClosure(info.path).first) : cloneStorePathSet(info.references); if (recursive && checks.ignoreSelfRefs) used.erase(info.path); diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 8e9f9d71b..f1afdff69 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -355,20 +355,24 @@ static void performOp(TunnelLogger * logger, ref store, } case wopAddToStore: { - bool fixed, recursive; std::string s, baseName; - from >> baseName >> fixed /* obsolete */ >> recursive >> s; - /* Compatibility hack. */ - if (!fixed) { - s = "sha256"; - recursive = true; + FileIngestionMethod method; + { + bool fixed, recursive; + from >> baseName >> fixed /* obsolete */ >> recursive >> s; + method = FileIngestionMethod { recursive }; + /* Compatibility hack. */ + if (!fixed) { + s = "sha256"; + method = FileIngestionMethod::Recursive; + } } HashType hashAlgo = parseHashType(s); TeeSource savedNAR(from); RetrieveRegularNARSink savedRegular; - if (recursive) { + if (method == FileIngestionMethod::Recursive) { /* Get the entire NAR dump from the client and save it to a string so that we can pass it to addToStoreFromDump(). */ @@ -380,7 +384,11 @@ static void performOp(TunnelLogger * logger, ref store, logger->startWork(); if (!savedRegular.regular) throw Error("regular file expected"); - auto path = store->addToStoreFromDump(recursive ? *savedNAR.data : savedRegular.s, baseName, recursive, hashAlgo); + auto path = store->addToStoreFromDump( + method == FileIngestionMethod::Recursive ? *savedNAR.data : savedRegular.s, + baseName, + method, + hashAlgo); logger->stopWork(); to << store->printStorePath(path); diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 973ddc86a..5934c1912 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -9,13 +9,13 @@ namespace nix { -void DerivationOutput::parseHashInfo(bool & recursive, Hash & hash) const +void DerivationOutput::parseHashInfo(FileIngestionMethod & recursive, Hash & hash) const { - recursive = false; + recursive = FileIngestionMethod::Flat; string algo = hashAlgo; if (string(algo, 0, 2) == "r:") { - recursive = true; + recursive = FileIngestionMethod::Recursive; algo = string(algo, 2); } diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index 7222d25e5..b1224b93b 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -22,7 +22,7 @@ struct DerivationOutput , hashAlgo(std::move(hashAlgo)) , hash(std::move(hash)) { } - void parseHashInfo(bool & recursive, Hash & hash) const; + void parseHashInfo(FileIngestionMethod & recursive, Hash & hash) const; }; typedef std::map DerivationOutputs; diff --git a/src/libstore/download.cc b/src/libstore/download.cc index 149c84765..8a9b65899 100644 --- a/src/libstore/download.cc +++ b/src/libstore/download.cc @@ -814,7 +814,8 @@ CachedDownloadResult Downloader::downloadCached( std::optional expectedStorePath; if (request.expectedHash) { - expectedStorePath = store->makeFixedOutputPath(request.unpack, request.expectedHash, name); + auto method = request.unpack ? FileIngestionMethod::Recursive : FileIngestionMethod::Flat; + expectedStorePath = store->makeFixedOutputPath(method, request.expectedHash, name); if (store->isValidPath(*expectedStorePath)) { CachedDownloadResult result; result.storePath = store->printStorePath(*expectedStorePath); @@ -875,10 +876,10 @@ CachedDownloadResult Downloader::downloadCached( StringSink sink; dumpString(*res.data, sink); Hash hash = hashString(request.expectedHash ? request.expectedHash.type : htSHA256, *res.data); - ValidPathInfo info(store->makeFixedOutputPath(false, hash, name)); + ValidPathInfo info(store->makeFixedOutputPath(FileIngestionMethod::Flat, hash, name)); info.narHash = hashString(htSHA256, *sink.s); info.narSize = sink.s->size(); - info.ca = makeFixedOutputCA(false, hash); + info.ca = makeFixedOutputCA(FileIngestionMethod::Flat, hash); store->addToStore(info, sink.s, NoRepair, NoCheckSigs); storePath = info.path.clone(); } @@ -914,7 +915,7 @@ CachedDownloadResult Downloader::downloadCached( if (members.size() != 1) throw nix::Error("tarball '%s' contains an unexpected number of top-level files", url); auto topDir = tmpDir + "/" + members.begin()->name; - unpackedStorePath = store->addToStore(name, topDir, true, htSHA256, defaultPathFilter, NoRepair); + unpackedStorePath = store->addToStore(name, topDir, FileIngestionMethod::Recursive, htSHA256, defaultPathFilter, NoRepair); } replaceSymlink(store->printStorePath(*unpackedStorePath), unpackedLink); storePath = std::move(*unpackedStorePath); diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index 458266be0..af20d389b 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -195,7 +195,7 @@ struct LegacySSHStore : public Store { unsupported("queryPathFromHashPart"); } StorePath addToStore(const string & name, const Path & srcPath, - bool recursive, HashType hashAlgo, + FileIngestionMethod method, HashType hashAlgo, PathFilter & filter, RepairFlag repair) override { unsupported("addToStore"); } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index ae7513ad8..746f81beb 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -557,7 +557,7 @@ void LocalStore::checkDerivationOutputs(const StorePath & drvPath, const Derivat if (out == drv.outputs.end()) throw Error("derivation '%s' does not have an output named 'out'", printStorePath(drvPath)); - bool recursive; Hash h; + FileIngestionMethod recursive; Hash h; out->second.parseHashInfo(recursive, h); check(makeFixedOutputPath(recursive, h, drvName), out->second.path, "out"); @@ -1043,7 +1043,7 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, StorePath LocalStore::addToStoreFromDump(const string & dump, const string & name, - bool recursive, HashType hashAlgo, RepairFlag repair) + FileIngestionMethod recursive, HashType hashAlgo, RepairFlag repair) { Hash h = hashString(hashAlgo, dump); @@ -1067,7 +1067,7 @@ StorePath LocalStore::addToStoreFromDump(const string & dump, const string & nam autoGC(); - if (recursive) { + if (recursive == FileIngestionMethod::Recursive) { StringSource source(dump); restorePath(realPath, source); } else @@ -1080,7 +1080,7 @@ StorePath LocalStore::addToStoreFromDump(const string & dump, const string & nam above (if called with recursive == true and hashAlgo == sha256); otherwise, compute it here. */ HashResult hash; - if (recursive) { + if (recursive == FileIngestionMethod::Recursive) { hash.first = hashAlgo == htSHA256 ? h : hashString(htSHA256, dump); hash.second = dump.size(); } else @@ -1103,7 +1103,7 @@ StorePath LocalStore::addToStoreFromDump(const string & dump, const string & nam StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, - bool recursive, HashType hashAlgo, PathFilter & filter, RepairFlag repair) + FileIngestionMethod recursive, HashType hashAlgo, PathFilter & filter, RepairFlag repair) { Path srcPath(absPath(_srcPath)); @@ -1111,7 +1111,7 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, method for very large paths, but `copyPath' is mainly used for small files. */ StringSink sink; - if (recursive) + if (recursive == FileIngestionMethod::Recursive) dumpPath(srcPath, sink, filter); else sink.s = make_ref(readFile(srcPath)); diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 16aeab0ad..c1e75390c 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -149,7 +149,7 @@ public: std::shared_ptr accessor) override; StorePath addToStore(const string & name, const Path & srcPath, - bool recursive, HashType hashAlgo, + FileIngestionMethod method, HashType hashAlgo, PathFilter & filter, RepairFlag repair) override; /* Like addToStore(), but the contents of the path are contained @@ -157,7 +157,7 @@ public: true) or simply the contents of a regular file (if recursive == false). */ StorePath addToStoreFromDump(const string & dump, const string & name, - bool recursive = true, HashType hashAlgo = htSHA256, RepairFlag repair = NoRepair) override; + FileIngestionMethod method = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, RepairFlag repair = NoRepair) override; StorePath addTextToStore(const string & name, const string & s, const StorePathSet & references, RepairFlag repair) override; diff --git a/src/libstore/path.hh b/src/libstore/path.hh index c90bb1fff..73c4b8e29 100644 --- a/src/libstore/path.hh +++ b/src/libstore/path.hh @@ -73,6 +73,11 @@ const size_t storePathHashLen = 32; // i.e. 160 bits /* Extension of derivations in the Nix store. */ const std::string drvExtension = ".drv"; +enum struct FileIngestionMethod : bool { + Flat = false, + Recursive = true +}; + struct StorePathWithOutputs { StorePath path; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 8c55da268..5c36693e6 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -484,7 +484,7 @@ void RemoteStore::addToStore(const ValidPathInfo & info, Source & source, StorePath RemoteStore::addToStore(const string & name, const Path & _srcPath, - bool recursive, HashType hashAlgo, PathFilter & filter, RepairFlag repair) + FileIngestionMethod method, HashType hashAlgo, PathFilter & filter, RepairFlag repair) { if (repair) throw Error("repairing is not supported when building through the Nix daemon"); @@ -492,10 +492,12 @@ StorePath RemoteStore::addToStore(const string & name, const Path & _srcPath, Path srcPath(absPath(_srcPath)); - conn->to << wopAddToStore << name - << ((hashAlgo == htSHA256 && recursive) ? 0 : 1) /* backwards compatibility hack */ - << (recursive ? 1 : 0) - << printHashType(hashAlgo); + conn->to + << wopAddToStore + << name + << ((hashAlgo == htSHA256 && method == FileIngestionMethod::Recursive) ? 0 : 1) /* backwards compatibility hack */ + << (method == FileIngestionMethod::Recursive ? 1 : 0) + << printHashType(hashAlgo); try { conn->to.written = 0; diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index f301a97d8..3c86b4524 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -65,7 +65,7 @@ public: std::shared_ptr accessor) override; StorePath addToStore(const string & name, const Path & srcPath, - bool recursive = true, HashType hashAlgo = htSHA256, + FileIngestionMethod method = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, PathFilter & filter = defaultPathFilter, RepairFlag repair = NoRepair) override; StorePath addTextToStore(const string & name, const string & s, diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index b9e894a9a..06aa0883c 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -171,18 +171,18 @@ static std::string makeType( StorePath Store::makeFixedOutputPath( - bool recursive, + FileIngestionMethod recursive, const Hash & hash, std::string_view name, const StorePathSet & references, bool hasSelfReference) const { - if (hash.type == htSHA256 && recursive) { + if (hash.type == htSHA256 && recursive == FileIngestionMethod::Recursive) { return makeStorePath(makeType(*this, "source", references, hasSelfReference), hash, name); } else { assert(references.empty()); return makeStorePath("output:out", hashString(htSHA256, - "fixed:out:" + (recursive ? (string) "r:" : "") + + "fixed:out:" + (static_cast(recursive) ? (string) "r:" : "") + hash.to_string(Base16) + ":"), name); } } @@ -200,9 +200,9 @@ StorePath Store::makeTextPath(std::string_view name, const Hash & hash, std::pair Store::computeStorePathForPath(std::string_view name, - const Path & srcPath, bool recursive, HashType hashAlgo, PathFilter & filter) const + const Path & srcPath, FileIngestionMethod recursive, HashType hashAlgo, PathFilter & filter) const { - Hash h = recursive ? hashPath(hashAlgo, srcPath, filter).first : hashFile(hashAlgo, srcPath); + Hash h = static_cast(recursive) ? hashPath(hashAlgo, srcPath, filter).first : hashFile(hashAlgo, srcPath); return std::make_pair(makeFixedOutputPath(recursive, h, name), h); } @@ -781,8 +781,8 @@ bool ValidPathInfo::isContentAddressed(const Store & store) const } else if (hasPrefix(ca, "fixed:")) { - bool recursive = ca.compare(6, 2, "r:") == 0; - Hash hash(std::string(ca, recursive ? 8 : 6)); + FileIngestionMethod recursive { ca.compare(6, 2, "r:") == 0 }; + Hash hash(std::string(ca, static_cast(recursive) ? 8 : 6)); auto refs = cloneStorePathSet(references); bool hasSelfReference = false; if (refs.count(path)) { @@ -826,9 +826,9 @@ Strings ValidPathInfo::shortRefs() const } -std::string makeFixedOutputCA(bool recursive, const Hash & hash) +std::string makeFixedOutputCA(FileIngestionMethod recursive, const Hash & hash) { - return "fixed:" + (recursive ? (std::string) "r:" : "") + hash.to_string(); + return "fixed:" + (static_cast(recursive) ? (std::string) "r:" : "") + hash.to_string(); } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 0fa59be6a..32c24c500 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -43,7 +43,6 @@ enum CheckSigsFlag : bool { NoCheckSigs = false, CheckSigs = true }; enum SubstituteFlag : bool { NoSubstitute = false, Substitute = true }; enum AllowInvalidFlag : bool { DisallowInvalid = false, AllowInvalid = true }; - /* Magic header of exportPath() output (obsolete). */ const uint32_t exportMagic = 0x4558494e; @@ -346,7 +345,7 @@ public: StorePath makeOutputPath(const string & id, const Hash & hash, std::string_view name) const; - StorePath makeFixedOutputPath(bool recursive, + StorePath makeFixedOutputPath(FileIngestionMethod method, const Hash & hash, std::string_view name, const StorePathSet & references = {}, bool hasSelfReference = false) const; @@ -358,7 +357,7 @@ public: store path to which srcPath is to be copied. Returns the store path and the cryptographic hash of the contents of srcPath. */ std::pair computeStorePathForPath(std::string_view name, - const Path & srcPath, bool recursive = true, + const Path & srcPath, FileIngestionMethod method = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, PathFilter & filter = defaultPathFilter) const; /* Preparatory part of addTextToStore(). @@ -462,12 +461,12 @@ public: The function object `filter' can be used to exclude files (see libutil/archive.hh). */ virtual StorePath addToStore(const string & name, const Path & srcPath, - bool recursive = true, HashType hashAlgo = htSHA256, + FileIngestionMethod method = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, PathFilter & filter = defaultPathFilter, RepairFlag repair = NoRepair) = 0; // FIXME: remove? virtual StorePath addToStoreFromDump(const string & dump, const string & name, - bool recursive = true, HashType hashAlgo = htSHA256, RepairFlag repair = NoRepair) + FileIngestionMethod method = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, RepairFlag repair = NoRepair) { throw Error("addToStoreFromDump() is not supported by this store"); } @@ -850,7 +849,7 @@ std::optional decodeValidPathInfo( /* Compute the content-addressability assertion (ValidPathInfo::ca) for paths created by makeFixedOutputPath() / addToStore(). */ -std::string makeFixedOutputCA(bool recursive, const Hash & hash); +std::string makeFixedOutputCA(FileIngestionMethod method, const Hash & hash); /* Split URI into protocol+hierarchy part and its parameter set. */ diff --git a/src/nix-prefetch-url/nix-prefetch-url.cc b/src/nix-prefetch-url/nix-prefetch-url.cc index 18ced94b1..8c364a86a 100644 --- a/src/nix-prefetch-url/nix-prefetch-url.cc +++ b/src/nix-prefetch-url/nix-prefetch-url.cc @@ -159,7 +159,8 @@ static int _main(int argc, char * * argv) std::optional storePath; if (args.size() == 2) { expectedHash = Hash(args[1], ht); - storePath = store->makeFixedOutputPath(unpack, expectedHash, name); + const auto recursive = unpack ? FileIngestionMethod::Recursive : FileIngestionMethod::Flat; + storePath = store->makeFixedOutputPath(recursive, expectedHash, name); if (store->isValidPath(*storePath)) hash = expectedHash; else @@ -208,13 +209,15 @@ static int _main(int argc, char * * argv) if (expectedHash != Hash(ht) && expectedHash != hash) throw Error(format("hash mismatch for '%1%'") % uri); + const auto recursive = unpack ? FileIngestionMethod::Recursive : FileIngestionMethod::Flat; + /* Copy the file to the Nix store. FIXME: if RemoteStore implemented addToStoreFromDump() and downloadFile() supported a sink, we could stream the download directly into the Nix store. */ - storePath = store->addToStore(name, tmpFile, unpack, ht); + storePath = store->addToStore(name, tmpFile, recursive, ht); - assert(*storePath == store->makeFixedOutputPath(unpack, hash, name)); + assert(*storePath == store->makeFixedOutputPath(recursive, hash, name)); } stopProgressBar(); diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index 806ab7563..9a26eb57f 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -174,10 +174,10 @@ static void opAdd(Strings opFlags, Strings opArgs) store. */ static void opAddFixed(Strings opFlags, Strings opArgs) { - bool recursive = false; + FileIngestionMethod recursive = FileIngestionMethod::Flat; for (auto & i : opFlags) - if (i == "--recursive") recursive = true; + if (i == "--recursive") recursive = FileIngestionMethod::Recursive; else throw UsageError(format("unknown flag '%1%'") % i); if (opArgs.empty()) @@ -194,10 +194,10 @@ static void opAddFixed(Strings opFlags, Strings opArgs) /* Hack to support caching in `nix-prefetch-url'. */ static void opPrintFixedPath(Strings opFlags, Strings opArgs) { - bool recursive = false; + FileIngestionMethod recursive = FileIngestionMethod::Flat; for (auto i : opFlags) - if (i == "--recursive") recursive = true; + if (i == "--recursive") recursive = FileIngestionMethod::Recursive; else throw UsageError(format("unknown flag '%1%'") % i); if (opArgs.size() != 3) diff --git a/src/nix/add-to-store.cc b/src/nix/add-to-store.cc index 139db3657..e82ab844e 100644 --- a/src/nix/add-to-store.cc +++ b/src/nix/add-to-store.cc @@ -42,10 +42,10 @@ struct CmdAddToStore : MixDryRun, StoreCommand auto narHash = hashString(htSHA256, *sink.s); - ValidPathInfo info(store->makeFixedOutputPath(true, narHash, *namePart)); + ValidPathInfo info(store->makeFixedOutputPath(FileIngestionMethod::Recursive, narHash, *namePart)); info.narHash = narHash; info.narSize = sink.s->size(); - info.ca = makeFixedOutputCA(true, info.narHash); + info.ca = makeFixedOutputCA(FileIngestionMethod::Recursive, info.narHash); if (!dryRun) store->addToStore(info, sink.s); diff --git a/src/nix/command.cc b/src/nix/command.cc index 442bc6c53..fce6c391c 100644 --- a/src/nix/command.cc +++ b/src/nix/command.cc @@ -28,20 +28,20 @@ void StoreCommand::run() run(getStore()); } -StorePathsCommand::StorePathsCommand(bool recursive) +StorePathsCommand::StorePathsCommand(FileIngestionMethod recursive) : recursive(recursive) { - if (recursive) + if (recursive == FileIngestionMethod::Recursive) mkFlag() .longName("no-recursive") .description("apply operation to specified paths only") - .set(&this->recursive, false); + .set(&this->recursive, FileIngestionMethod::Flat); else mkFlag() .longName("recursive") .shortName('r') .description("apply operation to closure of the specified paths") - .set(&this->recursive, true); + .set(&this->recursive, FileIngestionMethod::Recursive); mkFlag(0, "all", "apply operation to the entire store", &all); } @@ -61,7 +61,7 @@ void StorePathsCommand::run(ref store) for (auto & p : toStorePaths(store, realiseMode, installables)) storePaths.push_back(p.clone()); - if (recursive) { + if (recursive == FileIngestionMethod::Recursive) { StorePathSet closure; store->computeFSClosure(storePathsToSet(storePaths), closure, false, false); storePaths.clear(); diff --git a/src/nix/command.hh b/src/nix/command.hh index a954a7d04..4cdda9f79 100644 --- a/src/nix/command.hh +++ b/src/nix/command.hh @@ -114,7 +114,7 @@ struct StorePathsCommand : public InstallablesCommand { private: - bool recursive = false; + FileIngestionMethod recursive = FileIngestionMethod::Flat; bool all = false; protected: @@ -123,7 +123,7 @@ protected: public: - StorePathsCommand(bool recursive = false); + StorePathsCommand(FileIngestionMethod recursive = FileIngestionMethod::Flat); using StoreCommand::run; diff --git a/src/nix/copy.cc b/src/nix/copy.cc index 85c777d38..a8ee3fce3 100644 --- a/src/nix/copy.cc +++ b/src/nix/copy.cc @@ -17,7 +17,7 @@ struct CmdCopy : StorePathsCommand SubstituteFlag substitute = NoSubstitute; CmdCopy() - : StorePathsCommand(true) + : StorePathsCommand(FileIngestionMethod::Recursive) { mkFlag() .longName("from") diff --git a/src/nix/make-content-addressable.cc b/src/nix/make-content-addressable.cc index f9c7fef3f..93eddbb1f 100644 --- a/src/nix/make-content-addressable.cc +++ b/src/nix/make-content-addressable.cc @@ -74,12 +74,12 @@ struct CmdMakeContentAddressable : StorePathsCommand, MixJSON auto narHash = hashModuloSink.finish().first; - ValidPathInfo info(store->makeFixedOutputPath(true, narHash, path.name(), references, hasSelfReference)); + ValidPathInfo info(store->makeFixedOutputPath(FileIngestionMethod::Recursive, narHash, path.name(), references, hasSelfReference)); info.references = std::move(references); if (hasSelfReference) info.references.insert(info.path.clone()); info.narHash = narHash; info.narSize = sink.s->size(); - info.ca = makeFixedOutputCA(true, info.narHash); + info.ca = makeFixedOutputCA(FileIngestionMethod::Recursive, info.narHash); if (!json) printError("rewrote '%s' to '%s'", pathS, store->printStorePath(info.path)); From bbbb7c1bc7a66e1169931cda8bd863b68236726a Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 30 Mar 2020 18:15:55 -0400 Subject: [PATCH 02/58] Use `auto` with some `FileIngestionMethod` local variables --- src/libexpr/primops.cc | 2 +- src/nix-store/nix-store.cc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 7d45733f4..71a511c41 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -563,7 +563,7 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * std::optional outputHash; std::string outputHashAlgo; - FileIngestionMethod outputHashRecursive = FileIngestionMethod::Flat; + auto outputHashRecursive = FileIngestionMethod::Flat; StringSet outputs; outputs.insert("out"); diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index 9a26eb57f..cde7d561b 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -174,7 +174,7 @@ static void opAdd(Strings opFlags, Strings opArgs) store. */ static void opAddFixed(Strings opFlags, Strings opArgs) { - FileIngestionMethod recursive = FileIngestionMethod::Flat; + auto recursive = FileIngestionMethod::Flat; for (auto & i : opFlags) if (i == "--recursive") recursive = FileIngestionMethod::Recursive; @@ -194,7 +194,7 @@ static void opAddFixed(Strings opFlags, Strings opArgs) /* Hack to support caching in `nix-prefetch-url'. */ static void opPrintFixedPath(Strings opFlags, Strings opArgs) { - FileIngestionMethod recursive = FileIngestionMethod::Flat; + auto recursive = FileIngestionMethod::Flat; for (auto i : opFlags) if (i == "--recursive") recursive = FileIngestionMethod::Recursive; From 51afea3af23640a3fb2cfe8cfae5cf3cc27eb3a1 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 30 Mar 2020 22:31:51 +0000 Subject: [PATCH 03/58] Never cast `FileIngestionMethod` to or from boolean --- src/libexpr/primops.cc | 9 ++++++--- src/libstore/build.cc | 10 +++++++--- src/libstore/path.hh | 2 +- src/libstore/store-api.cc | 21 ++++++++++++++------- 4 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 71a511c41..4d5aaf86a 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -724,9 +724,12 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * auto outPath = state.store->makeFixedOutputPath(outputHashRecursive, h, drvName); if (!jsonObject) drv.env["out"] = state.store->printStorePath(outPath); - drv.outputs.insert_or_assign("out", DerivationOutput(std::move(outPath), - (static_cast(outputHashRecursive) ? "r:" : "") + printHashType(h.type), - h.to_string(Base16, false))); + drv.outputs.insert_or_assign("out", DerivationOutput { + std::move(outPath), + (outputHashRecursive == FileIngestionMethod::Recursive ? "r:" : "") + + printHashType(h.type), + h.to_string(Base16, false), + }); } else { diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 224633106..be52e637e 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -3650,7 +3650,7 @@ void DerivationGoal::registerOutputs() FileIngestionMethod recursive; Hash h; i.second.parseHashInfo(recursive, h); - if (!static_cast(recursive)) { + if (recursive == FileIngestionMethod::Flat) { /* The output path should be a regular file without execute permission. */ if (!S_ISREG(st.st_mode) || (st.st_mode & S_IXUSR) != 0) throw BuildError( @@ -3659,7 +3659,9 @@ void DerivationGoal::registerOutputs() /* Check the hash. In hash mode, move the path produced by the derivation to its content-addressed location. */ - Hash h2 = static_cast(recursive) ? hashPath(h.type, actualPath).first : hashFile(h.type, actualPath); + Hash h2 = recursive == FileIngestionMethod::Recursive + ? hashPath(h.type, actualPath).first + : hashFile(h.type, actualPath); auto dest = worker.store.makeFixedOutputPath(recursive, h2, i.second.path.name()); @@ -3912,7 +3914,9 @@ void DerivationGoal::checkOutputs(const std::map & outputs) auto spec = parseReferenceSpecifiers(worker.store, *drv, *value); - auto used = static_cast(recursive) ? cloneStorePathSet(getClosure(info.path).first) : cloneStorePathSet(info.references); + auto used = recursive + ? cloneStorePathSet(getClosure(info.path).first) + : cloneStorePathSet(info.references); if (recursive && checks.ignoreSelfRefs) used.erase(info.path); diff --git a/src/libstore/path.hh b/src/libstore/path.hh index 73c4b8e29..5122e7422 100644 --- a/src/libstore/path.hh +++ b/src/libstore/path.hh @@ -73,7 +73,7 @@ const size_t storePathHashLen = 32; // i.e. 160 bits /* Extension of derivations in the Nix store. */ const std::string drvExtension = ".drv"; -enum struct FileIngestionMethod : bool { +enum struct FileIngestionMethod : uint8_t { Flat = false, Recursive = true }; diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 06aa0883c..ca1409588 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -177,13 +177,16 @@ StorePath Store::makeFixedOutputPath( const StorePathSet & references, bool hasSelfReference) const { - if (hash.type == htSHA256 && recursive == FileIngestionMethod::Recursive) { + if (hash.type == htSHA256 && recursive == FileIngestionMethod::Recursive) { return makeStorePath(makeType(*this, "source", references, hasSelfReference), hash, name); } else { assert(references.empty()); - return makeStorePath("output:out", hashString(htSHA256, - "fixed:out:" + (static_cast(recursive) ? (string) "r:" : "") + - hash.to_string(Base16) + ":"), name); + return makeStorePath("output:out", + hashString(htSHA256, + "fixed:out:" + + (recursive == FileIngestionMethod::Recursive ? (string) "r:" : "") + + hash.to_string(Base16) + ":"), + name); } } @@ -202,7 +205,9 @@ StorePath Store::makeTextPath(std::string_view name, const Hash & hash, std::pair Store::computeStorePathForPath(std::string_view name, const Path & srcPath, FileIngestionMethod recursive, HashType hashAlgo, PathFilter & filter) const { - Hash h = static_cast(recursive) ? hashPath(hashAlgo, srcPath, filter).first : hashFile(hashAlgo, srcPath); + Hash h = recursive == FileIngestionMethod::Recursive + ? hashPath(hashAlgo, srcPath, filter).first + : hashFile(hashAlgo, srcPath); return std::make_pair(makeFixedOutputPath(recursive, h, name), h); } @@ -782,7 +787,7 @@ bool ValidPathInfo::isContentAddressed(const Store & store) const else if (hasPrefix(ca, "fixed:")) { FileIngestionMethod recursive { ca.compare(6, 2, "r:") == 0 }; - Hash hash(std::string(ca, static_cast(recursive) ? 8 : 6)); + Hash hash(std::string(ca, recursive == FileIngestionMethod::Recursive ? 8 : 6)); auto refs = cloneStorePathSet(references); bool hasSelfReference = false; if (refs.count(path)) { @@ -828,7 +833,9 @@ Strings ValidPathInfo::shortRefs() const std::string makeFixedOutputCA(FileIngestionMethod recursive, const Hash & hash) { - return "fixed:" + (static_cast(recursive) ? (std::string) "r:" : "") + hash.to_string(); + return "fixed:" + + (recursive == FileIngestionMethod::Recursive ? (std::string) "r:" : "") + + hash.to_string(); } From 7e9a2718f0c39fde53a3447c3b7f3617a4a9b958 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 30 Mar 2020 22:36:15 +0000 Subject: [PATCH 04/58] s/outputHashRecursive/ingestionMethod/c --- src/libexpr/primops.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 4d5aaf86a..415e77fe7 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -563,7 +563,7 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * std::optional outputHash; std::string outputHashAlgo; - auto outputHashRecursive = FileIngestionMethod::Flat; + auto ingestionMethod = FileIngestionMethod::Flat; StringSet outputs; outputs.insert("out"); @@ -574,8 +574,8 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * vomit("processing attribute '%1%'", key); auto handleHashMode = [&](const std::string & s) { - if (s == "recursive") outputHashRecursive = FileIngestionMethod::Recursive; - else if (s == "flat") outputHashRecursive = FileIngestionMethod::Flat; + if (s == "recursive") ingestionMethod = FileIngestionMethod::Recursive; + else if (s == "flat") ingestionMethod = FileIngestionMethod::Flat; else throw EvalError("invalid value '%s' for 'outputHashMode' attribute, at %s", s, posDrvName); }; @@ -722,11 +722,11 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * HashType ht = outputHashAlgo.empty() ? htUnknown : parseHashType(outputHashAlgo); Hash h(*outputHash, ht); - auto outPath = state.store->makeFixedOutputPath(outputHashRecursive, h, drvName); + auto outPath = state.store->makeFixedOutputPath(ingestionMethod, h, drvName); if (!jsonObject) drv.env["out"] = state.store->printStorePath(outPath); drv.outputs.insert_or_assign("out", DerivationOutput { std::move(outPath), - (outputHashRecursive == FileIngestionMethod::Recursive ? "r:" : "") + (ingestionMethod == FileIngestionMethod::Recursive ? "r:" : "") + printHashType(h.type), h.to_string(Base16, false), }); From 8aa46cd340c1294c3d06cd52f85c906bdf749070 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 30 Mar 2020 22:40:41 +0000 Subject: [PATCH 05/58] Get rid of FileIngestionMethod casts in perl bindings, too --- perl/lib/Nix/Store.xs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/perl/lib/Nix/Store.xs b/perl/lib/Nix/Store.xs index eab8ccacb..890310b3e 100644 --- a/perl/lib/Nix/Store.xs +++ b/perl/lib/Nix/Store.xs @@ -274,7 +274,8 @@ int checkSignature(SV * publicKey_, SV * sig_, char * msg) SV * addToStore(char * srcPath, int recursive, char * algo) PPCODE: try { - auto path = store()->addToStore(std::string(baseNameOf(srcPath)), srcPath, (FileIngestionMethod) recursive, parseHashType(algo)); + auto method = recursive ? FileIngestionMethod::Recursive : FileIngestionMethod::Flat; + auto path = store()->addToStore(std::string(baseNameOf(srcPath)), srcPath, method, parseHashType(algo)); XPUSHs(sv_2mortal(newSVpv(store()->printStorePath(path).c_str(), 0))); } catch (Error & e) { croak("%s", e.what()); @@ -285,7 +286,8 @@ SV * makeFixedOutputPath(int recursive, char * algo, char * hash, char * name) PPCODE: try { Hash h(hash, parseHashType(algo)); - auto path = store()->makeFixedOutputPath((FileIngestionMethod) recursive, h, name); + auto method = recursive ? FileIngestionMethod::Recursive : FileIngestionMethod::Flat; + auto path = store()->makeFixedOutputPath(method, h, name); XPUSHs(sv_2mortal(newSVpv(store()->printStorePath(path).c_str(), 0))); } catch (Error & e) { croak("%s", e.what()); From 04967dee9d08befe4661e6fa5da4a00da0538a13 Mon Sep 17 00:00:00 2001 From: Alexander Bantyev Date: Tue, 5 May 2020 13:04:36 +0300 Subject: [PATCH 06/58] Wait for build users when none are available --- src/libstore/build.cc | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 147093fae..1b7e6d75e 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -507,6 +507,9 @@ private: Path fnUserLock; AutoCloseFD fdUserLock; + bool findFreeUser(); + + string user; uid_t uid; gid_t gid; @@ -526,10 +529,19 @@ public: }; - UserLock::UserLock() { assert(settings.buildUsersGroup != ""); + createDirs(settings.nixStateDir + "/userpool"); + + if (findFreeUser()) return; + + printError("waiting for build users"); + + do std::this_thread::sleep_for(std::chrono::seconds(2)); while (! findFreeUser()); +} + +bool UserLock::findFreeUser() { /* Get the members of the build-users-group. */ struct group * gr = getgrnam(settings.buildUsersGroup.get().c_str()); @@ -559,7 +571,6 @@ UserLock::UserLock() throw Error(format("the user '%1%' in the group '%2%' does not exist") % i % settings.buildUsersGroup); - createDirs(settings.nixStateDir + "/userpool"); fnUserLock = (format("%1%/userpool/%2%") % settings.nixStateDir % pw->pw_uid).str(); @@ -590,16 +601,12 @@ UserLock::UserLock() supplementaryGIDs.resize(ngroups); #endif - return; + return true; } } - - throw Error(format("all build users are currently in use; " - "consider creating additional users and adding them to the '%1%' group") - % settings.buildUsersGroup); + return false; } - void UserLock::kill() { killUser(uid); From 14073fb76be0f46cb9335edf747fcfa1a5275b7b Mon Sep 17 00:00:00 2001 From: Alexander Bantyev Date: Fri, 8 May 2020 12:22:39 +0300 Subject: [PATCH 07/58] Don't block while waiting for build users --- src/libstore/build.cc | 54 +++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 1b7e6d75e..00101f553 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -507,9 +507,6 @@ private: Path fnUserLock; AutoCloseFD fdUserLock; - bool findFreeUser(); - - string user; uid_t uid; gid_t gid; @@ -525,20 +522,19 @@ public: uid_t getGID() { assert(gid); return gid; } std::vector getSupplementaryGIDs() { return supplementaryGIDs; } + bool findFreeUser(); + bool enabled() { return uid != 0; } }; + UserLock::UserLock() { assert(settings.buildUsersGroup != ""); createDirs(settings.nixStateDir + "/userpool"); - - if (findFreeUser()) return; - - printError("waiting for build users"); - - do std::this_thread::sleep_for(std::chrono::seconds(2)); while (! findFreeUser()); + /* Mark that user is not enabled by default */ + uid = 0; } bool UserLock::findFreeUser() { @@ -1398,6 +1394,30 @@ void DerivationGoal::tryToBuild() { trace("trying to build"); + /* 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) { +#if defined(__linux__) || defined(__APPLE__) + if (!buildUser) buildUser = std::make_unique(); + + if (!buildUser->enabled()) { + if (!buildUser->findFreeUser()) { + debug("waiting for build users"); + worker.waitForAWhile(shared_from_this()); + return; + } + + /* Make sure that no other processes are executing under this + uid. */ + buildUser->kill(); + } +#else + /* Don't know how to block the creation of setuid/setgid + binaries on this platform. */ + throw Error("build users are not supported on this platform for security reasons"); +#endif + } + /* Obtain locks on all output paths. The locks are automatically released when we exit this function or Nix crashes. If we can't acquire the lock, then continue; hopefully some other @@ -1950,22 +1970,6 @@ void DerivationGoal::startBuilder() #endif } - /* 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) { -#if defined(__linux__) || defined(__APPLE__) - buildUser = std::make_unique(); - - /* Make sure that no other processes are executing under this - uid. */ - buildUser->kill(); -#else - /* Don't know how to block the creation of setuid/setgid - binaries on this platform. */ - throw Error("build users are not supported on this platform for security reasons"); -#endif - } - /* Create a temporary directory where the build will take place. */ tmpDir = createTempDir("", "nix-build-" + std::string(drvPath.name()), false, false, 0700); From 772e5db828c9924c803b4d126bca72d7e123614b Mon Sep 17 00:00:00 2001 From: Alexander Bantyev Date: Fri, 8 May 2020 12:29:00 +0300 Subject: [PATCH 08/58] Mention build users in the 'waiting for' message --- 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 00101f553..a8c1cd565 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -4830,7 +4830,7 @@ void Worker::waitForInput() if (!waitingForAWhile.empty()) { useTimeout = true; if (lastWokenUp == steady_time_point::min()) - printError("waiting for locks or build slots..."); + printError("waiting for locks, build slots or build users..."); if (lastWokenUp == steady_time_point::min() || lastWokenUp > before) lastWokenUp = before; timeout = std::max(1L, (long) std::chrono::duration_cast( From 183dd28266e0292116da6cfbc3e2643577adbfbe Mon Sep 17 00:00:00 2001 From: Alexander Bantyev Date: Thu, 14 May 2020 17:00:54 +0300 Subject: [PATCH 09/58] Don't lock a user while doing remote builds --- src/libstore/build.cc | 88 ++++++++++++++++++++++++------------------- 1 file changed, 49 insertions(+), 39 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index a8c1cd565..f7fc23b35 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -507,6 +507,7 @@ private: Path fnUserLock; AutoCloseFD fdUserLock; + bool isEnabled; string user; uid_t uid; gid_t gid; @@ -524,7 +525,7 @@ public: bool findFreeUser(); - bool enabled() { return uid != 0; } + bool enabled() { return isEnabled; } }; @@ -535,9 +536,11 @@ UserLock::UserLock() createDirs(settings.nixStateDir + "/userpool"); /* Mark that user is not enabled by default */ uid = 0; + isEnabled = false; } bool UserLock::findFreeUser() { + if (enabled()) return true; /* Get the members of the build-users-group. */ struct group * gr = getgrnam(settings.buildUsersGroup.get().c_str()); @@ -597,6 +600,7 @@ bool UserLock::findFreeUser() { supplementaryGIDs.resize(ngroups); #endif + isEnabled = true; return true; } } @@ -931,6 +935,7 @@ private: void closureRepaired(); void inputsRealised(); void tryToBuild(); + void tryLocalBuild(); void buildDone(); /* Is the build hook willing to perform the build? */ @@ -1002,6 +1007,8 @@ private: Goal::amDone(result); } + void started(); + void done(BuildResult::Status status, const string & msg = ""); StorePathSet exportReferences(const StorePathSet & storePaths); @@ -1389,35 +1396,24 @@ void DerivationGoal::inputsRealised() result = BuildResult(); } +void DerivationGoal::started() { + auto msg = fmt( + buildMode == bmRepair ? "repairing outputs of '%s'" : + buildMode == bmCheck ? "checking outputs of '%s'" : + nrRounds > 1 ? "building '%s' (round %d/%d)" : + "building '%s'", worker.store.printStorePath(drvPath), curRound, nrRounds); + fmt("building '%s'", worker.store.printStorePath(drvPath)); + if (hook) msg += fmt(" on '%s'", machineName); + act = std::make_unique(*logger, lvlInfo, actBuild, msg, + Logger::Fields{worker.store.printStorePath(drvPath), hook ? machineName : "", curRound, nrRounds}); + mcRunningBuilds = std::make_unique>(worker.runningBuilds); + worker.updateProgress(); +} void DerivationGoal::tryToBuild() { trace("trying to build"); - /* 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) { -#if defined(__linux__) || defined(__APPLE__) - if (!buildUser) buildUser = std::make_unique(); - - if (!buildUser->enabled()) { - if (!buildUser->findFreeUser()) { - debug("waiting for build users"); - worker.waitForAWhile(shared_from_this()); - return; - } - - /* Make sure that no other processes are executing under this - uid. */ - buildUser->kill(); - } -#else - /* Don't know how to block the creation of setuid/setgid - binaries on this platform. */ - throw Error("build users are not supported on this platform for security reasons"); -#endif - } - /* Obtain locks on all output paths. The locks are automatically released when we exit this function or Nix crashes. If we can't acquire the lock, then continue; hopefully some other @@ -1464,20 +1460,6 @@ void DerivationGoal::tryToBuild() supported for local builds. */ bool buildLocally = buildMode != bmNormal || parsedDrv->willBuildLocally(); - auto started = [&]() { - auto msg = fmt( - buildMode == bmRepair ? "repairing outputs of '%s'" : - buildMode == bmCheck ? "checking outputs of '%s'" : - nrRounds > 1 ? "building '%s' (round %d/%d)" : - "building '%s'", worker.store.printStorePath(drvPath), curRound, nrRounds); - fmt("building '%s'", worker.store.printStorePath(drvPath)); - if (hook) msg += fmt(" on '%s'", machineName); - act = std::make_unique(*logger, lvlInfo, actBuild, msg, - Logger::Fields{worker.store.printStorePath(drvPath), hook ? machineName : "", curRound, nrRounds}); - mcRunningBuilds = std::make_unique>(worker.runningBuilds); - worker.updateProgress(); - }; - /* Is the build hook willing to accept this job? */ if (!buildLocally) { switch (tryBuildHook()) { @@ -1510,6 +1492,34 @@ 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) { +#if defined(__linux__) || defined(__APPLE__) + if (!buildUser) buildUser = std::make_unique(); + + if (buildUser->findFreeUser()) { + /* Make sure that no other processes are executing under this + uid. */ + buildUser->kill(); + } else { + debug("waiting for build users"); + worker.waitForAWhile(shared_from_this()); + return; + } +#else + /* Don't know how to block the creation of setuid/setgid + binaries on this platform. */ + throw Error("build users are not supported on this platform for security reasons"); +#endif + } + try { /* Okay, we have to build. */ From e223eeac09c6468db80ee0497c84960b4ec73e02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Sat, 16 May 2020 08:40:55 +0100 Subject: [PATCH 10/58] Remove -j option from simple-build-testing By default Nix/NixOS already set a reasonable default `max-jobs = auto` so we don't need to mention it in this tutorial. The option is still documented in other parts of the documentation if users ever stumble over this. Fixes https://github.com/NixOS/nix/issues/2531 --- doc/manual/expressions/simple-building-testing.xml | 8 -------- 1 file changed, 8 deletions(-) diff --git a/doc/manual/expressions/simple-building-testing.xml b/doc/manual/expressions/simple-building-testing.xml index 7326a3e76..ce0a1636d 100644 --- a/doc/manual/expressions/simple-building-testing.xml +++ b/doc/manual/expressions/simple-building-testing.xml @@ -73,12 +73,4 @@ waiting for lock on `/nix/store/0h5b7hp8d4hqfrw8igvx97x1xawrjnac-hello-2.1.1x'make). -If you have a system with multiple CPUs, you may want to have -Nix build different derivations in parallel (insofar as possible). -Just pass the option , where -N is the maximum number of jobs to be run -in parallel, or set. Typically this should be the number of -CPUs. - From 5ef64f05e6e493d5fe69c87127328f68500b9e50 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 18 May 2020 15:50:29 +0200 Subject: [PATCH 11/58] Cleanup --- src/libstore/build.cc | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 4f2f38d63..a2b16f95c 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -507,10 +507,10 @@ private: Path fnUserLock; AutoCloseFD fdUserLock; - bool isEnabled; + bool isEnabled = false; string user; - uid_t uid; - gid_t gid; + uid_t uid = 0; + gid_t gid = 0; std::vector supplementaryGIDs; public: @@ -534,9 +534,6 @@ UserLock::UserLock() { assert(settings.buildUsersGroup != ""); createDirs(settings.nixStateDir + "/userpool"); - /* Mark that user is not enabled by default */ - uid = 0; - isEnabled = false; } bool UserLock::findFreeUser() { From a73a820a5d73e0c595ff807a752f76ef1184ca2d Mon Sep 17 00:00:00 2001 From: Tobias Pflug Date: Wed, 20 May 2020 16:27:53 +0200 Subject: [PATCH 12/58] Add unit testes for url.cc This adds tests for - parseURL - percentDecode - decodeQuery --- src/libutil/tests/tests.cc | 6 +- src/libutil/tests/url.cc | 266 +++++++++++++++++++++++++++++++++++++ 2 files changed, 271 insertions(+), 1 deletion(-) create mode 100644 src/libutil/tests/url.cc diff --git a/src/libutil/tests/tests.cc b/src/libutil/tests/tests.cc index 5abfe2c9c..8e77ccbe1 100644 --- a/src/libutil/tests/tests.cc +++ b/src/libutil/tests/tests.cc @@ -5,6 +5,8 @@ namespace nix { +/* ----------- tests for util.hh ------------------------------------------------*/ + /* ---------------------------------------------------------------------------- * absPath * --------------------------------------------------------------------------*/ @@ -15,6 +17,9 @@ namespace nix { ASSERT_EQ(p, "/"); } + + + TEST(absPath, turnsEmptyPathIntoCWD) { char cwd[PATH_MAX+1]; auto p = absPath(""); @@ -581,5 +586,4 @@ namespace nix { ASSERT_EQ(filterANSIEscapes(s, true), "foo bar baz" ); } - } diff --git a/src/libutil/tests/url.cc b/src/libutil/tests/url.cc new file mode 100644 index 000000000..80646ad3e --- /dev/null +++ b/src/libutil/tests/url.cc @@ -0,0 +1,266 @@ +#include "url.hh" +#include + +namespace nix { + +/* ----------- tests for url.hh --------------------------------------------------*/ + + string print_map(std::map m) { + std::map::iterator it; + string s = "{ "; + for (it = m.begin(); it != m.end(); ++it) { + s += "{ "; + s += it->first; + s += " = "; + s += it->second; + s += " } "; + } + s += "}"; + return s; + } + + + std::ostream& operator<<(std::ostream& os, const ParsedURL& p) { + return os << "\n" + << "url: " << p.url << "\n" + << "base: " << p.base << "\n" + << "scheme: " << p.scheme << "\n" + << "authority: " << p.authority.value() << "\n" + << "path: " << p.path << "\n" + << "query: " << print_map(p.query) << "\n" + << "fragment: " << p.fragment << "\n"; + } + + TEST(parseURL, parsesSimpleHttpUrl) { + auto s = "http://www.example.org/file.tar.gz"; + auto parsed = parseURL(s); + + ParsedURL expected { + .url = "http://www.example.org/file.tar.gz", + .base = "http://www.example.org/file.tar.gz", + .scheme = "http", + .authority = "www.example.org", + .path = "/file.tar.gz", + .query = (StringMap) { }, + .fragment = "", + }; + + ASSERT_EQ(parsed, expected); + } + + TEST(parseURL, parsesSimpleHttpsUrl) { + auto s = "https://www.example.org/file.tar.gz"; + auto parsed = parseURL(s); + + ParsedURL expected { + .url = "https://www.example.org/file.tar.gz", + .base = "https://www.example.org/file.tar.gz", + .scheme = "https", + .authority = "www.example.org", + .path = "/file.tar.gz", + .query = (StringMap) { }, + .fragment = "", + }; + + ASSERT_EQ(parsed, expected); + } + + TEST(parseURL, parsesSimpleHttpUrlWithQueryAndFragment) { + auto s = "https://www.example.org/file.tar.gz?download=fast&when=now#hello"; + auto parsed = parseURL(s); + + ParsedURL expected { + .url = "https://www.example.org/file.tar.gz", + .base = "https://www.example.org/file.tar.gz", + .scheme = "https", + .authority = "www.example.org", + .path = "/file.tar.gz", + .query = (StringMap) { { "download", "fast" }, { "when", "now" } }, + .fragment = "hello", + }; + + ASSERT_EQ(parsed, expected); + } + + TEST(parseURL, parsesSimpleHttpUrlWithComplexFragment) { + auto s = "http://www.example.org/file.tar.gz?field=value#?foo=bar%23"; + auto parsed = parseURL(s); + + ParsedURL expected { + .url = "http://www.example.org/file.tar.gz", + .base = "http://www.example.org/file.tar.gz", + .scheme = "http", + .authority = "www.example.org", + .path = "/file.tar.gz", + .query = (StringMap) { { "field", "value" } }, + .fragment = "?foo=bar#", + }; + + ASSERT_EQ(parsed, expected); + } + + + TEST(parseURL, parseIPv4Address) { + auto s = "http://127.0.0.1:8080/file.tar.gz?download=fast&when=now#hello"; + auto parsed = parseURL(s); + + ParsedURL expected { + .url = "http://127.0.0.1:8080/file.tar.gz", + .base = "https://127.0.0.1:8080/file.tar.gz", + .scheme = "http", + .authority = "127.0.0.1:8080", + .path = "/file.tar.gz", + .query = (StringMap) { { "download", "fast" }, { "when", "now" } }, + .fragment = "hello", + }; + + ASSERT_EQ(parsed, expected); + } + + TEST(parseURL, parseIPv6Address) { + auto s = "http://[2a02:8071:8192:c100:311d:192d:81ac:11ea]:8080"; + auto parsed = parseURL(s); + + ParsedURL expected { + .url = "http://[2a02:8071:8192:c100:311d:192d:81ac:11ea]:8080", + .base = "http://[2a02:8071:8192:c100:311d:192d:81ac:11ea]:8080", + .scheme = "http", + .authority = "[2a02:8071:8192:c100:311d:192d:81ac:11ea]:8080", + .path = "", + .query = (StringMap) { }, + .fragment = "", + }; + + ASSERT_EQ(parsed, expected); + + } + + TEST(parseURL, parseEmptyQueryParams) { + auto s = "http://127.0.0.1:8080/file.tar.gz?&&&&&"; + auto parsed = parseURL(s); + ASSERT_EQ(parsed.query, (StringMap) { }); + } + + TEST(parseURL, parseUserPassword) { + auto s = "http://user:pass@www.example.org:8080/file.tar.gz"; + auto parsed = parseURL(s); + + ParsedURL expected { + .url = "http://user:pass@www.example.org/file.tar.gz", + .base = "http://user:pass@www.example.org/file.tar.gz", + .scheme = "http", + .authority = "user:pass@www.example.org:8080", + .path = "/file.tar.gz", + .query = (StringMap) { }, + .fragment = "", + }; + + + ASSERT_EQ(parsed, expected); + } + + TEST(parseURL, parseFileURLWithQueryAndFragment) { + auto s = "file:///none/of/your/business"; + auto parsed = parseURL(s); + + ParsedURL expected { + .url = "", + .base = "", + .scheme = "file", + .authority = "", + .path = "/none/of/your/business", + .query = (StringMap) { }, + .fragment = "", + }; + + ASSERT_EQ(parsed, expected); + + } + + TEST(parseURL, parsedUrlsIsEqualToItself) { + auto s = "http://www.example.org/file.tar.gz"; + auto url = parseURL(s); + + ASSERT_TRUE(url == url); + } + + TEST(parseURL, parseFTPUrl) { + auto s = "ftp://ftp.nixos.org/downloads/nixos.iso"; + auto parsed = parseURL(s); + + ParsedURL expected { + .url = "ftp://ftp.nixos.org/downloads/nixos.iso", + .base = "ftp://ftp.nixos.org/downloads/nixos.iso", + .scheme = "ftp", + .authority = "ftp.nixos.org", + .path = "/downloads/nixos.iso", + .query = (StringMap) { }, + .fragment = "", + }; + + ASSERT_EQ(parsed, expected); + } + + TEST(parseURL, parsesAnythingInUriFormat) { + auto s = "whatever://github.com/NixOS/nixpkgs.git"; + auto parsed = parseURL(s); + } + + TEST(parseURL, parsesAnythingInUriFormatWithoutDoubleSlash) { + auto s = "whatever:github.com/NixOS/nixpkgs.git"; + auto parsed = parseURL(s); + } + + TEST(parseURL, emptyStringIsInvalidURL) { + ASSERT_THROW(parseURL(""), Error); + } + + /* ---------------------------------------------------------------------------- + * decodeQuery + * --------------------------------------------------------------------------*/ + + TEST(decodeQuery, emptyStringYieldsEmptyMap) { + auto d = decodeQuery(""); + ASSERT_EQ(d, (StringMap) { }); + } + + TEST(decodeQuery, simpleDecode) { + auto d = decodeQuery("yi=one&er=two"); + ASSERT_EQ(d, ((StringMap) { { "yi", "one" }, { "er", "two" } })); + } + + TEST(decodeQuery, decodeUrlEncodedArgs) { + auto d = decodeQuery("arg=%3D%3D%40%3D%3D"); + ASSERT_EQ(d, ((StringMap) { { "arg", "==@==" } })); + } + + TEST(decodeQuery, decodeArgWithEmptyValue) { + auto d = decodeQuery("arg="); + ASSERT_EQ(d, ((StringMap) { { "arg", ""} })); + } + + /* ---------------------------------------------------------------------------- + * percentDecode + * --------------------------------------------------------------------------*/ + + TEST(percentDecode, decodesUrlEncodedString) { + string s = "==@=="; + string d = percentDecode("%3D%3D%40%3D%3D"); + ASSERT_EQ(d, s); + } + + TEST(percentDecode, multipleDecodesAreIdempotent) { + string once = percentDecode("%3D%3D%40%3D%3D"); + string twice = percentDecode(once); + + ASSERT_EQ(once, twice); + } + + TEST(percentDecode, trailingPercent) { + string s = "==@==%"; + string d = percentDecode("%3D%3D%40%3D%3D%25"); + + ASSERT_EQ(d, s); + } + +} From c8cb558849da8ef88f15cc2a70c570f1f5013a30 Mon Sep 17 00:00:00 2001 From: Krzysztof Gogolewski Date: Thu, 21 May 2020 19:29:13 +0200 Subject: [PATCH 13/58] documentation: avoid unquoted URLs --- doc/manual/command-ref/conf-file.xml | 2 +- doc/manual/expressions/advanced-attributes.xml | 4 ++-- doc/manual/expressions/builtins.xml | 4 ++-- doc/manual/expressions/expression-syntax.xml | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/doc/manual/command-ref/conf-file.xml b/doc/manual/command-ref/conf-file.xml index 1820598e5..1fa74a143 100644 --- a/doc/manual/command-ref/conf-file.xml +++ b/doc/manual/command-ref/conf-file.xml @@ -386,7 +386,7 @@ false. builtins.fetchurl { - url = https://example.org/foo-1.2.3.tar.xz; + url = "https://example.org/foo-1.2.3.tar.xz"; sha256 = "2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae"; } diff --git a/doc/manual/expressions/advanced-attributes.xml b/doc/manual/expressions/advanced-attributes.xml index 372d03de7..5759ff50e 100644 --- a/doc/manual/expressions/advanced-attributes.xml +++ b/doc/manual/expressions/advanced-attributes.xml @@ -178,7 +178,7 @@ impureEnvVars = [ "http_proxy" "https_proxy" ... ]; fetchurl { - url = http://ftp.gnu.org/pub/gnu/hello/hello-2.1.1.tar.gz; + url = "http://ftp.gnu.org/pub/gnu/hello/hello-2.1.1.tar.gz"; sha256 = "1md7jsfd8pa45z73bz1kszpp01yw6x5ljkjk2hx7wl800any6465"; } @@ -189,7 +189,7 @@ fetchurl { fetchurl { - url = ftp://ftp.nluug.nl/pub/gnu/hello/hello-2.1.1.tar.gz; + url = "ftp://ftp.nluug.nl/pub/gnu/hello/hello-2.1.1.tar.gz"; sha256 = "1md7jsfd8pa45z73bz1kszpp01yw6x5ljkjk2hx7wl800any6465"; } diff --git a/doc/manual/expressions/builtins.xml b/doc/manual/expressions/builtins.xml index f71a8f3be..6709c4081 100644 --- a/doc/manual/expressions/builtins.xml +++ b/doc/manual/expressions/builtins.xml @@ -349,7 +349,7 @@ stdenv.mkDerivation { … } with import (fetchTarball { - url = https://github.com/NixOS/nixpkgs-channels/archive/nixos-14.12.tar.gz; + url = "https://github.com/NixOS/nixpkgs-channels/archive/nixos-14.12.tar.gz"; sha256 = "1jppksrfvbk5ypiqdz4cddxdl8z6zyzdb2srq8fcffr327ld5jj2"; }) {}; @@ -1406,7 +1406,7 @@ stdenv.mkDerivation { "; src = fetchurl { - url = http://ftp.nluug.nl/pub/gnu/hello/hello-2.1.1.tar.gz; + url = "http://ftp.nluug.nl/pub/gnu/hello/hello-2.1.1.tar.gz"; sha256 = "1md7jsfd8pa45z73bz1kszpp01yw6x5ljkjk2hx7wl800any6465"; }; inherit perl; diff --git a/doc/manual/expressions/expression-syntax.xml b/doc/manual/expressions/expression-syntax.xml index 42b9dca36..a3de20713 100644 --- a/doc/manual/expressions/expression-syntax.xml +++ b/doc/manual/expressions/expression-syntax.xml @@ -15,7 +15,7 @@ stdenv.mkDerivation { name = "hello-2.1.1"; builder = ./builder.sh; src = fetchurl { - url = ftp://ftp.nluug.nl/pub/gnu/hello/hello-2.1.1.tar.gz; + url = "ftp://ftp.nluug.nl/pub/gnu/hello/hello-2.1.1.tar.gz"; sha256 = "1md7jsfd8pa45z73bz1kszpp01yw6x5ljkjk2hx7wl800any6465"; }; inherit perl; From 0726ad5825f60e543d9cf535c62673685adbf5c8 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Sun, 15 Dec 2019 16:43:43 +0100 Subject: [PATCH 14/58] install: configure and bootstrap synthetic.conf on darwin Starting macOS 10.15 /nix can't be creasted directly anymore due to the readonly filesystem, but synthetic.conf was introduced to enable creating mountpoints or symlinks for special usecases like package managers. --- release.nix | 12 +++- scripts/create-darwin-volume.sh | 107 ++++++++++++++++++++++++++++ scripts/install-nix-from-closure.sh | 93 +++++++++++++++--------- 3 files changed, 176 insertions(+), 36 deletions(-) create mode 100755 scripts/create-darwin-volume.sh diff --git a/release.nix b/release.nix index f5729cee3..2cc4bb4c0 100644 --- a/release.nix +++ b/release.nix @@ -177,10 +177,10 @@ let } '' cp ${installerClosureInfo}/registration $TMPDIR/reginfo + cp ${./scripts/create-darwin-volume.sh} $TMPDIR/create-darwin-volume.sh substitute ${./scripts/install-nix-from-closure.sh} $TMPDIR/install \ --subst-var-by nix ${toplevel} \ --subst-var-by cacert ${cacert} - substitute ${./scripts/install-darwin-multi-user.sh} $TMPDIR/install-darwin-multi-user.sh \ --subst-var-by nix ${toplevel} \ --subst-var-by cacert ${cacert} @@ -195,6 +195,7 @@ let # SC1090: Don't worry about not being able to find # $nix/etc/profile.d/nix.sh shellcheck --exclude SC1090 $TMPDIR/install + shellcheck $TMPDIR/create-darwin-volume.sh shellcheck $TMPDIR/install-darwin-multi-user.sh shellcheck $TMPDIR/install-systemd-multi-user.sh @@ -210,6 +211,7 @@ let fi chmod +x $TMPDIR/install + chmod +x $TMPDIR/create-darwin-volume.sh chmod +x $TMPDIR/install-darwin-multi-user.sh chmod +x $TMPDIR/install-systemd-multi-user.sh chmod +x $TMPDIR/install-multi-user @@ -222,11 +224,15 @@ let --absolute-names \ --hard-dereference \ --transform "s,$TMPDIR/install,$dir/install," \ + --transform "s,$TMPDIR/create-darwin-volume.sh,$dir/create-darwin-volume.sh," \ --transform "s,$TMPDIR/reginfo,$dir/.reginfo," \ --transform "s,$NIX_STORE,$dir/store,S" \ - $TMPDIR/install $TMPDIR/install-darwin-multi-user.sh \ + $TMPDIR/install \ + $TMPDIR/create-darwin-volume.sh \ + $TMPDIR/install-darwin-multi-user.sh \ $TMPDIR/install-systemd-multi-user.sh \ - $TMPDIR/install-multi-user $TMPDIR/reginfo \ + $TMPDIR/install-multi-user \ + $TMPDIR/reginfo \ $(cat ${installerClosureInfo}/store-paths) ''); diff --git a/scripts/create-darwin-volume.sh b/scripts/create-darwin-volume.sh new file mode 100755 index 000000000..f3e0d46f1 --- /dev/null +++ b/scripts/create-darwin-volume.sh @@ -0,0 +1,107 @@ +#!/bin/sh +set -e + +root_disks() { + diskutil list -plist / +} + +apfs_volumes_for() { + disk=$1 + diskutil apfs list -plist "$disk" +} + +disk_identifier() { + xpath "/plist/dict/key[text()='WholeDisks']/following-sibling::array[1]/string/text()" 2>/dev/null +} + +volume_get() { + key=$1 i=$2 + xpath "/plist/dict/array/dict/key[text()='Volumes']/following-sibling::array/dict[$i]/key[text()='$key']/following-sibling::string[1]/text()" 2> /dev/null +} + +find_nix_volume() { + disk=$1 + i=1 + volumes=$(apfs_volumes_for "$disk") + while true; do + name=$(echo "$volumes" | volume_get "Name" "$i") + if [ -z "$name" ]; then + break + fi + case "$name" in + [Nn]ix*) + echo "$name" + break + ;; + esac + i=$((i+1)) + done +} + +test_fstab() { + grep -q "/nix" /etc/fstab 2>/dev/null +} + +test_synthetic_conf() { + grep -q "^nix" /etc/synthetic.conf 2>/dev/null +} + +test_nix() { + test -d "/nix" +} + +main() { + ( + echo "" + echo " ------------------------------------------------------------------ " + echo " | This installer will create a volume for the nix store and |" + echo " | configure it to mount at /nix. Follow these steps to uninstall. |" + echo " ------------------------------------------------------------------ " + echo "" + echo " 1. Remove the entry from fstab using 'sudo vifs'" + echo " 2. Destroy the data volume using 'diskutil apfs deleteVolume'" + echo " 3. Delete /etc/synthetic.conf" + echo "" + ) >&2 + + if [ -L "/nix" ]; then + echo "error: /nix is a symlink, please remove it or edit synthetic.conf (requires reboot)" >&2 + echo " /nix -> $(readlink "/nix")" >&2 + exit 2 + fi + + if ! test_synthetic_conf; then + echo "Configuring /etc/synthetic.conf..." >&2 + echo nix | sudo tee /etc/synthetic.conf + /System/Library/Filesystems/apfs.fs/Contents/Resources/apfs.util -B + fi + + if ! test_nix; then + echo "Creating mountpoint for /nix..." >&2 + sudo mkdir /nix + fi + + disk=$(root_disks | disk_identifier) + volume=$(find_nix_volume "$disk") + if [ -z "$volume" ]; then + echo "Creating a Nix Store volume..." >&2 + sudo diskutil apfs addVolume "$disk" APFS 'Nix Store' -mountpoint /nix + volume="Nix Store" + else + echo "Using existing '$volume' volume" >&2 + fi + + if ! test_fstab; then + echo "Configuring /etc/fstab..." >&2 + label=$(echo "$volume" | sed 's/ /\\040/g') + printf "\$a\nLABEL=%s /nix apfs rw\n.\nwq\n" "$label" | EDITOR=ed sudo vifs + fi + + echo "The following options can be enabled to disable spotlight indexing" >&2 + echo "of the volume, which might be desirable." >&2 + echo "" >&2 + echo " $ mdutil -i off /nix" >&2 + echo "" >&2 +} + +main "$@" diff --git a/scripts/install-nix-from-closure.sh b/scripts/install-nix-from-closure.sh index e06530ddf..34be7ee6a 100644 --- a/scripts/install-nix-from-closure.sh +++ b/scripts/install-nix-from-closure.sh @@ -40,44 +40,62 @@ elif [ "$(uname -s)" = "Linux" ] && [ -e /run/systemd/system ]; then fi INSTALL_MODE=no-daemon - +CREATE_DARWIN_VOLUME=0 # handle the command line flags -while [ "x${1:-}" != "x" ]; do - if [ "x${1:-}" = "x--no-daemon" ]; then - INSTALL_MODE=no-daemon - elif [ "x${1:-}" = "x--daemon" ]; then - INSTALL_MODE=daemon - elif [ "x${1:-}" = "x--no-channel-add" ]; then - NIX_INSTALLER_NO_CHANNEL_ADD=1 - elif [ "x${1:-}" = "x--no-modify-profile" ]; then - NIX_INSTALLER_NO_MODIFY_PROFILE=1 - elif [ "x${1:-}" != "x" ]; then - ( - echo "Nix Installer [--daemon|--no-daemon] [--no-channel-add] [--no-modify-profile]" +while [ $# -gt 0 ]; do + case $1 in + --daemon) + INSTALL_MODE=daemon;; + --no-daemon) + INSTALL_MODE=no-daemon;; + --no-channel-add) + NIX_INSTALLER_NO_CHANNEL_ADD=1;; + --no-modify-profile) + NIX_INSTALLER_NO_MODIFY_PROFILE=1;; + --create-volume) + CREATE_DARWIN_VOLUME=1;; + *) + ( + echo "Nix Installer [--daemon|--no-daemon] [--no-channel-add] [--no-modify-profile]" - echo "Choose installation method." - echo "" - echo " --daemon: Installs and configures a background daemon that manages the store," - echo " providing multi-user support and better isolation for local builds." - echo " Both for security and reproducibility, this method is recommended if" - echo " supported on your platform." - echo " See https://nixos.org/nix/manual/#sect-multi-user-installation" - echo "" - echo " --no-daemon: Simple, single-user installation that does not require root and is" - echo " trivial to uninstall." - echo " (default)" - echo "" - echo " --no-channel-add: Don't add any channels. nixpkgs-unstable is installed by default." - echo "" - echo " --no-modify-profile: Skip channel installation. When not provided nixpkgs-unstable" - echo " is installed by default." - echo "" - ) >&2 - exit - fi + echo "Choose installation method." + echo "" + echo " --daemon: Installs and configures a background daemon that manages the store," + echo " providing multi-user support and better isolation for local builds." + echo " Both for security and reproducibility, this method is recommended if" + echo " supported on your platform." + echo " See https://nixos.org/nix/manual/#sect-multi-user-installation" + echo "" + echo " --no-daemon: Simple, single-user installation that does not require root and is" + echo " trivial to uninstall." + echo " (default)" + echo "" + echo " --no-channel-add: Don't add any channels. nixpkgs-unstable is installed by default." + echo "" + echo " --no-modify-profile: Skip channel installation. When not provided nixpkgs-unstable" + echo " is installed by default." + echo "" + ) >&2 + + if [ "$(uname -s)" = "Darwin" ]; then + ( + echo " --create-volume: Create an APFS volume for the store and create the /nix" + echo " mountpoint for it using synthetic.conf." + echo " (required on macOS >=10.15)" + echo " See https://nixos.org/nix/manual/#sect-darwin-apfs-volume" + echo "" + ) >&2 + fi + exit;; + esac shift done +if [ "$(uname -s)" = "Darwin" ] && [ "$CREATE_DARWIN_VOLUME" = 1 ]; then + printf '\e[1;31mCreating volume and mountpoint /nix.\e[0m\n' + "$self/create-darwin-volume.sh" +fi + if [ "$INSTALL_MODE" = "daemon" ]; then printf '\e[1;31mSwitching to the Daemon-based Installer\e[0m\n' exec "$self/install-multi-user" @@ -95,6 +113,15 @@ if ! [ -e $dest ]; then echo "directory $dest does not exist; creating it by running '$cmd' using sudo" >&2 if ! sudo sh -c "$cmd"; then echo "$0: please manually run '$cmd' as root to create $dest" >&2 + if [ "$(uname -s)" = "Darwin" ]; then + ( + echo "" + echo "Installing on macOS >=10.15 requires relocating the store to an apfs volume." + echo "Use --create-volume or run the preparation steps manually." + echo "See https://nixos.org/nix/manual/#sect-darwin-apfs-volume." + echo "" + ) >&2 + fi exit 1 fi fi From 10202628b911980f05fc2c9460995af30f1bcbf3 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Sun, 15 Dec 2019 21:11:14 +0100 Subject: [PATCH 15/58] install: also configure ~/.zshenv The default login shell for users on macOS 10.15 changed from bash to zsh. So while generally nonstandard we need to configure it to make nix function out of the box on macOS. --- scripts/install-nix-from-closure.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/scripts/install-nix-from-closure.sh b/scripts/install-nix-from-closure.sh index 34be7ee6a..88275d1f0 100644 --- a/scripts/install-nix-from-closure.sh +++ b/scripts/install-nix-from-closure.sh @@ -197,6 +197,17 @@ if [ -z "$NIX_INSTALLER_NO_MODIFY_PROFILE" ]; then break fi done + for i in .zshenv .zshrc; do + fn="$HOME/$i" + if [ -w "$fn" ]; then + if ! grep -q "$p" "$fn"; then + echo "modifying $fn..." >&2 + echo "if [ -e $p ]; then . $p; fi # added by Nix installer" >> "$fn" + fi + added=1 + break + fi + done fi if [ -z "$added" ]; then From 083bb3bbfcdccebd06bde81a66f158d51ed6e455 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Thu, 13 Feb 2020 12:57:35 +0100 Subject: [PATCH 16/58] install: show macOS 10.15 message with --daemon --- scripts/create-darwin-volume.sh | 8 ++++---- scripts/install-nix-from-closure.sh | 28 ++++++++++++++++------------ 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/scripts/create-darwin-volume.sh b/scripts/create-darwin-volume.sh index f3e0d46f1..d6fb44f43 100755 --- a/scripts/create-darwin-volume.sh +++ b/scripts/create-darwin-volume.sh @@ -1,8 +1,8 @@ #!/bin/sh set -e -root_disks() { - diskutil list -plist / +root_disk() { + diskutil info -plist / } apfs_volumes_for() { @@ -11,7 +11,7 @@ apfs_volumes_for() { } disk_identifier() { - xpath "/plist/dict/key[text()='WholeDisks']/following-sibling::array[1]/string/text()" 2>/dev/null + xpath "/plist/dict/key[text()='ParentWholeDisk']/following-sibling::string[1]/text()" 2>/dev/null } volume_get() { @@ -81,7 +81,7 @@ main() { sudo mkdir /nix fi - disk=$(root_disks | disk_identifier) + disk=$(root_disk | disk_identifier) volume=$(find_nix_volume "$disk") if [ -z "$volume" ]; then echo "Creating a Nix Store volume..." >&2 diff --git a/scripts/install-nix-from-closure.sh b/scripts/install-nix-from-closure.sh index 88275d1f0..7d32bf92e 100644 --- a/scripts/install-nix-from-closure.sh +++ b/scripts/install-nix-from-closure.sh @@ -91,9 +91,22 @@ while [ $# -gt 0 ]; do shift done -if [ "$(uname -s)" = "Darwin" ] && [ "$CREATE_DARWIN_VOLUME" = 1 ]; then - printf '\e[1;31mCreating volume and mountpoint /nix.\e[0m\n' - "$self/create-darwin-volume.sh" +if [ "$(uname -s)" = "Darwin" ]; then + if [ "$CREATE_DARWIN_VOLUME" = 1 ]; then + printf '\e[1;31mCreating volume and mountpoint /nix.\e[0m\n' + "$self/create-darwin-volume.sh" + fi + + info=$(diskutil info -plist / | xpath "/plist/dict/key[text()='Writable']/following-sibling::true[1]" 2> /dev/null) + if ! [ -e $dest ] && [ -n "$info" ]; then + ( + echo "" + echo "Installing on macOS >=10.15 requires relocating the store to an apfs volume." + echo "Use --create-volume or run the preparation steps manually." + echo "See https://nixos.org/nix/manual/#sect-darwin-apfs-volume." + echo "" + ) >&2 + fi fi if [ "$INSTALL_MODE" = "daemon" ]; then @@ -113,15 +126,6 @@ if ! [ -e $dest ]; then echo "directory $dest does not exist; creating it by running '$cmd' using sudo" >&2 if ! sudo sh -c "$cmd"; then echo "$0: please manually run '$cmd' as root to create $dest" >&2 - if [ "$(uname -s)" = "Darwin" ]; then - ( - echo "" - echo "Installing on macOS >=10.15 requires relocating the store to an apfs volume." - echo "Use --create-volume or run the preparation steps manually." - echo "See https://nixos.org/nix/manual/#sect-darwin-apfs-volume." - echo "" - ) >&2 - fi exit 1 fi fi From ee89b7797d4ec1db6dad9df5fb3bb8cc2f05de12 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Fri, 17 Jan 2020 23:27:29 +0100 Subject: [PATCH 17/58] manual: add apfs volume section --- doc/manual/installation/installing-binary.xml | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/doc/manual/installation/installing-binary.xml b/doc/manual/installation/installing-binary.xml index 3f57f47b5..86cbce6bf 100644 --- a/doc/manual/installation/installing-binary.xml +++ b/doc/manual/installation/installing-binary.xml @@ -136,6 +136,109 @@ sudo rm /Library/LaunchDaemons/org.nixos.nix-daemon.plist +
+ APFS Volume Installation + + + The root filesystem is read-only as of macOS 10.15 Catalina, all writable + paths to a separate data volume. This means creating or writing to /nix + is not allowed. While changing the default prefix would be possible, it's + a very intrusive change that has side effects we want to avoid for now. + + + + For common writable locations firmlinks where introduced, + described by Apple as a "bi-directional wormhole" between two filesystems. + Essentially a bind mount for APFS volumes. However this is (currently) not + user configurable and only available for paths like /Users. + + + + For special cases like NFS mount points or package manager roots synthetic.conf(5) + provides a mechanism for some limited, user-controlled file-creation at /. + This only applies on a reboot, but apfs.util can be used + to trigger the creation (not deletion) of new entries. + + + +alice$ /System/Library/Filesystems/apfs.fs/Contents/Resources/apfs.util -B + + + + + + The simplest solution is creating a symlink with /etc/synthetic.conf + to the data volume. (not recommended) + + + +nix /System/Volumes/Data/nix + + + +alice$ ls -l / +lrwxr-xr-x 1 root wheel 25 Jan 1 2019 nix -> /System/Volumes/Data/nix + + + + However builds that detect or resolve this symlink will leak the canonical + location or even fail in certain cases, making this approach undesirable. + + + + + + An empty directory can also be created using /etc/synthetic.conf, + this won't be writable but can be used as a mount point. And with + APFS it's relatively easy to create an separate + volume for nix instead. + + + +nix + + + +alice$ sudo diskutil apfs addVolume diskX APFS 'Nix Store' -mountpoint /nix +alice$ mount +/dev/disk1s6 on /nix (apfs, local, journaled) + + + + This does make the installation more complicated, requiring both + /etc/synthetic.conf as well as /etc/fstab + + + +# +# Warning - this file should only be modified with vifs(8) +# +# Failure to do so is unsupported and may be destructive. +# +LABEL=Nix\040Store /nix apfs rw + + + + On macOS volumes are also mounted quite late, launchd services or other + things that start during login will start before our volume is mounted. + For these cases eg. wait4path must be used for + things that depend on /nix. + + + + This new volume also won't be encrypted by default, and enabling is + only possible interactively? + + + +diskutil apfs enableFileVault /nix -user disk + + + + + +
+
Installing a pinned Nix version from a URL From caface1980344347f2a50701ec133e98e6094c8c Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Thu, 5 Mar 2020 23:12:52 +0100 Subject: [PATCH 18/58] install: hide the store volume on darwin --- scripts/create-darwin-volume.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/create-darwin-volume.sh b/scripts/create-darwin-volume.sh index d6fb44f43..a1a67acea 100755 --- a/scripts/create-darwin-volume.sh +++ b/scripts/create-darwin-volume.sh @@ -94,7 +94,7 @@ main() { if ! test_fstab; then echo "Configuring /etc/fstab..." >&2 label=$(echo "$volume" | sed 's/ /\\040/g') - printf "\$a\nLABEL=%s /nix apfs rw\n.\nwq\n" "$label" | EDITOR=ed sudo vifs + printf "\$a\nLABEL=%s /nix apfs rw,nobrowse\n.\nwq\n" "$label" | EDITOR=ed sudo vifs fi echo "The following options can be enabled to disable spotlight indexing" >&2 From 04f597c3f4d0ac8b8a677c798642329a5a6768e3 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Sat, 7 Mar 2020 12:01:40 +0100 Subject: [PATCH 19/58] install: improve output and error handling --- doc/manual/installation/installing-binary.xml | 3 ++- scripts/create-darwin-volume.sh | 17 ++++++++++++++--- scripts/install-nix-from-closure.sh | 7 ++++--- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/doc/manual/installation/installing-binary.xml b/doc/manual/installation/installing-binary.xml index 86cbce6bf..7f4bba3a0 100644 --- a/doc/manual/installation/installing-binary.xml +++ b/doc/manual/installation/installing-binary.xml @@ -199,6 +199,7 @@ nix +alice$ /System/Library/Filesystems/apfs.fs/Contents/Resources/apfs.util -B alice$ sudo diskutil apfs addVolume diskX APFS 'Nix Store' -mountpoint /nix alice$ mount /dev/disk1s6 on /nix (apfs, local, journaled) @@ -231,7 +232,7 @@ LABEL=Nix\040Store /nix apfs rw -diskutil apfs enableFileVault /nix -user disk +alice$ diskutil apfs enableFileVault /nix -user disk diff --git a/scripts/create-darwin-volume.sh b/scripts/create-darwin-volume.sh index a1a67acea..e2b49c9a0 100755 --- a/scripts/create-darwin-volume.sh +++ b/scripts/create-darwin-volume.sh @@ -73,12 +73,22 @@ main() { if ! test_synthetic_conf; then echo "Configuring /etc/synthetic.conf..." >&2 echo nix | sudo tee /etc/synthetic.conf - /System/Library/Filesystems/apfs.fs/Contents/Resources/apfs.util -B + if ! test_synthetic_conf; then + echo "error: failed to configure synthetic.conf" >&2 + exit 1 + fi fi if ! test_nix; then echo "Creating mountpoint for /nix..." >&2 - sudo mkdir /nix + /System/Library/Filesystems/apfs.fs/Contents/Resources/apfs.util -B || true + if ! test_nix; then + sudo mkdir -p /nix 2>/dev/null || true + fi + if ! test_nix; then + echo "error: failed to bootstrap /nix, a reboot might be required" >&2 + exit 1 + fi fi disk=$(root_disk | disk_identifier) @@ -97,10 +107,11 @@ main() { printf "\$a\nLABEL=%s /nix apfs rw,nobrowse\n.\nwq\n" "$label" | EDITOR=ed sudo vifs fi + echo "" >&2 echo "The following options can be enabled to disable spotlight indexing" >&2 echo "of the volume, which might be desirable." >&2 echo "" >&2 - echo " $ mdutil -i off /nix" >&2 + echo " $ sudo mdutil -i off /nix" >&2 echo "" >&2 } diff --git a/scripts/install-nix-from-closure.sh b/scripts/install-nix-from-closure.sh index 7d32bf92e..2f291ed4c 100644 --- a/scripts/install-nix-from-closure.sh +++ b/scripts/install-nix-from-closure.sh @@ -82,7 +82,7 @@ while [ $# -gt 0 ]; do echo " --create-volume: Create an APFS volume for the store and create the /nix" echo " mountpoint for it using synthetic.conf." echo " (required on macOS >=10.15)" - echo " See https://nixos.org/nix/manual/#sect-darwin-apfs-volume" + echo " See https://nixos.org/nix/manual/#sect-apfs-volume-installation" echo "" ) >&2 fi @@ -102,10 +102,11 @@ if [ "$(uname -s)" = "Darwin" ]; then ( echo "" echo "Installing on macOS >=10.15 requires relocating the store to an apfs volume." - echo "Use --create-volume or run the preparation steps manually." - echo "See https://nixos.org/nix/manual/#sect-darwin-apfs-volume." + echo "Use sh <(curl https://nixos.org/nix/install) --create-volume or run the preparation steps manually." + echo "See https://nixos.org/nix/manual/#sect-apfs-volume-installation" echo "" ) >&2 + exit 1 fi fi From bc24c09968bb35fce599151f86d123cb5984f727 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Wed, 18 Mar 2020 20:34:46 +0100 Subject: [PATCH 20/58] install: make synthetic.conf and fstab checks stricter --- scripts/create-darwin-volume.sh | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/scripts/create-darwin-volume.sh b/scripts/create-darwin-volume.sh index e2b49c9a0..ea4133444 100755 --- a/scripts/create-darwin-volume.sh +++ b/scripts/create-darwin-volume.sh @@ -39,11 +39,15 @@ find_nix_volume() { } test_fstab() { - grep -q "/nix" /etc/fstab 2>/dev/null + grep -q "/nix apfs rw" /etc/fstab 2>/dev/null +} + +test_nix_symlink() { + [ -L "/nix" ] || grep -q "^nix." /etc/synthetic.conf 2>/dev/null } test_synthetic_conf() { - grep -q "^nix" /etc/synthetic.conf 2>/dev/null + grep -q "^nix$" /etc/synthetic.conf 2>/dev/null } test_nix() { @@ -60,12 +64,12 @@ main() { echo "" echo " 1. Remove the entry from fstab using 'sudo vifs'" echo " 2. Destroy the data volume using 'diskutil apfs deleteVolume'" - echo " 3. Delete /etc/synthetic.conf" + echo " 3. Remove the 'nix' line from /etc/synthetic.conf or the file" echo "" ) >&2 - if [ -L "/nix" ]; then - echo "error: /nix is a symlink, please remove it or edit synthetic.conf (requires reboot)" >&2 + if test_nix_symlink; then + echo "error: /nix is a symlink, please remove it and make sure it's not in synthetic.conf (in which case a reboot is required)" >&2 echo " /nix -> $(readlink "/nix")" >&2 exit 2 fi From 33865752960c9a2ff28eb9024f20d2103918685c Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Thu, 26 Mar 2020 20:14:08 +0100 Subject: [PATCH 21/58] manual: clarify volume creation section --- doc/manual/installation/installing-binary.xml | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/doc/manual/installation/installing-binary.xml b/doc/manual/installation/installing-binary.xml index 7f4bba3a0..c11ba9cce 100644 --- a/doc/manual/installation/installing-binary.xml +++ b/doc/manual/installation/installing-binary.xml @@ -141,13 +141,14 @@ sudo rm /Library/LaunchDaemons/org.nixos.nix-daemon.plist The root filesystem is read-only as of macOS 10.15 Catalina, all writable - paths to a separate data volume. This means creating or writing to /nix - is not allowed. While changing the default prefix would be possible, it's - a very intrusive change that has side effects we want to avoid for now. + paths were moved to a separate data volume. This means creating or writing + to /nix is not allowed. While changing the default prefix + would be possible, it's a very intrusive change that has side effects we want to + avoid for now. - For common writable locations firmlinks where introduced, + For common writable locations firmlinks were introduced, described by Apple as a "bi-directional wormhole" between two filesystems. Essentially a bind mount for APFS volumes. However this is (currently) not user configurable and only available for paths like /Users. @@ -156,8 +157,10 @@ sudo rm /Library/LaunchDaemons/org.nixos.nix-daemon.plist For special cases like NFS mount points or package manager roots synthetic.conf(5) provides a mechanism for some limited, user-controlled file-creation at /. - This only applies on a reboot, but apfs.util can be used - to trigger the creation (not deletion) of new entries. + This only applies at boot time, however apfs.util can be used + to trigger the creation (not deletion) of new entries without a reboot. + It would be ideal if this could create firmlinks, however a symlink or mountpoint + are the only options. From 477d7c2d07e146c91950401b8b9d9380ce6787e5 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Thu, 26 Mar 2020 21:51:13 +0100 Subject: [PATCH 22/58] installer: refuse apfs volume creation when FileVault is enabled --- doc/manual/installation/installing-binary.xml | 6 +++-- scripts/create-darwin-volume.sh | 22 +++++++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/doc/manual/installation/installing-binary.xml b/doc/manual/installation/installing-binary.xml index c11ba9cce..498248662 100644 --- a/doc/manual/installation/installing-binary.xml +++ b/doc/manual/installation/installing-binary.xml @@ -230,8 +230,10 @@ LABEL=Nix\040Store /nix apfs rw - This new volume also won't be encrypted by default, and enabling is - only possible interactively? + This new volume also won't be encrypted by default, and enabling it + requires extra setup. For machines with a T2 chip + all data is already entrypted at rest, older hardware won't even when + FileVault is enabled for the rest of the system. diff --git a/scripts/create-darwin-volume.sh b/scripts/create-darwin-volume.sh index ea4133444..a0da85f43 100755 --- a/scripts/create-darwin-volume.sh +++ b/scripts/create-darwin-volume.sh @@ -14,7 +14,12 @@ disk_identifier() { xpath "/plist/dict/key[text()='ParentWholeDisk']/following-sibling::string[1]/text()" 2>/dev/null } -volume_get() { +volume_list_true() { + key=$1 t=$2 + xpath "/plist/dict/array/dict/key[text()='Volumes']/following-sibling::array/dict/key[text()='$key']/following-sibling::true[1]" 2> /dev/null +} + +volume_get_string() { key=$1 i=$2 xpath "/plist/dict/array/dict/key[text()='Volumes']/following-sibling::array/dict[$i]/key[text()='$key']/following-sibling::string[1]/text()" 2> /dev/null } @@ -24,7 +29,7 @@ find_nix_volume() { i=1 volumes=$(apfs_volumes_for "$disk") while true; do - name=$(echo "$volumes" | volume_get "Name" "$i") + name=$(echo "$volumes" | volume_get_string "Name" "$i") if [ -z "$name" ]; then break fi @@ -54,6 +59,12 @@ test_nix() { test -d "/nix" } +test_filevault() { + disk=$1 + apfs_volumes_for "$disk" | volume_list_true FileVault | grep -q true || return + ! sudo xartutil --list >/dev/null 2>/dev/null +} + main() { ( echo "" @@ -99,6 +110,13 @@ main() { volume=$(find_nix_volume "$disk") if [ -z "$volume" ]; then echo "Creating a Nix Store volume..." >&2 + + if test_filevault "$disk"; then + echo "error: FileVault detected, refusing to create unencrypted volume" >&2 + echo "See https://nixos.org/nix/manual/#sect-apfs-volume-installation" >&2 + exit 1 + fi + sudo diskutil apfs addVolume "$disk" APFS 'Nix Store' -mountpoint /nix volume="Nix Store" else From 2b0a81d92d28994374465c44c79f020d5e044700 Mon Sep 17 00:00:00 2001 From: "Travis A. Everett" Date: Thu, 14 May 2020 21:59:10 -0500 Subject: [PATCH 23/58] focus on golden-path covering most scenarios This should handle installation scenarios we can handle with anything resembling confidence. Goal is approximating the existing setup--not enforcing a best-practice... Approaches (+ installer-handled, - manual) and configs each covers: + no change needed; /nix OK on boot volume: All pre-Catalina (regardless of T2 or FileVault use) + create new unencrypted volume: Catalina, pre-T2, no FileVault + create new encrypted-at-rest volume: Catalina, pre-T2, FileVault Catalina, T2, no FileVault - require user to pre-create encrypted volume Catalina, T2, FileVault --- doc/manual/installation/installing-binary.xml | 350 +++++++++++++----- scripts/create-darwin-volume.sh | 77 +++- scripts/install-nix-from-closure.sh | 19 +- 3 files changed, 331 insertions(+), 115 deletions(-) diff --git a/doc/manual/installation/installing-binary.xml b/doc/manual/installation/installing-binary.xml index 498248662..8d548f0ea 100644 --- a/doc/manual/installation/installing-binary.xml +++ b/doc/manual/installation/installing-binary.xml @@ -6,16 +6,30 @@ Installing a Binary Distribution -If you are using Linux or macOS, the easiest way to install Nix -is to run the following command: + + If you are using Linux or macOS versions up to 10.14 (Mojave), the + easiest way to install Nix is to run the following command: + $ sh <(curl https://nixos.org/nix/install) -As of Nix 2.1.0, the Nix installer will always default to creating a -single-user installation, however opting in to the multi-user -installation is highly recommended. + + If you're using macOS 10.15 (Catalina) or newer, consult + the macOS installation instructions + before installing. + + + + As of Nix 2.1.0, the Nix installer will always default to creating a + single-user installation, however opting in to the multi-user + installation is highly recommended. +
@@ -36,7 +50,7 @@ run this under your usual user account, not as root. The script will invoke sudo to create /nix if it doesn’t already exist. If you don’t have sudo, you should manually create -/nix first as root, e.g.: +/nix first as root, e.g.: $ mkdir /nix @@ -47,7 +61,7 @@ The install script will modify the first writable file from amongst .bash_profile, .bash_login and .profile to source ~/.nix-profile/etc/profile.d/nix.sh. You can set -the NIX_INSTALLER_NO_MODIFY_PROFILE environment +the NIX_INSTALLER_NO_MODIFY_PROFILE environment variable before executing the install script to disable this behaviour. @@ -81,12 +95,10 @@ $ rm -rf /nix You can instruct the installer to perform a multi-user installation on your system: - - - sh <(curl https://nixos.org/nix/install) --daemon - + sh <(curl https://nixos.org/nix/install) --daemon + The multi-user installation of Nix will create build users between the user IDs 30001 and 30032, and a group with the group ID 30000. @@ -136,82 +148,253 @@ sudo rm /Library/LaunchDaemons/org.nixos.nix-daemon.plist
-
- APFS Volume Installation +
+ macOS Installation - The root filesystem is read-only as of macOS 10.15 Catalina, all writable - paths were moved to a separate data volume. This means creating or writing - to /nix is not allowed. While changing the default prefix - would be possible, it's a very intrusive change that has side effects we want to - avoid for now. + Starting with macOS 10.15 (Catalina), the root filesystem is read-only. + This means /nix can no longer live on your system + volume, and that you'll need a workaround to install Nix. - For common writable locations firmlinks were introduced, - described by Apple as a "bi-directional wormhole" between two filesystems. - Essentially a bind mount for APFS volumes. However this is (currently) not - user configurable and only available for paths like /Users. + The recommended approach, which creates an unencrypted APFS volume + for your Nix store and a "synthetic" empty directory to mount it + over at /nix, is least likely to impair Nix + or your system. + + With all separate-volume approaches, it's possible something on + your system (particularly daemons/services and restored apps) may + need access to your Nix store before the volume is mounted. Adding + additional encryption makes this more likely. + + - For special cases like NFS mount points or package manager roots synthetic.conf(5) - provides a mechanism for some limited, user-controlled file-creation at /. - This only applies at boot time, however apfs.util can be used - to trigger the creation (not deletion) of new entries without a reboot. - It would be ideal if this could create firmlinks, however a symlink or mountpoint - are the only options. + If you're using a recent Mac with a + T2 chip, + your drive will still be encrypted at rest (in which case "unencrypted" + is a bit of a misnomer). To use this approach, just install Nix with: - -alice$ /System/Library/Filesystems/apfs.fs/Contents/Resources/apfs.util -B - + $ sh <(curl https://nixos.org/nix/install) --darwin-use-unencrypted-nix-store-volume - - - - The simplest solution is creating a symlink with /etc/synthetic.conf - to the data volume. (not recommended) - + + If you don't like the sound of this, you'll want to weigh the + other approaches and tradeoffs detailed in this section. + - -nix /System/Volumes/Data/nix - + + Eventual solutions? + + All of the known workarounds have drawbacks, but we hope + better solutions will be available in the future. Some that + we have our eye on are: + + + + + A true firmlink would enable the Nix store to live on the + primary data volume without the build problems caused by + the symlink approach. End users cannot currently + create true firmlinks. + + + + + If the Nix store volume shared FileVault encryption + with the primary data volume (probably by using the same + volume group and role), FileVault encryption could be + easily supported by the installer without requiring + manual setup by each user. + + + + - -alice$ ls -l / -lrwxr-xr-x 1 root wheel 25 Jan 1 2019 nix -> /System/Volumes/Data/nix - +
+ Change the Nix store path prefix + + Changing the default prefix for the Nix store is a simple + approach which enables you to leave it on your root volume, + where it can take full advantage of FileVault encryption if + enabled. Unfortunately, this approach also opts your device out + of some benefits that are enabled by using the same prefix + across systems: - - However builds that detect or resolve this symlink will leak the canonical - location or even fail in certain cases, making this approach undesirable. - - + + + + Your system won't be able to take advantage of the binary + cache (unless someone is able to stand up and support + duplicate caching infrastructure), which means you'll + spend more time waiting for builds. + + + + + It's harder to build and deploy packages to Linux systems. + + + + - - - An empty directory can also be created using /etc/synthetic.conf, - this won't be writable but can be used as a mount point. And with - APFS it's relatively easy to create an separate - volume for nix instead. - + - -nix - + It would also possible (and often requested) to just apply this + change ecosystem-wide, but it's an intrusive process that has + side effects we want to avoid for now. + + + + +
- -alice$ /System/Library/Filesystems/apfs.fs/Contents/Resources/apfs.util -B -alice$ sudo diskutil apfs addVolume diskX APFS 'Nix Store' -mountpoint /nix -alice$ mount -/dev/disk1s6 on /nix (apfs, local, journaled) - +
+ Use a separate encrypted volume + + If you like, you can also add encryption to the recommended + approach taken by the installer. You can do this by pre-creating + an encrypted volume before you run the installer--or you can + run the installer and encrypt the volume it creates later. + + + + In either case, adding encryption to a second volume isn't quite + as simple as enabling FileVault for your boot volume. Before you + dive in, there are a few things to weigh: + + + + + The additional volume won't be encrypted with your existing + FileVault key, so you'll need another mechanism to decrypt + the volume. + + + + + You can store the password in Keychain to automatically + decrypt the volume on boot--but it'll have to wait on Keychain + and may not mount before your GUI apps restore. If any of + your launchd agents or apps depend on Nix-installed software + (for example, if you use a Nix-installed login shell), the + restore may fail or break. + + + On a case-by-case basis, you may be able to work around this + problem by using wait4path to block + execution until your executable is available. + + + It's also possible to decrypt and mount the volume earlier + with a login hook--but this mechanism appears to be + deprecated and its future is unclear. + + + + + You can hard-code the password in the clear, so that your + store volume can be decrypted before Keychain is available. + + + + + If you are comfortable navigating these tradeoffs, you can encrypt the volume with + something along the lines of: + + + + alice$ diskutil apfs enableFileVault /nix -user disk + + +
+ +
+ + Symlink the Nix store to a custom location + + Another simple approach is using /etc/synthetic.conf + to symlink the Nix store to the data volume. This option also + enables your store to share any configured FileVault encryption. + Unfortunately, builds that resolve the symlink may leak the + canonical path or even fail. + + + Because of these downsides, we can't recommend this approach. + + +
+ +
+ Notes on the recommended approach + + This section goes into a little more detail on the recommended + approach. You don't need to understand it to run the installer, + but it can serve as a helpful reference if you run into trouble. + + + + + In order to compose user-writable locations into the new + read-only system root, Apple introduced a new concept called + firmlinks, which it describes as a + "bi-directional wormhole" between two filesystems. You can + see the current firmlinks in /usr/share/firmlinks. + Unfortunately, firmlinks aren't (currently?) user-configurable. + + + + For special cases like NFS mount points or package manager roots, + synthetic.conf(5) + supports limited user-controlled file-creation (of symlinks, + and synthetic empty directories) at /. + To create a synthetic empty directory for mounting at /nix, + add the following line to /etc/synthetic.conf + (create it if necessary): + + + nix + + + + + This configuration is applied at boot time, but you can use + apfs.util to trigger creation (not deletion) + of new entries without a reboot: + + + alice$ /System/Library/Filesystems/apfs.fs/Contents/Resources/apfs.util -B + + + + + Create the new APFS volume with diskutil: + + + alice$ sudo diskutil apfs addVolume diskX APFS 'Nix Store' -mountpoint /nix + + + + + Using vifs, add the new mount to + /etc/fstab. If it doesn't already have + other entries, it should look something like: + # @@ -219,29 +402,16 @@ alice$ mount # # Failure to do so is unsupported and may be destructive. # -LABEL=Nix\040Store /nix apfs rw +LABEL=Nix\040Store /nix apfs rw,nobrowse - - On macOS volumes are also mounted quite late, launchd services or other - things that start during login will start before our volume is mounted. - For these cases eg. wait4path must be used for - things that depend on /nix. - - - - This new volume also won't be encrypted by default, and enabling it - requires extra setup. For machines with a T2 chip - all data is already entrypted at rest, older hardware won't even when - FileVault is enabled for the rest of the system. - - - -alice$ diskutil apfs enableFileVault /nix -user disk - - - - + + The nobrowse setting will keep Spotlight from indexing this + volume, and keep it from showing up on your desktop. + + + +
diff --git a/scripts/create-darwin-volume.sh b/scripts/create-darwin-volume.sh index a0da85f43..47cc3e913 100755 --- a/scripts/create-darwin-volume.sh +++ b/scripts/create-darwin-volume.sh @@ -59,10 +59,45 @@ test_nix() { test -d "/nix" } -test_filevault() { +test_t2_chip_present(){ + # Use xartutil to see if system has a t2 chip. + # + # This isn't well-documented on its own; until it is, + # let's keep track of knowledge/assumptions. + # + # Warnings: + # - Don't search "xart" if porn will cause you trouble :) + # - Other xartutil flags do dangerous things. Don't run them + # naively. If you must, search "xartutil" first. + # + # Assumptions: + # - the "xART session seeds recovery utility" + # appears to interact with xartstorageremoted + # - `sudo xartutil --list` lists xART sessions + # and their seeds and exits 0 if successful. If + # not, it exits 1 and prints an error such as: + # xartutil: ERROR: No supported link to the SEP present + # - xART sessions/seeds are present when a T2 chip is + # (and not, otherwise) + # - the presence of a T2 chip means a newly-created + # volume on the primary drive will be + # encrypted at rest + # - all together: `sudo xartutil --list` + # should exit 0 if a new Nix Store volume will + # be encrypted at rest, and exit 1 if not. + sudo xartutil --list >/dev/null 2>/dev/null +} + +test_filevault_in_use() { disk=$1 - apfs_volumes_for "$disk" | volume_list_true FileVault | grep -q true || return - ! sudo xartutil --list >/dev/null 2>/dev/null + # list vols on disk | get value of Filevault key | value is true + apfs_volumes_for "$disk" | volume_list_true FileVault | grep -q true +} + +# use after error msg for conditions we don't understand +suggest_report_error(){ + # ex "error: something sad happened :(" >&2 + echo " please report this @ https://github.com/nixos/nix/issues" >&2 } main() { @@ -89,7 +124,8 @@ main() { echo "Configuring /etc/synthetic.conf..." >&2 echo nix | sudo tee /etc/synthetic.conf if ! test_synthetic_conf; then - echo "error: failed to configure synthetic.conf" >&2 + echo "error: failed to configure synthetic.conf;" >&2 + suggest_report_error exit 1 fi fi @@ -101,7 +137,8 @@ main() { sudo mkdir -p /nix 2>/dev/null || true fi if ! test_nix; then - echo "error: failed to bootstrap /nix, a reboot might be required" >&2 + echo "error: failed to bootstrap /nix; if a reboot doesn't help," >&2 + suggest_report_error exit 1 fi fi @@ -111,10 +148,25 @@ main() { if [ -z "$volume" ]; then echo "Creating a Nix Store volume..." >&2 - if test_filevault "$disk"; then - echo "error: FileVault detected, refusing to create unencrypted volume" >&2 - echo "See https://nixos.org/nix/manual/#sect-apfs-volume-installation" >&2 - exit 1 + if test_filevault_in_use "$disk"; then + # TODO: Not sure if it's in-scope now, but `diskutil apfs list` + # shows both filevault and encrypted at rest status, and it + # may be the more semantic way to test for this? It'll show + # `FileVault: No (Encrypted at rest)` + # `FileVault: No` + # `FileVault: Yes (Unlocked)` + # and so on. + if test_t2_chip_present; then + echo "warning: boot volume is FileVault-encrypted, but the Nix store volume" >&2 + echo " is only encrypted at rest." >&2 + echo " See https://nixos.org/nix/manual/#sect-macos-installation" >&2 + else + echo "error: refusing to create Nix store volume because the boot volume is" >&2 + echo " FileVault encrypted, but encryption-at-rest is not available." >&2 + echo " Manually create a volume for the store and re-run this script." >&2 + echo " See https://nixos.org/nix/manual/#sect-macos-installation" >&2 + exit 1 + fi fi sudo diskutil apfs addVolume "$disk" APFS 'Nix Store' -mountpoint /nix @@ -128,13 +180,6 @@ main() { label=$(echo "$volume" | sed 's/ /\\040/g') printf "\$a\nLABEL=%s /nix apfs rw,nobrowse\n.\nwq\n" "$label" | EDITOR=ed sudo vifs fi - - echo "" >&2 - echo "The following options can be enabled to disable spotlight indexing" >&2 - echo "of the volume, which might be desirable." >&2 - echo "" >&2 - echo " $ sudo mdutil -i off /nix" >&2 - echo "" >&2 } main "$@" diff --git a/scripts/install-nix-from-closure.sh b/scripts/install-nix-from-closure.sh index 2f291ed4c..72aa5abf5 100644 --- a/scripts/install-nix-from-closure.sh +++ b/scripts/install-nix-from-closure.sh @@ -52,7 +52,7 @@ while [ $# -gt 0 ]; do NIX_INSTALLER_NO_CHANNEL_ADD=1;; --no-modify-profile) NIX_INSTALLER_NO_MODIFY_PROFILE=1;; - --create-volume) + --darwin-use-unencrypted-nix-store-volume) CREATE_DARWIN_VOLUME=1;; *) ( @@ -77,12 +77,13 @@ while [ $# -gt 0 ]; do echo "" ) >&2 - if [ "$(uname -s)" = "Darwin" ]; then + # darwin and Catalina+ + if [ "$(uname -s)" = "Darwin" ] && [ "$macos_major" -gt 14 ]; then ( - echo " --create-volume: Create an APFS volume for the store and create the /nix" - echo " mountpoint for it using synthetic.conf." - echo " (required on macOS >=10.15)" - echo " See https://nixos.org/nix/manual/#sect-apfs-volume-installation" + echo " --darwin-use-unencrypted-nix-store-volume: Create an APFS volume for the Nix" + echo " store and mount it at /nix. This is the recommended way to create" + echo " /nix with a read-only / on macOS >=10.15." + echo " See: https://nixos.org/nix/manual/#sect-macos-installation" echo "" ) >&2 fi @@ -98,12 +99,12 @@ if [ "$(uname -s)" = "Darwin" ]; then fi info=$(diskutil info -plist / | xpath "/plist/dict/key[text()='Writable']/following-sibling::true[1]" 2> /dev/null) - if ! [ -e $dest ] && [ -n "$info" ]; then + if ! [ -e $dest ] && [ -n "$info" ] && [ "$macos_major" -gt 14 ]; then ( echo "" echo "Installing on macOS >=10.15 requires relocating the store to an apfs volume." - echo "Use sh <(curl https://nixos.org/nix/install) --create-volume or run the preparation steps manually." - echo "See https://nixos.org/nix/manual/#sect-apfs-volume-installation" + echo "Use sh <(curl https://nixos.org/nix/install) --darwin-use-unencrypted-nix-store-volume or run the preparation steps manually." + echo "See https://nixos.org/nix/manual/#sect-macos-installation" echo "" ) >&2 exit 1 From d3df1889a1f49b114a8d78270ea8a6d0523f5e35 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Thu, 21 May 2020 20:03:09 +0200 Subject: [PATCH 24/58] installer: don't clobber synthetic.conf --- scripts/create-darwin-volume.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/create-darwin-volume.sh b/scripts/create-darwin-volume.sh index 47cc3e913..5214d1652 100755 --- a/scripts/create-darwin-volume.sh +++ b/scripts/create-darwin-volume.sh @@ -122,7 +122,7 @@ main() { if ! test_synthetic_conf; then echo "Configuring /etc/synthetic.conf..." >&2 - echo nix | sudo tee /etc/synthetic.conf + echo nix | sudo tee -a /etc/synthetic.conf if ! test_synthetic_conf; then echo "error: failed to configure synthetic.conf;" >&2 suggest_report_error From 909d8cb2934869c945ac1cc20dfb71df513042eb Mon Sep 17 00:00:00 2001 From: Suraj Barkale Date: Fri, 22 May 2020 11:05:25 +1000 Subject: [PATCH 25/58] Use /etc/zshenv instead of /etc/zshrc for profile As noted in https://github.com/NixOS/nix/issues/3456 the `/etc/zshenv` file provides a better place for sourcing the nix environment. --- scripts/install-multi-user.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/install-multi-user.sh b/scripts/install-multi-user.sh index a0f1deb98..838192733 100644 --- a/scripts/install-multi-user.sh +++ b/scripts/install-multi-user.sh @@ -28,7 +28,7 @@ readonly NIX_FIRST_BUILD_UID="30001" # default shell profile that comes with Nix doesn't support it. readonly NIX_ROOT="/nix" -readonly PROFILE_TARGETS=("/etc/bashrc" "/etc/profile.d/nix.sh" "/etc/zshrc") +readonly PROFILE_TARGETS=("/etc/bashrc" "/etc/profile.d/nix.sh" "/etc/zshenv") readonly PROFILE_BACKUP_SUFFIX=".backup-before-nix" readonly PROFILE_NIX_FILE="$NIX_ROOT/var/nix/profiles/default/etc/profile.d/nix-daemon.sh" From 2a7ea2eb6c54c82d5e858ea6ae9de929face5e55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Domen=20Ko=C5=BEar?= Date: Sat, 23 May 2020 11:12:05 +0200 Subject: [PATCH 26/58] scripts/create-darwin-volume.sh: remove unused variable --- scripts/create-darwin-volume.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/create-darwin-volume.sh b/scripts/create-darwin-volume.sh index 5214d1652..dac30d72d 100755 --- a/scripts/create-darwin-volume.sh +++ b/scripts/create-darwin-volume.sh @@ -15,7 +15,7 @@ disk_identifier() { } volume_list_true() { - key=$1 t=$2 + key=$1 xpath "/plist/dict/array/dict/key[text()='Volumes']/following-sibling::array/dict/key[text()='$key']/following-sibling::true[1]" 2> /dev/null } From 6f6bdd63a0f6dae4ab91422645f548837029dee9 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Sat, 23 May 2020 12:10:12 +0200 Subject: [PATCH 27/58] fix hydra build products Since the binary tarball was replaced none of the hydra builds include the manual. The dist phase isn't enabled by default the manual build products where not written. --- release.nix | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/release.nix b/release.nix index 2cc4bb4c0..2a320e1c3 100644 --- a/release.nix +++ b/release.nix @@ -115,17 +115,17 @@ let installFlags = "sysconfdir=$(out)/etc"; + postInstall = '' + mkdir -p $doc/nix-support + echo "doc manual $doc/share/doc/nix/manual" >> $doc/nix-support/hydra-build-products + ''; + doCheck = true; doInstallCheck = true; installCheckFlags = "sysconfdir=$(out)/etc"; separateDebugInfo = true; - - preDist = '' - mkdir -p $doc/nix-support - echo "doc manual $doc/share/doc/nix/manual" >> $doc/nix-support/hydra-build-products - ''; }); From e2af11ce071fc109ad3e517bf15a87024d0a9ae5 Mon Sep 17 00:00:00 2001 From: Joachim Breitner Date: Sat, 23 May 2020 15:26:59 +0200 Subject: [PATCH 28/58] Manpages: Do not refer to nixpkgs-channels Unless I am misinformed, using the `nixpkgs` repository directly is now preferred? --- doc/manual/command-ref/env-common.xml | 2 +- doc/manual/command-ref/nix-env.xml | 7 ++----- doc/manual/command-ref/nix-shell.xml | 6 +++--- doc/manual/expressions/builtins.xml | 4 ++-- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/doc/manual/command-ref/env-common.xml b/doc/manual/command-ref/env-common.xml index 0217de7b2..8466cc463 100644 --- a/doc/manual/command-ref/env-common.xml +++ b/doc/manual/command-ref/env-common.xml @@ -53,7 +53,7 @@ nixpkgs=/home/eelco/Dev/nixpkgs-branch:/etc/nixos NIX_PATH to -nixpkgs=https://github.com/NixOS/nixpkgs-channels/archive/nixos-15.09.tar.gz +nixpkgs=https://github.com/NixOS/nixpkgs/archive/nixos-15.09.tar.gz tells Nix to download the latest revision in the Nixpkgs/NixOS 15.09 channel. diff --git a/doc/manual/command-ref/nix-env.xml b/doc/manual/command-ref/nix-env.xml index 9c03ccce1..2b95b6819 100644 --- a/doc/manual/command-ref/nix-env.xml +++ b/doc/manual/command-ref/nix-env.xml @@ -526,13 +526,10 @@ these paths will be fetched (0.04 MiB download, 0.19 MiB unpacked): 14.12 channel: -$ nix-env -f https://github.com/NixOS/nixpkgs-channels/archive/nixos-14.12.tar.gz -iA firefox +$ nix-env -f https://github.com/NixOS/nixpkgs/archive/nixos-14.12.tar.gz -iA firefox -(The GitHub repository nixpkgs-channels is updated -automatically from the main nixpkgs repository -after certain tests have succeeded and binaries have been built and -uploaded to the binary cache at cache.nixos.org.) + diff --git a/doc/manual/command-ref/nix-shell.xml b/doc/manual/command-ref/nix-shell.xml index 766482460..2fef323c5 100644 --- a/doc/manual/command-ref/nix-shell.xml +++ b/doc/manual/command-ref/nix-shell.xml @@ -258,7 +258,7 @@ path. You can override it by passing or setting containing the Pan package from a specific revision of Nixpkgs: -$ nix-shell -p pan -I nixpkgs=https://github.com/NixOS/nixpkgs-channels/archive/8a3eea054838b55aca962c3fbde9c83c102b8bf2.tar.gz +$ nix-shell -p pan -I nixpkgs=https://github.com/NixOS/nixpkgs/archive/8a3eea054838b55aca962c3fbde9c83c102b8bf2.tar.gz [nix-shell:~]$ pan --version Pan 0.139 @@ -352,7 +352,7 @@ following Haskell script uses a specific branch of Nixpkgs/NixOS (the -#! nix-shell -I nixpkgs=https://github.com/NixOS/nixpkgs-channels/archive/0672315759b3e15e2121365f067c1c8c56bb4722.tar.gz +#! nix-shell -I nixpkgs=https://github.com/NixOS/nixpkgs/archive/0672315759b3e15e2121365f067c1c8c56bb4722.tar.gz diff --git a/doc/manual/expressions/builtins.xml b/doc/manual/expressions/builtins.xml index 6709c4081..a18c5801a 100644 --- a/doc/manual/expressions/builtins.xml +++ b/doc/manual/expressions/builtins.xml @@ -324,7 +324,7 @@ if builtins ? getEnv then builtins.getEnv "PATH" else "" particular version of Nixpkgs, e.g. -with import (fetchTarball https://github.com/NixOS/nixpkgs-channels/archive/nixos-14.12.tar.gz) {}; +with import (fetchTarball https://github.com/NixOS/nixpkgs/archive/nixos-14.12.tar.gz) {}; stdenv.mkDerivation { … } @@ -349,7 +349,7 @@ stdenv.mkDerivation { … } with import (fetchTarball { - url = "https://github.com/NixOS/nixpkgs-channels/archive/nixos-14.12.tar.gz"; + url = "https://github.com/NixOS/nixpkgs/archive/nixos-14.12.tar.gz"; sha256 = "1jppksrfvbk5ypiqdz4cddxdl8z6zyzdb2srq8fcffr327ld5jj2"; }) {}; From ecc5c90dfc0e9877aef89bf0192203cd1a8f2b21 Mon Sep 17 00:00:00 2001 From: Tobias Pflug Date: Mon, 25 May 2020 11:50:41 +0200 Subject: [PATCH 29/58] Add unit tests for hashing functions --- src/libutil/tests/hash.cc | 80 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 src/libutil/tests/hash.cc diff --git a/src/libutil/tests/hash.cc b/src/libutil/tests/hash.cc new file mode 100644 index 000000000..7cb439817 --- /dev/null +++ b/src/libutil/tests/hash.cc @@ -0,0 +1,80 @@ +#include "hash.hh" +#include + +namespace nix { + + /* ---------------------------------------------------------------------------- + * hashString + * --------------------------------------------------------------------------*/ + + TEST(hashString, testKnownMD5Hashes1) { + // values taken from: https://tools.ietf.org/html/rfc1321 + auto s1 = ""; + auto hash = hashString(HashType::htMD5, s1); + ASSERT_EQ(hash.to_string(Base::Base16), "md5:d41d8cd98f00b204e9800998ecf8427e"); + } + + TEST(hashString, testKnownMD5Hashes2) { + // values taken from: https://tools.ietf.org/html/rfc1321 + auto s2 = "abc"; + auto hash = hashString(HashType::htMD5, s2); + ASSERT_EQ(hash.to_string(Base::Base16), "md5:900150983cd24fb0d6963f7d28e17f72"); + } + + TEST(hashString, testKnownSHA1Hashes1) { + // values taken from: https://tools.ietf.org/html/rfc3174 + auto s = "abc"; + auto hash = hashString(HashType::htSHA1, s); + ASSERT_EQ(hash.to_string(Base::Base16),"sha1:a9993e364706816aba3e25717850c26c9cd0d89d"); + } + + TEST(hashString, testKnownSHA1Hashes2) { + // values taken from: https://tools.ietf.org/html/rfc3174 + auto s = "abcdbcdecdefdefgefghfghighijhijkijkljklmklmnlmnomnopnopq"; + auto hash = hashString(HashType::htSHA1, s); + ASSERT_EQ(hash.to_string(Base::Base16),"sha1:84983e441c3bd26ebaae4aa1f95129e5e54670f1"); + } + + TEST(hashString, testKnownSHA256Hashes1) { + // values taken from: https://tools.ietf.org/html/rfc4634 + auto s = "abc"; + + auto hash = hashString(HashType::htSHA256, s); + ASSERT_EQ(hash.to_string(Base::Base16), + "sha256:ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad"); + } + + TEST(hashString, testKnownSHA256Hashes2) { + // values taken from: https://tools.ietf.org/html/rfc4634 + auto s = "abcdbcdecdefdefgefghfghighijhijkijkljklmklmnlmnomnopnopq"; + auto hash = hashString(HashType::htSHA256, s); + ASSERT_EQ(hash.to_string(Base::Base16), + "sha256:248d6a61d20638b8e5c026930c3e6039a33ce45964ff2167f6ecedd419db06c1"); + } + + TEST(hashString, testKnownSHA512Hashes1) { + // values taken from: https://tools.ietf.org/html/rfc4634 + auto s = "abc"; + auto hash = hashString(HashType::htSHA512, s); + ASSERT_EQ(hash.to_string(Base::Base16), + "sha512:ddaf35a193617abacc417349ae20413112e6fa4e89a9" + "7ea20a9eeee64b55d39a2192992a274fc1a836ba3c23a3feebbd" + "454d4423643ce80e2a9ac94fa54ca49f"); + } + + TEST(hashString, testKnownSHA512Hashes2) { + // values taken from: https://tools.ietf.org/html/rfc4634 + auto s = "abcdefghbcdefghicdefghijdefghijkefghijklfghijklmghijklmnhijklmnoijklmnopjklmnopqklmnopqrlmnopqrsmnopqrstnopqrstu"; + + auto hash = hashString(HashType::htSHA512, s); + ASSERT_EQ(hash.to_string(Base::Base16), + "sha512:8e959b75dae313da8cf4f72814fc143f8f7779c6eb9f7fa1" + "7299aeadb6889018501d289e4900f7e4331b99dec4b5433a" + "c7d329eeb6dd26545e96e55b874be909"); + } + + TEST(hashString, hashingWithUnknownAlgoExits) { + auto s = "unknown"; + ASSERT_DEATH(hashString(HashType::htUnknown, s), ""); + } +} From c284700867ce09abee1f891a402271a42d207b4d Mon Sep 17 00:00:00 2001 From: Tobias Pflug Date: Mon, 25 May 2020 11:57:45 +0200 Subject: [PATCH 30/58] Add unit tests for "json.hh" --- src/libutil/tests/json.cc | 193 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 193 insertions(+) create mode 100644 src/libutil/tests/json.cc diff --git a/src/libutil/tests/json.cc b/src/libutil/tests/json.cc new file mode 100644 index 000000000..dea73f53a --- /dev/null +++ b/src/libutil/tests/json.cc @@ -0,0 +1,193 @@ +#include "json.hh" +#include +#include + +namespace nix { + + /* ---------------------------------------------------------------------------- + * toJSON + * --------------------------------------------------------------------------*/ + + TEST(toJSON, quotesCharPtr) { + const char* input = "test"; + std::stringstream out; + toJSON(out, input); + + ASSERT_EQ(out.str(), "\"test\""); + } + + TEST(toJSON, quotesStdString) { + std::string input = "test"; + std::stringstream out; + toJSON(out, input); + + ASSERT_EQ(out.str(), "\"test\""); + } + + TEST(toJSON, convertsNullptrtoNull) { + auto input = nullptr; + std::stringstream out; + toJSON(out, input); + + ASSERT_EQ(out.str(), "null"); + } + + TEST(toJSON, convertsNullToNull) { + const char* input = 0; + std::stringstream out; + toJSON(out, input); + + ASSERT_EQ(out.str(), "null"); + } + + + TEST(toJSON, convertsFloat) { + auto input = 1.024f; + std::stringstream out; + toJSON(out, input); + + ASSERT_EQ(out.str(), "1.024"); + } + + TEST(toJSON, convertsDouble) { + const double input = 1.024; + std::stringstream out; + toJSON(out, input); + + ASSERT_EQ(out.str(), "1.024"); + } + + TEST(toJSON, convertsBool) { + auto input = false; + std::stringstream out; + toJSON(out, input); + + ASSERT_EQ(out.str(), "false"); + } + + TEST(toJSON, quotesTab) { + std::stringstream out; + toJSON(out, "\t"); + + ASSERT_EQ(out.str(), "\"\\t\""); + } + + TEST(toJSON, quotesNewline) { + std::stringstream out; + toJSON(out, "\n"); + + ASSERT_EQ(out.str(), "\"\\n\""); + } + + TEST(toJSON, quotesCreturn) { + std::stringstream out; + toJSON(out, "\r"); + + ASSERT_EQ(out.str(), "\"\\r\""); + } + + TEST(toJSON, quotesCreturnNewLine) { + std::stringstream out; + toJSON(out, "\r\n"); + + ASSERT_EQ(out.str(), "\"\\r\\n\""); + } + + TEST(toJSON, quotesDoublequotes) { + std::stringstream out; + toJSON(out, "\""); + + ASSERT_EQ(out.str(), "\"\\\"\""); + } + + TEST(toJSON, substringEscape) { + std::stringstream out; + const char *s = "foo\t"; + toJSON(out, s+3, s + strlen(s)); + + ASSERT_EQ(out.str(), "\"\\t\""); + } + + /* ---------------------------------------------------------------------------- + * JSONObject + * --------------------------------------------------------------------------*/ + + TEST(JSONObject, emptyObject) { + std::stringstream out; + { + JSONObject t(out); + } + ASSERT_EQ(out.str(), "{}"); + } + + TEST(JSONObject, objectWithList) { + std::stringstream out; + { + JSONObject t(out); + auto l = t.list("list"); + l.elem("element"); + } + ASSERT_EQ(out.str(), R"#({"list":["element"]})#"); + } + + TEST(JSONObject, objectWithListIndent) { + std::stringstream out; + { + JSONObject t(out, true); + auto l = t.list("list"); + l.elem("element"); + } + ASSERT_EQ(out.str(), +R"#({ + "list": [ + "element" + ] +})#"); + } + + TEST(JSONObject, objectWithPlaceholderAndList) { + std::stringstream out; + { + JSONObject t(out); + auto l = t.placeholder("list"); + l.list().elem("element"); + } + + ASSERT_EQ(out.str(), R"#({"list":["element"]})#"); + } + + TEST(JSONObject, objectWithPlaceholderAndObject) { + std::stringstream out; + { + JSONObject t(out); + auto l = t.placeholder("object"); + l.object().attr("key", "value"); + } + + ASSERT_EQ(out.str(), R"#({"object":{"key":"value"}})#"); + } + + /* ---------------------------------------------------------------------------- + * JSONList + * --------------------------------------------------------------------------*/ + + TEST(JSONList, empty) { + std::stringstream out; + { + JSONList l(out); + } + ASSERT_EQ(out.str(), R"#([])#"); + } + + TEST(JSONList, withElements) { + std::stringstream out; + { + JSONList l(out); + l.elem("one"); + l.object(); + l.placeholder().write("three"); + } + ASSERT_EQ(out.str(), R"#(["one",{},"three"])#"); + } +} + From 90b0c630a0e9e66f69f1d24b538982c20b5486b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Domen=20Ko=C5=BEar?= Date: Wed, 26 Feb 2020 10:50:40 +0100 Subject: [PATCH 31/58] install-multi-user: allow overriding user count --- scripts/install-multi-user.sh | 4 +++- scripts/install-nix-from-closure.sh | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/install-multi-user.sh b/scripts/install-multi-user.sh index a0f1deb98..991cf1998 100644 --- a/scripts/install-multi-user.sh +++ b/scripts/install-multi-user.sh @@ -20,7 +20,9 @@ readonly GREEN='\033[32m' readonly GREEN_UL='\033[4;32m' readonly RED='\033[31m' -readonly NIX_USER_COUNT="32" +# installer allows overriding build user count to speed up installation +# as creating each user takes non-trivial amount of time on macos +readonly NIX_USER_COUNT=${NIX_USER_COUNT:-32} readonly NIX_BUILD_GROUP_ID="30000" readonly NIX_BUILD_GROUP_NAME="nixbld" readonly NIX_FIRST_BUILD_UID="30001" diff --git a/scripts/install-nix-from-closure.sh b/scripts/install-nix-from-closure.sh index 72aa5abf5..6ea23a44a 100644 --- a/scripts/install-nix-from-closure.sh +++ b/scripts/install-nix-from-closure.sh @@ -50,6 +50,9 @@ while [ $# -gt 0 ]; do INSTALL_MODE=no-daemon;; --no-channel-add) NIX_INSTALLER_NO_CHANNEL_ADD=1;; + --daemon-user-count) + NIX_USER_COUNT=$2 + shift;; --no-modify-profile) NIX_INSTALLER_NO_MODIFY_PROFILE=1;; --darwin-use-unencrypted-nix-store-volume) From 573ff8dfcaacc4495e7f7880d86f70145a074578 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Domen=20Ko=C5=BEar?= Date: Mon, 25 May 2020 17:31:46 +0200 Subject: [PATCH 32/58] Allow passing extra nix.conf to installer --- scripts/install-multi-user.sh | 1 + scripts/install-nix-from-closure.sh | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/scripts/install-multi-user.sh b/scripts/install-multi-user.sh index a0f1deb98..b0cb51943 100644 --- a/scripts/install-multi-user.sh +++ b/scripts/install-multi-user.sh @@ -646,6 +646,7 @@ setup_default_profile() { place_nix_configuration() { cat < "$SCRATCH/nix.conf" +$NIX_EXTRA_CONF build-users-group = $NIX_BUILD_GROUP_NAME EOF _sudo "to place the default nix daemon configuration (part 2)" \ diff --git a/scripts/install-nix-from-closure.sh b/scripts/install-nix-from-closure.sh index 72aa5abf5..3e0312c78 100644 --- a/scripts/install-nix-from-closure.sh +++ b/scripts/install-nix-from-closure.sh @@ -41,6 +41,7 @@ fi INSTALL_MODE=no-daemon CREATE_DARWIN_VOLUME=0 +NIX_EXTRA_CONF= # handle the command line flags while [ $# -gt 0 ]; do case $1 in @@ -54,6 +55,9 @@ while [ $# -gt 0 ]; do NIX_INSTALLER_NO_MODIFY_PROFILE=1;; --darwin-use-unencrypted-nix-store-volume) CREATE_DARWIN_VOLUME=1;; + --nix-extra-conf-file) + NIX_EXTRA_CONF=$(cat $2) + shift;; *) ( echo "Nix Installer [--daemon|--no-daemon] [--no-channel-add] [--no-modify-profile]" From 4b388e84318722f970326967e1648675655740fe Mon Sep 17 00:00:00 2001 From: Tobias Pflug Date: Mon, 25 May 2020 18:34:55 +0200 Subject: [PATCH 33/58] Add unit tests for xml-writer --- src/libutil/tests/xml-writer.cc | 105 ++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 src/libutil/tests/xml-writer.cc diff --git a/src/libutil/tests/xml-writer.cc b/src/libutil/tests/xml-writer.cc new file mode 100644 index 000000000..adcde25c9 --- /dev/null +++ b/src/libutil/tests/xml-writer.cc @@ -0,0 +1,105 @@ +#include "xml-writer.hh" +#include +#include + +namespace nix { + + /* ---------------------------------------------------------------------------- + * XMLWriter + * --------------------------------------------------------------------------*/ + + TEST(XMLWriter, emptyObject) { + std::stringstream out; + { + XMLWriter t(false, out); + } + + ASSERT_EQ(out.str(), "\n"); + } + + TEST(XMLWriter, objectWithEmptyElement) { + std::stringstream out; + { + XMLWriter t(false, out); + t.openElement("foobar"); + } + + ASSERT_EQ(out.str(), "\n"); + } + + TEST(XMLWriter, objectWithElementWithAttrs) { + std::stringstream out; + { + XMLWriter t(false, out); + XMLAttrs attrs = { + { "foo", "bar" } + }; + t.openElement("foobar", attrs); + } + + ASSERT_EQ(out.str(), "\n"); + } + + TEST(XMLWriter, objectWithElementWithEmptyAttrs) { + std::stringstream out; + { + XMLWriter t(false, out); + XMLAttrs attrs = {}; + t.openElement("foobar", attrs); + } + + ASSERT_EQ(out.str(), "\n"); + } + + TEST(XMLWriter, objectWithElementWithAttrsEscaping) { + std::stringstream out; + { + XMLWriter t(false, out); + XMLAttrs attrs = { + { "", "" } + }; + t.openElement("foobar", attrs); + } + + // XXX: While "" is escaped, "" isn't which I think is a bug. + ASSERT_EQ(out.str(), "\n=\"<value>\">"); + } + + TEST(XMLWriter, objectWithElementWithAttrsIndented) { + std::stringstream out; + { + XMLWriter t(true, out); + XMLAttrs attrs = { + { "foo", "bar" } + }; + t.openElement("foobar", attrs); + } + + ASSERT_EQ(out.str(), "\n\n\n"); + } + + TEST(XMLWriter, writeEmptyElement) { + std::stringstream out; + { + XMLWriter t(false, out); + t.writeEmptyElement("foobar"); + } + + ASSERT_EQ(out.str(), "\n"); + } + + TEST(XMLWriter, writeEmptyElementWithAttributes) { + std::stringstream out; + { + XMLWriter t(false, out); + XMLAttrs attrs = { + { "foo", "bar" } + }; + t.writeEmptyElement("foobar", attrs); + + } + + ASSERT_EQ(out.str(), "\n"); + } + +} From 1a5ac894e929cefceec22c0f1ca248ee6be445ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Domen=20Ko=C5=BEar?= Date: Tue, 26 May 2020 15:49:26 +0200 Subject: [PATCH 34/58] Fix installer script bugs - --no-channel-add didn't have effect on multi-user installation - some new flags didn't work at all - document all installer flags --- scripts/install-multi-user.sh | 24 ++++++++++++++---------- scripts/install-nix-from-closure.sh | 13 ++++++++----- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/scripts/install-multi-user.sh b/scripts/install-multi-user.sh index 74cc6a5b0..ab41e0242 100644 --- a/scripts/install-multi-user.sh +++ b/scripts/install-multi-user.sh @@ -29,6 +29,7 @@ readonly NIX_FIRST_BUILD_UID="30001" # Please don't change this. We don't support it, because the # default shell profile that comes with Nix doesn't support it. readonly NIX_ROOT="/nix" +readonly NIX_EXTRA_CONF=${NIX_EXTRA_CONF:-} readonly PROFILE_TARGETS=("/etc/bashrc" "/etc/profile.d/nix.sh" "/etc/zshrc") readonly PROFILE_BACKUP_SUFFIX=".backup-before-nix" @@ -452,9 +453,11 @@ create_directories() { } place_channel_configuration() { - echo "https://nixos.org/channels/nixpkgs-unstable nixpkgs" > "$SCRATCH/.nix-channels" - _sudo "to set up the default system channel (part 1)" \ - install -m 0664 "$SCRATCH/.nix-channels" "$ROOT_HOME/.nix-channels" + if [ -z "$NIX_INSTALLER_NO_CHANNEL_ADD" ]; then + echo "https://nixos.org/channels/nixpkgs-unstable nixpkgs" > "$SCRATCH/.nix-channels" + _sudo "to set up the default system channel (part 1)" \ + install -m 0664 "$SCRATCH/.nix-channels" "$ROOT_HOME/.nix-channels" + fi } welcome_to_nix() { @@ -636,13 +639,14 @@ setup_default_profile() { export NIX_SSL_CERT_FILE=/nix/var/nix/profiles/default/etc/ssl/certs/ca-bundle.crt fi - # Have to explicitly pass NIX_SSL_CERT_FILE as part of the sudo call, - # otherwise it will be lost in environments where sudo doesn't pass - # all the environment variables by default. - _sudo "to update the default channel in the default profile" \ - HOME="$ROOT_HOME" NIX_SSL_CERT_FILE="$NIX_SSL_CERT_FILE" "$NIX_INSTALLED_NIX/bin/nix-channel" --update nixpkgs \ - || channel_update_failed=1 - + if [ -z "$NIX_INSTALLER_NO_CHANNEL_ADD" ]; then + # Have to explicitly pass NIX_SSL_CERT_FILE as part of the sudo call, + # otherwise it will be lost in environments where sudo doesn't pass + # all the environment variables by default. + _sudo "to update the default channel in the default profile" \ + HOME="$ROOT_HOME" NIX_SSL_CERT_FILE="$NIX_SSL_CERT_FILE" "$NIX_INSTALLED_NIX/bin/nix-channel" --update nixpkgs \ + || channel_update_failed=1 + fi } diff --git a/scripts/install-nix-from-closure.sh b/scripts/install-nix-from-closure.sh index 635aaa16d..826ca8b8c 100644 --- a/scripts/install-nix-from-closure.sh +++ b/scripts/install-nix-from-closure.sh @@ -41,7 +41,6 @@ fi INSTALL_MODE=no-daemon CREATE_DARWIN_VOLUME=0 -NIX_EXTRA_CONF= # handle the command line flags while [ $# -gt 0 ]; do case $1 in @@ -50,20 +49,20 @@ while [ $# -gt 0 ]; do --no-daemon) INSTALL_MODE=no-daemon;; --no-channel-add) - NIX_INSTALLER_NO_CHANNEL_ADD=1;; + export NIX_INSTALLER_NO_CHANNEL_ADD=1;; --daemon-user-count) - NIX_USER_COUNT=$2 + export NIX_USER_COUNT=$2 shift;; --no-modify-profile) NIX_INSTALLER_NO_MODIFY_PROFILE=1;; --darwin-use-unencrypted-nix-store-volume) CREATE_DARWIN_VOLUME=1;; --nix-extra-conf-file) - NIX_EXTRA_CONF=$(cat $2) + export NIX_EXTRA_CONF="$(cat $2)" shift;; *) ( - echo "Nix Installer [--daemon|--no-daemon] [--no-channel-add] [--no-modify-profile]" + echo "Nix Installer [--daemon|--no-daemon] [--daemon-user-count INT] [--no-channel-add] [--no-modify-profile] [--darwin-use-unencrypted-nix-store-volume] [--nix-extra-conf-file FILE]" echo "Choose installation method." echo "" @@ -82,6 +81,10 @@ while [ $# -gt 0 ]; do echo " --no-modify-profile: Skip channel installation. When not provided nixpkgs-unstable" echo " is installed by default." echo "" + echo " --daemon-user-count: Number of build users to create. Defaults to 32." + echo "" + echo " --nix-extra-conf-file: Path to nix.conf to prepend when installing /etc/nix.conf" + echo "" ) >&2 # darwin and Catalina+ From 3d3c219d917525b0a131c4332dd65eadfc818f49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Domen=20Ko=C5=BEar?= Date: Tue, 26 May 2020 16:23:03 +0200 Subject: [PATCH 35/58] installer: fix unused variable --- scripts/install-multi-user.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/install-multi-user.sh b/scripts/install-multi-user.sh index ab41e0242..d04b4bbbf 100644 --- a/scripts/install-multi-user.sh +++ b/scripts/install-multi-user.sh @@ -453,7 +453,7 @@ create_directories() { } place_channel_configuration() { - if [ -z "$NIX_INSTALLER_NO_CHANNEL_ADD" ]; then + if [ -z "${NIX_INSTALLER_NO_CHANNEL_ADD:-}" ]; then echo "https://nixos.org/channels/nixpkgs-unstable nixpkgs" > "$SCRATCH/.nix-channels" _sudo "to set up the default system channel (part 1)" \ install -m 0664 "$SCRATCH/.nix-channels" "$ROOT_HOME/.nix-channels" @@ -639,7 +639,7 @@ setup_default_profile() { export NIX_SSL_CERT_FILE=/nix/var/nix/profiles/default/etc/ssl/certs/ca-bundle.crt fi - if [ -z "$NIX_INSTALLER_NO_CHANNEL_ADD" ]; then + if [ -z "${NIX_INSTALLER_NO_CHANNEL_ADD:-}" ]; then # Have to explicitly pass NIX_SSL_CERT_FILE as part of the sudo call, # otherwise it will be lost in environments where sudo doesn't pass # all the environment variables by default. From b90241ceb13500238e428f8943d221f24bf5322b Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Tue, 26 May 2020 11:32:41 -0400 Subject: [PATCH 36/58] Change remaining bools with FileIngestionMethod --- src/libfetchers/git.cc | 4 ++-- src/libfetchers/mercurial.cc | 2 +- src/libfetchers/tarball.cc | 6 +++--- src/libfetchers/tree-info.cc | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 7c18cf67f..17cc60228 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -195,7 +195,7 @@ struct GitInput : Input return files.count(file); }; - auto storePath = store->addToStore("source", actualUrl, true, htSHA256, filter); + auto storePath = store->addToStore("source", actualUrl, FileIngestionMethod::Recursive, htSHA256, filter); auto tree = Tree { .actualPath = store->printStorePath(storePath), @@ -347,7 +347,7 @@ struct GitInput : Input unpackTarfile(*source, tmpDir); } - auto storePath = store->addToStore(name, tmpDir, true, htSHA256, filter); + auto storePath = store->addToStore(name, tmpDir, FileIngestionMethod::Recursive, htSHA256, filter); auto lastModified = std::stoull(runProgram("git", true, { "-C", repoDir, "log", "-1", "--format=%ct", input->rev->gitRev() })); diff --git a/src/libfetchers/mercurial.cc b/src/libfetchers/mercurial.cc index 1d6571571..2e0d4bf4d 100644 --- a/src/libfetchers/mercurial.cc +++ b/src/libfetchers/mercurial.cc @@ -114,7 +114,7 @@ struct MercurialInput : Input return files.count(file); }; - auto storePath = store->addToStore("source", actualUrl, true, htSHA256, filter); + auto storePath = store->addToStore("source", actualUrl, FileIngestionMethod::Recursive, htSHA256, filter); return {Tree { .actualPath = store->printStorePath(storePath), diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index 695525b31..bf2b2a5ff 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -67,10 +67,10 @@ DownloadFileResult downloadFile( StringSink sink; dumpString(*res.data, sink); auto hash = hashString(htSHA256, *res.data); - ValidPathInfo info(store->makeFixedOutputPath(false, hash, name)); + ValidPathInfo info(store->makeFixedOutputPath(FileIngestionMethod::Flat, hash, name)); info.narHash = hashString(htSHA256, *sink.s); info.narSize = sink.s->size(); - info.ca = makeFixedOutputCA(false, hash); + info.ca = makeFixedOutputCA(FileIngestionMethod::Flat, hash); store->addToStore(info, sink.s, NoRepair, NoCheckSigs); storePath = std::move(info.path); } @@ -141,7 +141,7 @@ Tree downloadTarball( throw nix::Error("tarball '%s' contains an unexpected number of top-level files", url); auto topDir = tmpDir + "/" + members.begin()->name; lastModified = lstat(topDir).st_mtime; - unpackedStorePath = store->addToStore(name, topDir, true, htSHA256, defaultPathFilter, NoRepair); + unpackedStorePath = store->addToStore(name, topDir, FileIngestionMethod::Recursive, htSHA256, defaultPathFilter, NoRepair); } Attrs infoAttrs({ diff --git a/src/libfetchers/tree-info.cc b/src/libfetchers/tree-info.cc index 5788e94a1..b2d8cfc8d 100644 --- a/src/libfetchers/tree-info.cc +++ b/src/libfetchers/tree-info.cc @@ -8,7 +8,7 @@ namespace nix::fetchers { StorePath TreeInfo::computeStorePath(Store & store) const { assert(narHash); - return store.makeFixedOutputPath(true, narHash, "source"); + return store.makeFixedOutputPath(FileIngestionMethod::Recursive, narHash, "source"); } } From 93129cf1ddf7f91ddec11ba7a6a364ab54f345c5 Mon Sep 17 00:00:00 2001 From: Andreas Rammhold Date: Tue, 26 May 2020 17:49:31 +0200 Subject: [PATCH 37/58] Add unit tests for config.cc --- src/libutil/tests/config.cc | 204 ++++++++++++++++++++++++++++++++++++ 1 file changed, 204 insertions(+) create mode 100644 src/libutil/tests/config.cc diff --git a/src/libutil/tests/config.cc b/src/libutil/tests/config.cc new file mode 100644 index 000000000..b0be24d5e --- /dev/null +++ b/src/libutil/tests/config.cc @@ -0,0 +1,204 @@ +#include "json.hh" +#include "config.hh" +#include "args.hh" + +#include +#include + +namespace nix { + + /* ---------------------------------------------------------------------------- + * Config + * --------------------------------------------------------------------------*/ + + TEST(Config, setUndefinedSetting) { + Config config; + ASSERT_EQ(config.set("undefined-key", "value"), false); + } + + TEST(Config, setDefinedSetting) { + Config config; + std::string value; + Setting foo{&config, value, "name-of-the-setting", "description"}; + ASSERT_EQ(config.set("name-of-the-setting", "value"), true); + } + + TEST(Config, getDefinedSetting) { + Config config; + std::string value; + std::map settings; + Setting foo{&config, value, "name-of-the-setting", "description"}; + + config.getSettings(settings, /* overridenOnly = */ false); + const auto iter = settings.find("name-of-the-setting"); + ASSERT_NE(iter, settings.end()); + ASSERT_EQ(iter->second.value, ""); + ASSERT_EQ(iter->second.description, "description"); + } + + TEST(Config, getDefinedOverridenSettingNotSet) { + Config config; + std::string value; + std::map settings; + Setting foo{&config, value, "name-of-the-setting", "description"}; + + config.getSettings(settings, /* overridenOnly = */ true); + const auto e = settings.find("name-of-the-setting"); + ASSERT_EQ(e, settings.end()); + } + + TEST(Config, getDefinedSettingSet1) { + Config config; + std::string value; + std::map settings; + Setting setting{&config, value, "name-of-the-setting", "description"}; + + setting.assign("value"); + + config.getSettings(settings, /* overridenOnly = */ false); + const auto iter = settings.find("name-of-the-setting"); + ASSERT_NE(iter, settings.end()); + ASSERT_EQ(iter->second.value, "value"); + ASSERT_EQ(iter->second.description, "description"); + } + + TEST(Config, getDefinedSettingSet2) { + Config config; + std::map settings; + Setting setting{&config, "", "name-of-the-setting", "description"}; + + ASSERT_TRUE(config.set("name-of-the-setting", "value")); + + config.getSettings(settings, /* overridenOnly = */ false); + const auto e = settings.find("name-of-the-setting"); + ASSERT_NE(e, settings.end()); + ASSERT_EQ(e->second.value, "value"); + ASSERT_EQ(e->second.description, "description"); + } + + TEST(Config, addSetting) { + class TestSetting : public AbstractSetting { + public: + TestSetting() : AbstractSetting("test", "test", {}) {} + void set(const std::string & value) {} + std::string to_string() const { return {}; } + }; + + Config config; + TestSetting setting; + + ASSERT_FALSE(config.set("test", "value")); + config.addSetting(&setting); + ASSERT_TRUE(config.set("test", "value")); + } + + TEST(Config, withInitialValue) { + const StringMap initials = { + { "key", "value" }, + }; + Config config(initials); + + { + std::map settings; + config.getSettings(settings, /* overridenOnly = */ false); + ASSERT_EQ(settings.find("key"), settings.end()); + } + + Setting setting{&config, "default-value", "key", "description"}; + + { + std::map settings; + config.getSettings(settings, /* overridenOnly = */ false); + ASSERT_EQ(settings["key"].value, "value"); + } + } + + TEST(Config, resetOverriden) { + Config config; + config.resetOverriden(); + } + + TEST(Config, resetOverridenWithSetting) { + Config config; + Setting setting{&config, "", "name-of-the-setting", "description"}; + + { + std::map settings; + + setting.set("foo"); + ASSERT_EQ(setting.get(), "foo"); + config.getSettings(settings, /* overridenOnly = */ true); + ASSERT_TRUE(settings.empty()); + } + + { + std::map settings; + + setting.override("bar"); + ASSERT_TRUE(setting.overriden); + ASSERT_EQ(setting.get(), "bar"); + config.getSettings(settings, /* overridenOnly = */ true); + ASSERT_FALSE(settings.empty()); + } + + { + std::map settings; + + config.resetOverriden(); + ASSERT_FALSE(setting.overriden); + config.getSettings(settings, /* overridenOnly = */ true); + ASSERT_TRUE(settings.empty()); + } + } + + TEST(Config, toJSONOnEmptyConfig) { + std::stringstream out; + { // Scoped to force the destructor of JSONObject to write the final `}` + JSONObject obj(out); + Config config; + config.toJSON(obj); + } + + ASSERT_EQ(out.str(), "{}"); + } + + TEST(Config, toJSONOnNonEmptyConfig) { + std::stringstream out; + { // Scoped to force the destructor of JSONObject to write the final `}` + JSONObject obj(out); + + Config config; + std::map settings; + Setting setting{&config, "", "name-of-the-setting", "description"}; + setting.assign("value"); + + config.toJSON(obj); + } + ASSERT_EQ(out.str(), R"#({"name-of-the-setting":{"description":"description","value":"value"}})#"); + } + + TEST(Config, setSettingAlias) { + Config config; + Setting setting{&config, "", "some-int", "best number", { "another-int" }}; + ASSERT_TRUE(config.set("some-int", "1")); + ASSERT_EQ(setting.get(), "1"); + ASSERT_TRUE(config.set("another-int", "2")); + ASSERT_EQ(setting.get(), "2"); + ASSERT_TRUE(config.set("some-int", "3")); + ASSERT_EQ(setting.get(), "3"); + } + + /* FIXME: The reapplyUnknownSettings method doesn't seem to do anything + * useful (these days). Whenever we add a new setting to Config the + * unknown settings are always considered. In which case is this function + * actually useful? Is there some way to register a Setting without calling + * addSetting? */ + TEST(Config, DISABLED_reapplyUnknownSettings) { + Config config; + ASSERT_FALSE(config.set("name-of-the-setting", "unknownvalue")); + Setting setting{&config, "default", "name-of-the-setting", "description"}; + ASSERT_EQ(setting.get(), "default"); + config.reapplyUnknownSettings(); + ASSERT_EQ(setting.get(), "unknownvalue"); + } +} From e1b8c64c04b40694418de87e3e0d118cf0677bfc Mon Sep 17 00:00:00 2001 From: Andreas Rammhold Date: Wed, 27 May 2020 14:39:02 +0200 Subject: [PATCH 38/58] config.cc: extract parts of applyConfigFile into applyConfig This moves the actual parsing of configuration contents into applyConfig which applyConfigFile is then going to call. By changing this we can now test the configuration file parsing without actually create a file on disk. --- src/libutil/config.cc | 103 ++++++++++++++++++++++-------------------- src/libutil/config.hh | 1 + 2 files changed, 54 insertions(+), 50 deletions(-) diff --git a/src/libutil/config.cc b/src/libutil/config.cc index f03e444ec..8fc700a2b 100644 --- a/src/libutil/config.cc +++ b/src/libutil/config.cc @@ -65,60 +65,63 @@ void Config::getSettings(std::map & res, bool override res.emplace(opt.first, SettingInfo{opt.second.setting->to_string(), opt.second.setting->description}); } +void AbstractConfig::applyConfig(const std::string & contents, const std::string & path) { + unsigned int pos = 0; + + while (pos < contents.size()) { + string line; + while (pos < contents.size() && contents[pos] != '\n') + line += contents[pos++]; + pos++; + + string::size_type hash = line.find('#'); + if (hash != string::npos) + line = string(line, 0, hash); + + vector tokens = tokenizeString >(line); + if (tokens.empty()) continue; + + if (tokens.size() < 2) + throw UsageError("illegal configuration line '%1%' in '%2%'", line, path); + + auto include = false; + auto ignoreMissing = false; + if (tokens[0] == "include") + include = true; + else if (tokens[0] == "!include") { + include = true; + ignoreMissing = true; + } + + if (include) { + if (tokens.size() != 2) + throw UsageError("illegal configuration line '%1%' in '%2%'", line, path); + auto p = absPath(tokens[1], dirOf(path)); + if (pathExists(p)) { + applyConfigFile(p); + } else if (!ignoreMissing) { + throw Error("file '%1%' included from '%2%' not found", p, path); + } + continue; + } + + if (tokens[1] != "=") + throw UsageError("illegal configuration line '%1%' in '%2%'", line, path); + + string name = tokens[0]; + + vector::iterator i = tokens.begin(); + advance(i, 2); + + set(name, concatStringsSep(" ", Strings(i, tokens.end()))); // FIXME: slow + }; +} + void AbstractConfig::applyConfigFile(const Path & path) { try { string contents = readFile(path); - - unsigned int pos = 0; - - while (pos < contents.size()) { - string line; - while (pos < contents.size() && contents[pos] != '\n') - line += contents[pos++]; - pos++; - - string::size_type hash = line.find('#'); - if (hash != string::npos) - line = string(line, 0, hash); - - vector tokens = tokenizeString >(line); - if (tokens.empty()) continue; - - if (tokens.size() < 2) - throw UsageError("illegal configuration line '%1%' in '%2%'", line, path); - - auto include = false; - auto ignoreMissing = false; - if (tokens[0] == "include") - include = true; - else if (tokens[0] == "!include") { - include = true; - ignoreMissing = true; - } - - if (include) { - if (tokens.size() != 2) - throw UsageError("illegal configuration line '%1%' in '%2%'", line, path); - auto p = absPath(tokens[1], dirOf(path)); - if (pathExists(p)) { - applyConfigFile(p); - } else if (!ignoreMissing) { - throw Error("file '%1%' included from '%2%' not found", p, path); - } - continue; - } - - if (tokens[1] != "=") - throw UsageError("illegal configuration line '%1%' in '%2%'", line, path); - - string name = tokens[0]; - - vector::iterator i = tokens.begin(); - advance(i, 2); - - set(name, concatStringsSep(" ", Strings(i, tokens.end()))); // FIXME: slow - }; + applyConfig(contents, path); } catch (SysError &) { } } diff --git a/src/libutil/config.hh b/src/libutil/config.hh index 7ea78fdaf..b04cba88b 100644 --- a/src/libutil/config.hh +++ b/src/libutil/config.hh @@ -33,6 +33,7 @@ public: virtual void getSettings(std::map & res, bool overridenOnly = false) = 0; + void applyConfig(const std::string & contents, const std::string & path = ""); void applyConfigFile(const Path & path); virtual void resetOverriden() = 0; From 9df3d8ccd7a3130faccb73933d0c196a81d605eb Mon Sep 17 00:00:00 2001 From: Andreas Rammhold Date: Wed, 27 May 2020 14:41:01 +0200 Subject: [PATCH 39/58] tests/config.cc: add tests for Config::applyConfig --- src/libutil/tests/config.cc | 60 +++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/src/libutil/tests/config.cc b/src/libutil/tests/config.cc index b0be24d5e..74c59fd31 100644 --- a/src/libutil/tests/config.cc +++ b/src/libutil/tests/config.cc @@ -201,4 +201,64 @@ namespace nix { config.reapplyUnknownSettings(); ASSERT_EQ(setting.get(), "unknownvalue"); } + + TEST(Config, applyConfigEmpty) { + Config config; + std::map settings; + config.applyConfig(""); + config.getSettings(settings); + ASSERT_TRUE(settings.empty()); + } + + TEST(Config, applyConfigEmptyWithComment) { + Config config; + std::map settings; + config.applyConfig("# just a comment"); + config.getSettings(settings); + ASSERT_TRUE(settings.empty()); + } + + TEST(Config, applyConfigAssignment) { + Config config; + std::map settings; + Setting setting{&config, "", "name-of-the-setting", "description"}; + config.applyConfig( + "name-of-the-setting = value-from-file #useful comment\n" + "# name-of-the-setting = foo\n" + ); + config.getSettings(settings); + ASSERT_FALSE(settings.empty()); + ASSERT_EQ(settings["name-of-the-setting"].value, "value-from-file"); + } + + TEST(Config, applyConfigWithReassignedSetting) { + Config config; + std::map settings; + Setting setting{&config, "", "name-of-the-setting", "description"}; + config.applyConfig( + "name-of-the-setting = first-value\n" + "name-of-the-setting = second-value\n" + ); + config.getSettings(settings); + ASSERT_FALSE(settings.empty()); + ASSERT_EQ(settings["name-of-the-setting"].value, "second-value"); + } + + TEST(Config, applyConfigFailsOnMissingIncludes) { + Config config; + std::map settings; + Setting setting{&config, "", "name-of-the-setting", "description"}; + + ASSERT_THROW(config.applyConfig( + "name-of-the-setting = value-from-file\n" + "# name-of-the-setting = foo\n" + "include /nix/store/does/not/exist.nix" + ), Error); + } + + TEST(Config, applyConfigInvalidThrows) { + Config config; + ASSERT_THROW(config.applyConfig("value == key"), UsageError); + ASSERT_THROW(config.applyConfig("value "), UsageError); + } } From fc137d2f007c32f27a49e1a00d5114c7d7663419 Mon Sep 17 00:00:00 2001 From: Andreas Rammhold Date: Wed, 27 May 2020 16:37:24 +0200 Subject: [PATCH 40/58] config.hh: Add documentation Provides some general overview on the mechanics of Config/Setting and comments for the public methods of Config. --- src/libutil/config.hh | 69 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/src/libutil/config.hh b/src/libutil/config.hh index b04cba88b..5c7a70a2e 100644 --- a/src/libutil/config.hh +++ b/src/libutil/config.hh @@ -7,6 +7,38 @@ namespace nix { +/** + * The Config class provides Nix runtime configurations. + * + * What is a Configuration? + * A collection of uniquely named Settings. + * + * What is a Setting? + * Each property that you can set in a configuration corresponds to a + * `Setting`. A setting records value and description of a property + * with a default and optional aliases. + * + * A valid configuration consists of settings that are registered to a + * `Config` object instance: + * + * Config config; + * Setting systemSetting{&config, "x86_64-linux", "system", "the current system"}; + * + * The above creates a `Config` object and registers a setting called "system" + * via the variable `systemSetting` with it. The setting defaults to the string + * "x86_64-linux", it's description is "the current system". All of the + * registered settings can then be accessed as shown below: + * + * std::map settings; + * config.getSettings(settings); + * config["system"].description == "the current system" + * config["system"].value == "x86_64-linux" + * + * + * The above retrieves all currently known settings from the `Config` object + * and adds them to the `settings` map. + */ + class Args; class AbstractSetting; class JSONPlaceholder; @@ -23,6 +55,10 @@ protected: public: + /** + * Sets the value referenced by `name` to `value`. Returns true if the + * setting is known, false otherwise. + */ virtual bool set(const std::string & name, const std::string & value) = 0; struct SettingInfo @@ -31,19 +67,52 @@ public: std::string description; }; + /** + * Adds the currently known settings to the given result map `res`. + * - res: map to store settings in + * - overridenOnly: when set to true only overridden settings will be added to `res` + */ virtual void getSettings(std::map & res, bool overridenOnly = false) = 0; + /** + * Parses the configuration in `contents` and applies it + * - contents: configuration contents to be parsed and applied + * - path: location of the configuration file + */ void applyConfig(const std::string & contents, const std::string & path = ""); + + /** + * Applies a nix configuration file + * - path: the location of the config file to apply + */ void applyConfigFile(const Path & path); + /** + * Resets the `overridden` flag of all Settings + */ virtual void resetOverriden() = 0; + /** + * Outputs all settings to JSON + * - out: JSONObject to write the configuration to + */ virtual void toJSON(JSONObject & out) = 0; + /** + * Converts settings to `Args` to be used on the command line interface + * - args: args to write to + * - category: category of the settings + */ virtual void convertToArgs(Args & args, const std::string & category) = 0; + /** + * Logs a warning for each unregistered setting + */ void warnUnknownSettings(); + /** + * Re-applies all previously attempted changes to unknown settings + */ void reapplyUnknownSettings(); }; From 7873fd175d9e4e5bdb281e005b18c388bcfe1dd8 Mon Sep 17 00:00:00 2001 From: Matthew Bauer Date: Wed, 27 May 2020 13:11:45 -0500 Subject: [PATCH 41/58] =?UTF-8?q?Don=E2=80=99t=20use=20FileIngestionMethod?= =?UTF-8?q?=20for=20StorePathsCommand?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a different recursive than used in makeFixedOutputPath. --- src/nix/command.cc | 10 +++++----- src/nix/command.hh | 4 ++-- src/nix/copy.cc | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/nix/command.cc b/src/nix/command.cc index ea0ade88e..71b027719 100644 --- a/src/nix/command.cc +++ b/src/nix/command.cc @@ -31,21 +31,21 @@ void StoreCommand::run() run(getStore()); } -StorePathsCommand::StorePathsCommand(FileIngestionMethod recursive) +StorePathsCommand::StorePathsCommand(bool recursive) : recursive(recursive) { - if (recursive == FileIngestionMethod::Recursive) + if (recursive) addFlag({ .longName = "no-recursive", .description = "apply operation to specified paths only", - .handler = {&this->recursive, FileIngestionMethod::Flat}, + .handler = {&this->recursive, false}, }); else addFlag({ .longName = "recursive", .shortName = 'r', .description = "apply operation to closure of the specified paths", - .handler = {&this->recursive, FileIngestionMethod::Recursive}, + .handler = {&this->recursive, true}, }); mkFlag(0, "all", "apply operation to the entire store", &all); @@ -66,7 +66,7 @@ void StorePathsCommand::run(ref store) for (auto & p : toStorePaths(store, realiseMode, installables)) storePaths.push_back(p.clone()); - if (recursive == FileIngestionMethod::Recursive) { + if (recursive) { StorePathSet closure; store->computeFSClosure(storePathsToSet(storePaths), closure, false, false); storePaths.clear(); diff --git a/src/nix/command.hh b/src/nix/command.hh index 09c621b5b..959d5f19d 100644 --- a/src/nix/command.hh +++ b/src/nix/command.hh @@ -92,7 +92,7 @@ struct StorePathsCommand : public InstallablesCommand { private: - FileIngestionMethod recursive = FileIngestionMethod::Flat; + bool recursive = false; bool all = false; protected: @@ -101,7 +101,7 @@ protected: public: - StorePathsCommand(FileIngestionMethod recursive = FileIngestionMethod::Flat); + StorePathsCommand(bool recursive = false); using StoreCommand::run; diff --git a/src/nix/copy.cc b/src/nix/copy.cc index 60aa3f14b..c7c38709d 100644 --- a/src/nix/copy.cc +++ b/src/nix/copy.cc @@ -17,7 +17,7 @@ struct CmdCopy : StorePathsCommand SubstituteFlag substitute = NoSubstitute; CmdCopy() - : StorePathsCommand(FileIngestionMethod::Recursive) + : StorePathsCommand(true) { addFlag({ .longName = "from", From c66441a646c2bbfab05fa4a4fd05c2ae897dda8b Mon Sep 17 00:00:00 2001 From: Matthew Bauer Date: Wed, 27 May 2020 13:04:20 -0500 Subject: [PATCH 42/58] =?UTF-8?q?Rename=20some=20variables=20named=20?= =?UTF-8?q?=E2=80=9Crecursive=E2=80=9D=20to=20=E2=80=9Cmethod=E2=80=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is much less confusing since recursive is no longer a boolean. --- src/libexpr/primops.cc | 14 +++++++------- src/libstore/binary-cache-store.cc | 6 +++--- src/libstore/binary-cache-store.hh | 2 +- src/libstore/build.cc | 18 +++++++++--------- src/libstore/local-store.cc | 22 +++++++++++----------- src/libstore/store-api.cc | 6 +++--- 6 files changed, 34 insertions(+), 34 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index fcbb75ceb..d458ab272 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1042,7 +1042,7 @@ static void prim_toFile(EvalState & state, const Pos & pos, Value * * args, Valu static void addPath(EvalState & state, const Pos & pos, const string & name, const Path & path_, - Value * filterFun, FileIngestionMethod recursive, const Hash & expectedHash, Value & v) + Value * filterFun, FileIngestionMethod method, const Hash & expectedHash, Value & v) { const auto path = evalSettings.pureEval && expectedHash ? path_ : @@ -1073,12 +1073,12 @@ static void addPath(EvalState & state, const Pos & pos, const string & name, con std::optional expectedStorePath; if (expectedHash) - expectedStorePath = state.store->makeFixedOutputPath(recursive, expectedHash, name); + expectedStorePath = state.store->makeFixedOutputPath(method, expectedHash, name); Path dstPath; if (!expectedHash || !state.store->isValidPath(*expectedStorePath)) { dstPath = state.store->printStorePath(settings.readOnlyMode - ? state.store->computeStorePathForPath(name, path, recursive, htSHA256, filter).first - : state.store->addToStore(name, path, recursive, htSHA256, filter, state.repair)); + ? state.store->computeStorePathForPath(name, path, method, htSHA256, filter).first + : state.store->addToStore(name, path, method, htSHA256, filter, state.repair)); if (expectedHash && expectedStorePath != state.store->parseStorePath(dstPath)) throw Error("store path mismatch in (possibly filtered) path added from '%s'", path); } else @@ -1108,7 +1108,7 @@ static void prim_path(EvalState & state, const Pos & pos, Value * * args, Value Path path; string name; Value * filterFun = nullptr; - auto recursive = FileIngestionMethod::Recursive; + auto method = FileIngestionMethod::Recursive; Hash expectedHash; for (auto & attr : *args[0]->attrs) { @@ -1124,7 +1124,7 @@ static void prim_path(EvalState & state, const Pos & pos, Value * * args, Value state.forceValue(*attr.value, pos); filterFun = attr.value; } else if (n == "recursive") - recursive = FileIngestionMethod { state.forceBool(*attr.value, *attr.pos) }; + method = FileIngestionMethod { state.forceBool(*attr.value, *attr.pos) }; else if (n == "sha256") expectedHash = Hash(state.forceStringNoCtx(*attr.value, *attr.pos), htSHA256); else @@ -1135,7 +1135,7 @@ static void prim_path(EvalState & state, const Pos & pos, Value * * args, Value if (name.empty()) name = baseNameOf(path); - addPath(state, pos, name, path, filterFun, recursive, expectedHash, v); + addPath(state, pos, name, path, filterFun, method, expectedHash, v); } diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index a61af4a00..f13736c58 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -327,7 +327,7 @@ void BinaryCacheStore::queryPathInfoUncached(const StorePath & storePath, } StorePath BinaryCacheStore::addToStore(const string & name, const Path & srcPath, - FileIngestionMethod recursive, HashType hashAlgo, PathFilter & filter, RepairFlag repair) + FileIngestionMethod method, HashType hashAlgo, PathFilter & filter, RepairFlag repair) { // FIXME: some cut&paste from LocalStore::addToStore(). @@ -336,7 +336,7 @@ StorePath BinaryCacheStore::addToStore(const string & name, const Path & srcPath small files. */ StringSink sink; Hash h; - if (recursive == FileIngestionMethod::Recursive) { + if (method == FileIngestionMethod::Recursive) { dumpPath(srcPath, sink, filter); h = hashString(hashAlgo, *sink.s); } else { @@ -345,7 +345,7 @@ StorePath BinaryCacheStore::addToStore(const string & name, const Path & srcPath h = hashString(hashAlgo, s); } - ValidPathInfo info(makeFixedOutputPath(recursive, h, name)); + ValidPathInfo info(makeFixedOutputPath(method, h, name)); addToStore(info, sink.s, repair, CheckSigs, nullptr); diff --git a/src/libstore/binary-cache-store.hh b/src/libstore/binary-cache-store.hh index 1a1ea636c..4ff5609ed 100644 --- a/src/libstore/binary-cache-store.hh +++ b/src/libstore/binary-cache-store.hh @@ -79,7 +79,7 @@ public: std::shared_ptr accessor) override; StorePath addToStore(const string & name, const Path & srcPath, - FileIngestionMethod recursive, HashType hashAlgo, + FileIngestionMethod method, HashType hashAlgo, PathFilter & filter, RepairFlag repair) override; StorePath addTextToStore(const string & name, const string & s, diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 272c4f1ae..f5c132a83 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2734,7 +2734,7 @@ struct RestrictedStore : public LocalFSStore { throw Error("queryPathFromHashPart"); } StorePath addToStore(const string & name, const Path & srcPath, - FileIngestionMethod recursive = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, + FileIngestionMethod method = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, PathFilter & filter = defaultPathFilter, RepairFlag repair = NoRepair) override { throw Error("addToStore"); } @@ -2747,9 +2747,9 @@ struct RestrictedStore : public LocalFSStore } StorePath addToStoreFromDump(const string & dump, const string & name, - FileIngestionMethod recursive = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, RepairFlag repair = NoRepair) override + FileIngestionMethod method = FileIngestionMethod::Recursive, HashType hashAlgo = htSHA256, RepairFlag repair = NoRepair) override { - auto path = next->addToStoreFromDump(dump, name, recursive, hashAlgo, repair); + auto path = next->addToStoreFromDump(dump, name, method, hashAlgo, repair); goal.addDependency(path); return path; } @@ -3692,10 +3692,10 @@ void DerivationGoal::registerOutputs() if (fixedOutput) { - FileIngestionMethod recursive; Hash h; - i.second.parseHashInfo(recursive, h); + FileIngestionMethod outputHashMode; Hash h; + i.second.parseHashInfo(outputHashMode, h); - if (recursive == FileIngestionMethod::Flat) { + if (outputHashMode == FileIngestionMethod::Flat) { /* The output path should be a regular file without execute permission. */ if (!S_ISREG(st.st_mode) || (st.st_mode & S_IXUSR) != 0) throw BuildError( @@ -3705,11 +3705,11 @@ void DerivationGoal::registerOutputs() /* Check the hash. In hash mode, move the path produced by the derivation to its content-addressed location. */ - Hash h2 = recursive == FileIngestionMethod::Recursive + Hash h2 = outputHashMode == FileIngestionMethod::Recursive ? hashPath(h.type, actualPath).first : hashFile(h.type, actualPath); - auto dest = worker.store.makeFixedOutputPath(recursive, h2, i.second.path.name()); + auto dest = worker.store.makeFixedOutputPath(outputHashMode, h2, i.second.path.name()); if (h != h2) { @@ -3738,7 +3738,7 @@ void DerivationGoal::registerOutputs() else assert(worker.store.parseStorePath(path) == dest); - ca = makeFixedOutputCA(recursive, h2); + ca = makeFixedOutputCA(outputHashMode, h2); } /* Get rid of all weird permissions. This also checks that diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 746f81beb..80851b591 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -557,10 +557,10 @@ void LocalStore::checkDerivationOutputs(const StorePath & drvPath, const Derivat if (out == drv.outputs.end()) throw Error("derivation '%s' does not have an output named 'out'", printStorePath(drvPath)); - FileIngestionMethod recursive; Hash h; - out->second.parseHashInfo(recursive, h); + FileIngestionMethod method; Hash h; + out->second.parseHashInfo(method, h); - check(makeFixedOutputPath(recursive, h, drvName), out->second.path, "out"); + check(makeFixedOutputPath(method, h, drvName), out->second.path, "out"); } else { @@ -1043,11 +1043,11 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, StorePath LocalStore::addToStoreFromDump(const string & dump, const string & name, - FileIngestionMethod recursive, HashType hashAlgo, RepairFlag repair) + FileIngestionMethod method, HashType hashAlgo, RepairFlag repair) { Hash h = hashString(hashAlgo, dump); - auto dstPath = makeFixedOutputPath(recursive, h, name); + auto dstPath = makeFixedOutputPath(method, h, name); addTempRoot(dstPath); @@ -1067,7 +1067,7 @@ StorePath LocalStore::addToStoreFromDump(const string & dump, const string & nam autoGC(); - if (recursive == FileIngestionMethod::Recursive) { + if (method == FileIngestionMethod::Recursive) { StringSource source(dump); restorePath(realPath, source); } else @@ -1080,7 +1080,7 @@ StorePath LocalStore::addToStoreFromDump(const string & dump, const string & nam above (if called with recursive == true and hashAlgo == sha256); otherwise, compute it here. */ HashResult hash; - if (recursive == FileIngestionMethod::Recursive) { + if (method == FileIngestionMethod::Recursive) { hash.first = hashAlgo == htSHA256 ? h : hashString(htSHA256, dump); hash.second = dump.size(); } else @@ -1091,7 +1091,7 @@ StorePath LocalStore::addToStoreFromDump(const string & dump, const string & nam ValidPathInfo info(dstPath.clone()); info.narHash = hash.first; info.narSize = hash.second; - info.ca = makeFixedOutputCA(recursive, h); + info.ca = makeFixedOutputCA(method, h); registerValidPath(info); } @@ -1103,7 +1103,7 @@ StorePath LocalStore::addToStoreFromDump(const string & dump, const string & nam StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, - FileIngestionMethod recursive, HashType hashAlgo, PathFilter & filter, RepairFlag repair) + FileIngestionMethod method, HashType hashAlgo, PathFilter & filter, RepairFlag repair) { Path srcPath(absPath(_srcPath)); @@ -1111,12 +1111,12 @@ StorePath LocalStore::addToStore(const string & name, const Path & _srcPath, method for very large paths, but `copyPath' is mainly used for small files. */ StringSink sink; - if (recursive == FileIngestionMethod::Recursive) + if (method == FileIngestionMethod::Recursive) dumpPath(srcPath, sink, filter); else sink.s = make_ref(readFile(srcPath)); - return addToStoreFromDump(*sink.s, name, recursive, hashAlgo, repair); + return addToStoreFromDump(*sink.s, name, method, hashAlgo, repair); } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 86cb20a26..d479b86cb 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -204,12 +204,12 @@ StorePath Store::makeTextPath(std::string_view name, const Hash & hash, std::pair Store::computeStorePathForPath(std::string_view name, - const Path & srcPath, FileIngestionMethod recursive, HashType hashAlgo, PathFilter & filter) const + const Path & srcPath, FileIngestionMethod method, HashType hashAlgo, PathFilter & filter) const { - Hash h = recursive == FileIngestionMethod::Recursive + Hash h = method == FileIngestionMethod::Recursive ? hashPath(hashAlgo, srcPath, filter).first : hashFile(hashAlgo, srcPath); - return std::make_pair(makeFixedOutputPath(recursive, h, name), h); + return std::make_pair(makeFixedOutputPath(method, h, name), h); } From 4e6d7cb55a884ac19990b487c2020ea257e0f9c3 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Wed, 27 May 2020 20:58:42 +0200 Subject: [PATCH 43/58] installer: don't require xz on darwin On macOS the system tar has builtin support for lzma while xz isn't available as a separate binary. There's no builtin package manager there available either so having to install lzma (without nix) would be rather painful. --- scripts/install.in | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/install.in b/scripts/install.in index 6709f00d4..1d26c4ff0 100644 --- a/scripts/install.in +++ b/scripts/install.in @@ -36,7 +36,9 @@ tarball="$tmpDir/$(basename "$tmpDir/nix-@nixVersion@-$system.tar.xz")" require_util curl "download the binary tarball" require_util tar "unpack the binary tarball" -require_util xz "unpack the binary tarball" +if [ "$(uname -s)" != "Darwin" ]; then + require_util xz "unpack the binary tarball" +fi echo "downloading Nix @nixVersion@ binary tarball for $system from '$url' to '$tmpDir'..." curl -L "$url" -o "$tarball" || oops "failed to download '$url'" From 0f96f45061e574c9cf217e3dd5634dfdbf210235 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 27 May 2020 23:50:11 -0400 Subject: [PATCH 44/58] Use `FileIngestionMethod` for `nix hash` There was an enum there that matched in perfectly. --- src/nix/hash.cc | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/nix/hash.cc b/src/nix/hash.cc index 366314227..0a7c343d0 100644 --- a/src/nix/hash.cc +++ b/src/nix/hash.cc @@ -9,15 +9,14 @@ using namespace nix; struct CmdHash : Command { - enum Mode { mFile, mPath }; - Mode mode; + FileIngestionMethod mode; Base base = SRI; bool truncate = false; HashType ht = htSHA256; std::vector paths; std::optional modulus; - CmdHash(Mode mode) : mode(mode) + CmdHash(FileIngestionMethod mode) : mode(mode) { mkFlag(0, "sri", "print hash in SRI format", &base, SRI); mkFlag(0, "base64", "print hash in base-64", &base, Base64); @@ -36,9 +35,14 @@ struct CmdHash : Command std::string description() override { - return mode == mFile - ? "print cryptographic hash of a regular file" - : "print cryptographic hash of the NAR serialisation of a path"; + const char* d; + switch (mode) { + case FileIngestionMethod::Flat: + d = "print cryptographic hash of a regular file"; + case FileIngestionMethod::Recursive: + d = "print cryptographic hash of the NAR serialisation of a path"; + }; + return d; } Category category() override { return catUtility; } @@ -53,10 +57,14 @@ struct CmdHash : Command else hashSink = std::make_unique(ht); - if (mode == mFile) + switch (mode) { + case FileIngestionMethod::Flat: readFile(path, *hashSink); - else + break; + case FileIngestionMethod::Recursive: dumpPath(path, *hashSink); + break; + } Hash h = hashSink->finish().first; if (truncate && h.hashSize > 20) h = compressHash(h, 20); @@ -65,8 +73,8 @@ struct CmdHash : Command } }; -static RegisterCommand r1("hash-file", [](){ return make_ref(CmdHash::mFile); }); -static RegisterCommand r2("hash-path", [](){ return make_ref(CmdHash::mPath); }); +static RegisterCommand r1("hash-file", [](){ return make_ref(FileIngestionMethod::Flat); }); +static RegisterCommand r2("hash-path", [](){ return make_ref(FileIngestionMethod::Recursive); }); struct CmdToBase : Command { @@ -137,7 +145,7 @@ static int compatNixHash(int argc, char * * argv) }); if (op == opHash) { - CmdHash cmd(flat ? CmdHash::mFile : CmdHash::mPath); + CmdHash cmd(flat ? FileIngestionMethod::Flat : FileIngestionMethod::Recursive); cmd.ht = ht; cmd.base = base32 ? Base32 : Base16; cmd.truncate = truncate; From fac0c2d54a6b04175b40009506f2720d2594ed4e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 29 May 2020 16:19:48 -0400 Subject: [PATCH 45/58] Remove addToStore variant as requested by `FIXME` The idea is it's always more flexible to consumer a `Source` than a plain string, and it might even reduce memory consumption. I also looked at `addToStoreFromDump` with its `// FIXME: remove?`, but the worked needed for that is far more up for interpretation, so I punted for now. --- src/libfetchers/tarball.cc | 3 ++- src/libstore/binary-cache-store.cc | 11 ++++++++--- src/libstore/binary-cache-store.hh | 2 +- src/libstore/export-import.cc | 5 ++++- src/libstore/store-api.cc | 15 --------------- src/libstore/store-api.hh | 7 +------ src/nix/add-to-store.cc | 6 ++++-- 7 files changed, 20 insertions(+), 29 deletions(-) diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index bf2b2a5ff..b6e57379b 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -71,7 +71,8 @@ DownloadFileResult downloadFile( info.narHash = hashString(htSHA256, *sink.s); info.narSize = sink.s->size(); info.ca = makeFixedOutputCA(FileIngestionMethod::Flat, hash); - store->addToStore(info, sink.s, NoRepair, NoCheckSigs); + auto source = StringSource { *sink.s }; + store->addToStore(info, source, NoRepair, NoCheckSigs); storePath = std::move(info.path); } diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index f13736c58..357962007 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -113,9 +113,12 @@ void BinaryCacheStore::writeNarInfo(ref narInfo) diskCache->upsertNarInfo(getUri(), hashPart, std::shared_ptr(narInfo)); } -void BinaryCacheStore::addToStore(const ValidPathInfo & info, const ref & nar, +void BinaryCacheStore::addToStore(const ValidPathInfo & info, Source & narSource, RepairFlag repair, CheckSigsFlag checkSigs, std::shared_ptr accessor) { + // FIXME: See if we can use the original source to reduce memory usage. + auto nar = make_ref(narSource.drain()); + if (!repair && isValidPath(info.path)) return; /* Verify that all references are valid. This may do some .narinfo @@ -347,7 +350,8 @@ StorePath BinaryCacheStore::addToStore(const string & name, const Path & srcPath ValidPathInfo info(makeFixedOutputPath(method, h, name)); - addToStore(info, sink.s, repair, CheckSigs, nullptr); + auto source = StringSource { *sink.s }; + addToStore(info, source, repair, CheckSigs, nullptr); return std::move(info.path); } @@ -361,7 +365,8 @@ StorePath BinaryCacheStore::addTextToStore(const string & name, const string & s if (repair || !isValidPath(info.path)) { StringSink sink; dumpString(s, sink); - addToStore(info, sink.s, repair, CheckSigs, nullptr); + auto source = StringSource { *sink.s }; + addToStore(info, source, repair, CheckSigs, nullptr); } return std::move(info.path); diff --git a/src/libstore/binary-cache-store.hh b/src/libstore/binary-cache-store.hh index 4ff5609ed..52ef8aa7a 100644 --- a/src/libstore/binary-cache-store.hh +++ b/src/libstore/binary-cache-store.hh @@ -74,7 +74,7 @@ public: std::optional queryPathFromHashPart(const std::string & hashPart) override { unsupported("queryPathFromHashPart"); } - void addToStore(const ValidPathInfo & info, const ref & nar, + void addToStore(const ValidPathInfo & info, Source & narSource, RepairFlag repair, CheckSigsFlag checkSigs, std::shared_ptr accessor) override; diff --git a/src/libstore/export-import.cc b/src/libstore/export-import.cc index 4692d1a7b..f0d01a240 100644 --- a/src/libstore/export-import.cc +++ b/src/libstore/export-import.cc @@ -1,3 +1,4 @@ +#include "serialise.hh" #include "store-api.hh" #include "archive.hh" #include "worker-protocol.hh" @@ -100,7 +101,9 @@ StorePaths Store::importPaths(Source & source, std::shared_ptr acces if (readInt(source) == 1) readString(source); - addToStore(info, tee.source.data, NoRepair, checkSigs, accessor); + // Can't use underlying source, which would have been exhausted + auto source = StringSource { *tee.source.data }; + addToStore(info, source, NoRepair, checkSigs, accessor); res.push_back(info.path.clone()); } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index d479b86cb..095363d0c 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -840,21 +840,6 @@ std::string makeFixedOutputCA(FileIngestionMethod recursive, const Hash & hash) } -void Store::addToStore(const ValidPathInfo & info, Source & narSource, - RepairFlag repair, CheckSigsFlag checkSigs, - std::shared_ptr accessor) -{ - addToStore(info, make_ref(narSource.drain()), repair, checkSigs, accessor); -} - -void Store::addToStore(const ValidPathInfo & info, const ref & nar, - RepairFlag repair, CheckSigsFlag checkSigs, - std::shared_ptr accessor) -{ - StringSource source(*nar); - addToStore(info, source, repair, checkSigs, accessor); -} - } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 3def209a6..b1e25fc7d 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -450,12 +450,7 @@ public: /* Import a path into the store. */ virtual void addToStore(const ValidPathInfo & info, Source & narSource, RepairFlag repair = NoRepair, CheckSigsFlag checkSigs = CheckSigs, - std::shared_ptr accessor = 0); - - // FIXME: remove - virtual void addToStore(const ValidPathInfo & info, const ref & nar, - RepairFlag repair = NoRepair, CheckSigsFlag checkSigs = CheckSigs, - std::shared_ptr accessor = 0); + std::shared_ptr accessor = 0) = 0; /* Copy the contents of a path to the store and register the validity the resulting path. The resulting path is returned. diff --git a/src/nix/add-to-store.cc b/src/nix/add-to-store.cc index 4b4ba81cb..f43f774c1 100644 --- a/src/nix/add-to-store.cc +++ b/src/nix/add-to-store.cc @@ -50,8 +50,10 @@ struct CmdAddToStore : MixDryRun, StoreCommand info.narSize = sink.s->size(); info.ca = makeFixedOutputCA(FileIngestionMethod::Recursive, info.narHash); - if (!dryRun) - store->addToStore(info, sink.s); + if (!dryRun) { + auto source = StringSource { *sink.s }; + store->addToStore(info, source); + } logger->stdout("%s", store->printStorePath(info.path)); } From 77007d4eabcf7090d1d3fbbdf84a67fb2262cf78 Mon Sep 17 00:00:00 2001 From: Nikola Knezevic Date: Sat, 30 May 2020 12:29:35 +0200 Subject: [PATCH 46/58] Improve ref validity checking in fetchGit The previous regex was too strict and did not match what git was allowing. It could lead to `fetchGit` not accepting valid branch names, even though they exist in a repository (for example, branch names containing `/`, which are pretty standard, like `release/1.0` branches). The new regex defines what a branch name should **NOT** contain. It takes the definitions from `refs.c` in https://github.com/git/git and `git help check-ref-format` pages. This change also introduces a test for ref name validity checking, which compares the result from Nix with the result of `git check-ref-format --branch`. --- src/libfetchers/git.cc | 2 +- src/libutil/url.cc | 1 + src/libutil/url.hh | 6 +++ tests/fetchGitRefs.sh | 111 +++++++++++++++++++++++++++++++++++++++++ tests/local.mk | 1 + 5 files changed, 120 insertions(+), 1 deletion(-) create mode 100644 tests/fetchGitRefs.sh diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 17cc60228..e069057eb 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -418,7 +418,7 @@ struct GitInputScheme : InputScheme auto input = std::make_unique(parseURL(getStrAttr(attrs, "url"))); if (auto ref = maybeGetStrAttr(attrs, "ref")) { - if (!std::regex_match(*ref, refRegex)) + if (std::regex_search(*ref, badGitRefRegex)) throw BadURL("invalid Git branch/tag name '%s'", *ref); input->ref = *ref; } diff --git a/src/libutil/url.cc b/src/libutil/url.cc index 5d5328e5d..88c09eef9 100644 --- a/src/libutil/url.cc +++ b/src/libutil/url.cc @@ -4,6 +4,7 @@ namespace nix { std::regex refRegex(refRegexS, std::regex::ECMAScript); +std::regex badGitRefRegex(badGitRefRegexS, std::regex::ECMAScript); std::regex revRegex(revRegexS, std::regex::ECMAScript); std::regex flakeIdRegex(flakeIdRegexS, std::regex::ECMAScript); diff --git a/src/libutil/url.hh b/src/libutil/url.hh index 1503023a2..4a0d4071b 100644 --- a/src/libutil/url.hh +++ b/src/libutil/url.hh @@ -49,6 +49,12 @@ const static std::string pathRegex = "(?:" + segmentRegex + "(?:/" + segmentRege const static std::string refRegexS = "[a-zA-Z0-9][a-zA-Z0-9_.-]*"; // FIXME: check extern std::regex refRegex; +// Instead of defining what a good Git Ref is, we define what a bad Git Ref is +// This is because of the definition of a ref in refs.c in https://github.com/git/git +// See tests/fetchGitRefs.sh for the full definition +const static std::string badGitRefRegexS = "//|^[./]|/\\.|\\.\\.|[[:cntrl:][:space:]:?^~\[]|\\\\|\\*|\\.lock$|\\.lock/|@\\{|[/.]$|^@$|^$"; +extern std::regex badGitRefRegex; + // A Git revision (a SHA-1 commit hash). const static std::string revRegexS = "[0-9a-fA-F]{40}"; extern std::regex revRegex; diff --git a/tests/fetchGitRefs.sh b/tests/fetchGitRefs.sh new file mode 100644 index 000000000..93993ae90 --- /dev/null +++ b/tests/fetchGitRefs.sh @@ -0,0 +1,111 @@ +source common.sh + +if [[ -z $(type -p git) ]]; then + echo "Git not installed; skipping Git tests" + exit 99 +fi + +clearStore + +repo="$TEST_ROOT/git" + +rm -rf "$repo" "${repo}-tmp" "$TEST_HOME/.cache/nix" + +git init "$repo" +git -C "$repo" config user.email "foobar@example.com" +git -C "$repo" config user.name "Foobar" + +echo utrecht > "$repo"/hello +git -C "$repo" add hello +git -C "$repo" commit -m 'Bla1' + +path=$(nix eval --raw "(builtins.fetchGit { url = $repo; ref = \"master\"; }).outPath") + +# Test various combinations of ref names +# (taken from the git project) + +# git help check-ref-format +# Git imposes the following rules on how references are named: +# +# 1. They can include slash / for hierarchical (directory) grouping, but no slash-separated component can begin with a dot . or end with the sequence .lock. +# 2. They must contain at least one /. This enforces the presence of a category like heads/, tags/ etc. but the actual names are not restricted. If the --allow-onelevel option is used, this rule is waived. +# 3. They cannot have two consecutive dots .. anywhere. +# 4. They cannot have ASCII control characters (i.e. bytes whose values are lower than \040, or \177 DEL), space, tilde ~, caret ^, or colon : anywhere. +# 5. They cannot have question-mark ?, asterisk *, or open bracket [ anywhere. See the --refspec-pattern option below for an exception to this rule. +# 6. They cannot begin or end with a slash / or contain multiple consecutive slashes (see the --normalize option below for an exception to this rule) +# 7. They cannot end with a dot .. +# 8. They cannot contain a sequence @{. +# 9. They cannot be the single character @. +# 10. They cannot contain a \. + +valid_ref() { + { set +x; printf >&2 '\n>>>>>>>>>> valid_ref %s\b <<<<<<<<<<\n' $(printf %s "$1" | sed -n -e l); set -x; } + git check-ref-format --branch "$1" >/dev/null + git -C "$repo" branch "$1" master >/dev/null + path1=$(nix eval --raw "(builtins.fetchGit { url = $repo; ref = ''$1''; }).outPath") + [[ $path1 = $path ]] + git -C "$repo" branch -D "$1" >/dev/null +} + +invalid_ref() { + { set +x; printf >&2 '\n>>>>>>>>>> invalid_ref %s\b <<<<<<<<<<\n' $(printf %s "$1" | sed -n -e l); set -x; } + # special case for a sole @: + # --branch @ will try to interpret @ as a branch reference and not fail. Thus we need --allow-onelevel + if [ "$1" = "@" ]; then + (! git check-ref-format --allow-onelevel "$1" >/dev/null 2>&1) + else + (! git check-ref-format --branch "$1" >/dev/null 2>&1) + fi + nix --debug eval --raw "(builtins.fetchGit { url = $repo; ref = ''$1''; }).outPath" 2>&1 | grep 'error: invalid Git branch/tag name' >/dev/null +} + + +valid_ref 'foox' +valid_ref '1337' +valid_ref 'foo.baz' +valid_ref 'foo/bar/baz' +valid_ref 'foo./bar' +valid_ref 'heads/foo@bar' +valid_ref "$(printf 'heads/fu\303\237')" +valid_ref 'foo-bar-baz' +valid_ref '$1' +valid_ref 'foo.locke' + +invalid_ref 'refs///heads/foo' +invalid_ref 'heads/foo/' +invalid_ref '///heads/foo' +invalid_ref '.foo' +invalid_ref './foo' +invalid_ref './foo/bar' +invalid_ref 'foo/./bar' +invalid_ref 'foo/bar/.' +invalid_ref 'foo bar' +invalid_ref 'foo?bar' +invalid_ref 'foo^bar' +invalid_ref 'foo~bar' +invalid_ref 'foo:bar' +invalid_ref 'foo[bar' +invalid_ref 'foo/bar/.' +invalid_ref '.refs/foo' +invalid_ref 'refs/heads/foo.' +invalid_ref 'heads/foo..bar' +invalid_ref 'heads/foo?bar' +invalid_ref 'heads/foo.lock' +invalid_ref 'heads///foo.lock' +invalid_ref 'foo.lock/bar' +invalid_ref 'foo.lock///bar' +invalid_ref 'heads/v@{ation' +invalid_ref 'heads/foo\.ar' # should fail due to \ +invalid_ref 'heads/foo\bar' # should fail due to \ +invalid_ref "$(printf 'heads/foo\t')" # should fail because it has a TAB +invalid_ref "$(printf 'heads/foo\177')" +invalid_ref '@' + +invalid_ref 'foo/*' +invalid_ref '*/foo' +invalid_ref 'foo/*/bar' +invalid_ref '*' +invalid_ref 'foo/*/*' +invalid_ref '*/foo/*' +invalid_ref '/foo' +invalid_ref '' diff --git a/tests/local.mk b/tests/local.mk index 56e5640ca..536661af8 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -18,6 +18,7 @@ nix_tests = \ nar-access.sh \ structured-attrs.sh \ fetchGit.sh \ + fetchGitRefs.sh \ fetchGitSubmodules.sh \ fetchMercurial.sh \ signing.sh \ From fb38459d6e58508245553380cccc03c0dbaa1542 Mon Sep 17 00:00:00 2001 From: Nikola Knezevic Date: Sat, 30 May 2020 12:33:38 +0200 Subject: [PATCH 47/58] Ensure we restrict refspec interpretation while fetching As `git fetch` may chose to interpret refspec to it's liking, ensure that we only pass refs that begin with `refs/` as is, otherwise, prepend them with `refs/heads`. Otherwise, branches named `heads/foo` (I know it's bad, but it's allowed), would be fetched as `foo`, instead of `heads/foo`. --- src/libfetchers/git.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index e069057eb..75ce5ee8b 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -282,7 +282,10 @@ struct GitInput : Input // FIXME: git stderr messes up our progress indicator, so // we're using --quiet for now. Should process its stderr. try { - runProgram("git", true, { "-C", repoDir, "fetch", "--quiet", "--force", "--", actualUrl, fmt("%s:%s", *input->ref, *input->ref) }); + auto fetchRef = input->ref->compare(0, 5, "refs/") == 0 + ? *input->ref + : "refs/heads/" + *input->ref; + runProgram("git", true, { "-C", repoDir, "fetch", "--quiet", "--force", "--", actualUrl, fmt("%s:%s", fetchRef, fetchRef) }); } catch (Error & e) { if (!pathExists(localRefFile)) throw; warn("could not update local clone of Git repository '%s'; continuing with the most recent version", actualUrl); From eca1ff7a9f784926c52533c81cb5403a2cb804d3 Mon Sep 17 00:00:00 2001 From: Tobias Pflug Date: Sun, 31 May 2020 01:05:05 +0200 Subject: [PATCH 48/58] Add tests for lru-cache.hh --- src/libutil/tests/lru-cache.cc | 130 +++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+) create mode 100644 src/libutil/tests/lru-cache.cc diff --git a/src/libutil/tests/lru-cache.cc b/src/libutil/tests/lru-cache.cc new file mode 100644 index 000000000..598977a6d --- /dev/null +++ b/src/libutil/tests/lru-cache.cc @@ -0,0 +1,130 @@ +#include "lru-cache.hh" +#include + +namespace nix { + + /* ---------------------------------------------------------------------------- + * size + * --------------------------------------------------------------------------*/ + + TEST(LRUCache, sizeOfEmptyCacheIsZero) { + LRUCache c(10); + ASSERT_EQ(c.size(), 0); + } + + TEST(LRUCache, sizeOfSingleElementCacheIsOne) { + LRUCache c(10); + c.upsert("foo", "bar"); + ASSERT_EQ(c.size(), 1); + } + + /* ---------------------------------------------------------------------------- + * upsert / get + * --------------------------------------------------------------------------*/ + + TEST(LRUCache, getFromEmptyCache) { + LRUCache c(10); + auto val = c.get("x"); + ASSERT_EQ(val.has_value(), false); + } + + TEST(LRUCache, getExistingValue) { + LRUCache c(10); + c.upsert("foo", "bar"); + auto val = c.get("foo"); + ASSERT_EQ(val, "bar"); + } + + TEST(LRUCache, getNonExistingValueFromNonEmptyCache) { + LRUCache c(10); + c.upsert("foo", "bar"); + auto val = c.get("another"); + ASSERT_EQ(val.has_value(), false); + } + + TEST(LRUCache, upsertOnZeroCapacityCache) { + LRUCache c(0); + c.upsert("foo", "bar"); + auto val = c.get("foo"); + ASSERT_EQ(val.has_value(), false); + } + + TEST(LRUCache, updateExistingValue) { + LRUCache c(1); + c.upsert("foo", "bar"); + + auto val = c.get("foo"); + ASSERT_EQ(val.value_or("error"), "bar"); + ASSERT_EQ(c.size(), 1); + + c.upsert("foo", "changed"); + val = c.get("foo"); + ASSERT_EQ(val.value_or("error"), "changed"); + ASSERT_EQ(c.size(), 1); + } + + TEST(LRUCache, overwriteOldestWhenCapacityIsReached) { + LRUCache c(3); + c.upsert("one", "eins"); + c.upsert("two", "zwei"); + c.upsert("three", "drei"); + + ASSERT_EQ(c.size(), 3); + ASSERT_EQ(c.get("one").value_or("error"), "eins"); + + // exceed capacity + c.upsert("another", "whatever"); + + ASSERT_EQ(c.size(), 3); + // Retrieving "one" makes it the most recent element thus + // two will be the oldest one and thus replaced. + ASSERT_EQ(c.get("two").has_value(), false); + ASSERT_EQ(c.get("another").value(), "whatever"); + } + + /* ---------------------------------------------------------------------------- + * clear + * --------------------------------------------------------------------------*/ + + TEST(LRUCache, clearEmptyCache) { + LRUCache c(10); + c.clear(); + ASSERT_EQ(c.size(), 0); + } + + TEST(LRUCache, clearNonEmptyCache) { + LRUCache c(10); + c.upsert("one", "eins"); + c.upsert("two", "zwei"); + c.upsert("three", "drei"); + ASSERT_EQ(c.size(), 3); + c.clear(); + ASSERT_EQ(c.size(), 0); + } + + /* ---------------------------------------------------------------------------- + * erase + * --------------------------------------------------------------------------*/ + + TEST(LRUCache, eraseFromEmptyCache) { + LRUCache c(10); + c.erase("foo"); + ASSERT_EQ(c.size(), 0); + } + + TEST(LRUCache, eraseMissingFromNonEmptyCache) { + LRUCache c(10); + c.upsert("one", "eins"); + c.erase("foo"); + ASSERT_EQ(c.size(), 1); + ASSERT_EQ(c.get("one").value_or("error"), "eins"); + } + + TEST(LRUCache, eraseFromNonEmptyCache) { + LRUCache c(10); + c.upsert("one", "eins"); + c.erase("one"); + ASSERT_EQ(c.size(), 0); + ASSERT_EQ(c.get("one").value_or("empty"), "empty"); + } +} From e9fee8e6a7ff8540a04620e7d75a987cfe89ab00 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 2 Jun 2020 12:06:59 +0200 Subject: [PATCH 49/58] src/libutil/tests/lru-cache.cc: Check erase() Co-authored-by: James Lee --- src/libutil/tests/lru-cache.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libutil/tests/lru-cache.cc b/src/libutil/tests/lru-cache.cc index 598977a6d..091d3d5ed 100644 --- a/src/libutil/tests/lru-cache.cc +++ b/src/libutil/tests/lru-cache.cc @@ -108,14 +108,14 @@ namespace nix { TEST(LRUCache, eraseFromEmptyCache) { LRUCache c(10); - c.erase("foo"); + ASSERT_EQ(c.erase("foo"), false); ASSERT_EQ(c.size(), 0); } TEST(LRUCache, eraseMissingFromNonEmptyCache) { LRUCache c(10); c.upsert("one", "eins"); - c.erase("foo"); + ASSERT_EQ(c.erase("foo"), false); ASSERT_EQ(c.size(), 1); ASSERT_EQ(c.get("one").value_or("error"), "eins"); } @@ -123,7 +123,7 @@ namespace nix { TEST(LRUCache, eraseFromNonEmptyCache) { LRUCache c(10); c.upsert("one", "eins"); - c.erase("one"); + ASSERT_EQ(c.erase("one"), true); ASSERT_EQ(c.size(), 0); ASSERT_EQ(c.get("one").value_or("empty"), "empty"); } From d82d230b4015c50e698e3be1ec7a3e12277a3251 Mon Sep 17 00:00:00 2001 From: Ben Burdette Date: Tue, 2 Jun 2020 08:22:24 -0600 Subject: [PATCH 50/58] elide the 'ErrorInfo' in logError and logWarning calls --- src/build-remote/build-remote.cc | 11 +- src/error-demo/error-demo.cc | 111 +++++++++--------- src/libstore/build.cc | 107 ++++++++--------- src/libstore/filetransfer.cc | 11 +- src/libstore/local-store.cc | 45 ++++--- src/libstore/optimise-store.cc | 21 ++-- src/libutil/logging.cc | 7 +- src/nix-env/nix-env.cc | 27 ++--- src/nix-store/nix-store.cc | 17 ++- src/nix/verify.cc | 26 ++-- .../resolve-system-dependencies.cc | 28 ++--- 11 files changed, 187 insertions(+), 224 deletions(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index 0caa97fa4..2f0b8c825 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -200,12 +200,11 @@ static int _main(int argc, char * * argv) } catch (std::exception & e) { auto msg = chomp(drainFD(5, false)); - logError( - ErrorInfo { - .name = "Remote build", - .hint = hintfmt("cannot build on '%s': %s%s", - bestMachine->storeUri, e.what(), - (msg.empty() ? "" : ": " + msg)) + logError({ + .name = "Remote build", + .hint = hintfmt("cannot build on '%s': %s%s", + bestMachine->storeUri, e.what(), + (msg.empty() ? "" : ": " + msg)) }); bestMachine->enabled = false; continue; diff --git a/src/error-demo/error-demo.cc b/src/error-demo/error-demo.cc index 0216092e3..f1e17b3de 100644 --- a/src/error-demo/error-demo.cc +++ b/src/error-demo/error-demo.cc @@ -62,101 +62,96 @@ int main() // For completeness sake, show 'info' through 'vomit' levels. // But this is maybe a heavy format for those. - logger->logEI( - ErrorInfo { .level = lvlInfo, + logger->logEI({ .level = lvlInfo, .name = "Info name", .description = "Info description", }); - logger->logEI( - ErrorInfo { .level = lvlTalkative, + logger->logEI({ .level = lvlTalkative, .name = "Talkative name", .description = "Talkative description", }); - logger->logEI( - ErrorInfo { .level = lvlChatty, + logger->logEI({ .level = lvlChatty, .name = "Chatty name", .description = "Chatty description", }); - logger->logEI( - ErrorInfo { .level = lvlDebug, + logger->logEI({ .level = lvlDebug, .name = "Debug name", .description = "Debug description", }); - logger->logEI( - ErrorInfo { .level = lvlVomit, + logger->logEI({ .level = lvlVomit, .name = "Vomit name", .description = "Vomit description", }); // Error in a program; no hint and no nix code. - logError( - ErrorInfo { .name = "name", - .description = "error description", - }); + logError({ + .name = "name", + .description = "error description", + }); // Warning with name, description, and hint. // The hintfmt function makes all the substituted text yellow. - logWarning( - ErrorInfo { .name = "name", - .description = "error description", - .hint = hintfmt("there was a %1%", "warning"), - }); + logWarning({ + .name = "name", + .description = "error description", + .hint = hintfmt("there was a %1%", "warning"), + }); // Warning with nix file, line number, column, and the lines of // code where a warning occurred. SymbolTable testTable; auto problem_file = testTable.create("myfile.nix"); - logWarning( - ErrorInfo { .name = "warning name", - .description = "warning description", - .hint = hintfmt("this hint has %1% templated %2%!!", - "yellow", - "values"), - .nixCode = NixCode { - .errPos = Pos(problem_file, 40, 13), - .prevLineOfCode = std::nullopt, - .errLineOfCode = "this is the problem line of code", - .nextLineOfCode = std::nullopt - }}); + logWarning({ + .name = "warning name", + .description = "warning description", + .hint = hintfmt("this hint has %1% templated %2%!!", + "yellow", + "values"), + .nixCode = NixCode { + .errPos = Pos(problem_file, 40, 13), + .prevLineOfCode = std::nullopt, + .errLineOfCode = "this is the problem line of code", + .nextLineOfCode = std::nullopt + }}); // Error with previous and next lines of code. - logError( - ErrorInfo { .name = "error name", - .description = "error with code lines", - .hint = hintfmt("this hint has %1% templated %2%!!", - "yellow", - "values"), - .nixCode = NixCode { - .errPos = Pos(problem_file, 40, 13), - .prevLineOfCode = "previous line of code", - .errLineOfCode = "this is the problem line of code", - .nextLineOfCode = "next line of code", - }}); + logError({ + .name = "error name", + .description = "error with code lines", + .hint = hintfmt("this hint has %1% templated %2%!!", + "yellow", + "values"), + .nixCode = NixCode { + .errPos = Pos(problem_file, 40, 13), + .prevLineOfCode = "previous line of code", + .errLineOfCode = "this is the problem line of code", + .nextLineOfCode = "next line of code", + }}); // Error without lines of code. - logError( - ErrorInfo { .name = "error name", - .description = "error without any code lines.", - .hint = hintfmt("this hint has %1% templated %2%!!", - "yellow", - "values"), - .nixCode = NixCode { - .errPos = Pos(problem_file, 40, 13) - }}); + logError({ + .name = "error name", + .description = "error without any code lines.", + .hint = hintfmt("this hint has %1% templated %2%!!", + "yellow", + "values"), + .nixCode = NixCode { + .errPos = Pos(problem_file, 40, 13) + }}); // Error with only hint and name.. - logError( - ErrorInfo { .name = "error name", - .hint = hintfmt("hint %1%", "only"), - .nixCode = NixCode { - .errPos = Pos(problem_file, 40, 13) - }}); + logError({ + .name = "error name", + .hint = hintfmt("hint %1%", "only"), + .nixCode = NixCode { + .errPos = Pos(problem_file, 40, 13) + }}); return 0; } diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 4630432c9..04e8d2ebe 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1158,10 +1158,9 @@ void DerivationGoal::loadDerivation() trace("loading derivation"); if (nrFailed != 0) { - logError( - ErrorInfo { - .name = "missing derivation during build", - .hint = hintfmt("cannot build missing derivation '%s'", worker.store.printStorePath(drvPath)) + logError({ + .name = "missing derivation during build", + .hint = hintfmt("cannot build missing derivation '%s'", worker.store.printStorePath(drvPath)) }); done(BuildResult::MiscFailure); return; @@ -1313,12 +1312,11 @@ void DerivationGoal::repairClosure() /* Check each path (slow!). */ for (auto & i : outputClosure) { if (worker.pathContentsGood(i)) continue; - logError( - ErrorInfo { - .name = "Corrupt path in closure", - .hint = hintfmt( - "found corrupted or missing path '%s' in the output closure of '%s'", - worker.store.printStorePath(i), worker.store.printStorePath(drvPath)) + logError({ + .name = "Corrupt path in closure", + .hint = hintfmt( + "found corrupted or missing path '%s' in the output closure of '%s'", + worker.store.printStorePath(i), worker.store.printStorePath(drvPath)) }); auto drvPath2 = outputsToDrv.find(i); if (drvPath2 == outputsToDrv.end()) @@ -1353,12 +1351,11 @@ void DerivationGoal::inputsRealised() if (nrFailed != 0) { if (!useDerivation) throw Error("some dependencies of '%s' are missing", worker.store.printStorePath(drvPath)); - logError( - ErrorInfo { - .name = "Dependencies could not be built", - .hint = hintfmt( - "cannot build derivation '%s': %s dependencies couldn't be built", - worker.store.printStorePath(drvPath), nrFailed) + logError({ + .name = "Dependencies could not be built", + .hint = hintfmt( + "cannot build derivation '%s': %s dependencies couldn't be built", + worker.store.printStorePath(drvPath), nrFailed) }); done(BuildResult::DependencyFailed); return; @@ -1844,12 +1841,11 @@ HookReply DerivationGoal::tryBuildHook() } catch (SysError & e) { if (e.errNo == EPIPE) { - logError( - ErrorInfo { - .name = "Build hook died", - .hint = hintfmt( - "build hook died unexpectedly: %s", - chomp(drainFD(worker.hook->fromHook.readSide.get()))) + logError({ + .name = "Build hook died", + .hint = hintfmt( + "build hook died unexpectedly: %s", + chomp(drainFD(worker.hook->fromHook.readSide.get()))) }); worker.hook = 0; return rpDecline; @@ -3692,11 +3688,10 @@ void DerivationGoal::registerOutputs() /* Apply hash rewriting if necessary. */ bool rewritten = false; if (!outputRewrites.empty()) { - logWarning( - ErrorInfo { - .name = "Rewriting hashes", - .hint = hintfmt("rewriting hashes in '%1%'; cross fingers", path) - }); + logWarning({ + .name = "Rewriting hashes", + .hint = hintfmt("rewriting hashes in '%1%'; cross fingers", path) + }); /* Canonicalise first. This ensures that the path we're rewriting doesn't contain a hard link to /etc/shadow or @@ -3875,10 +3870,9 @@ void DerivationGoal::registerOutputs() if (settings.enforceDeterminism) throw NotDeterministic(hint); - logError( - ErrorInfo { - .name = "Output determinism error", - .hint = hint + logError({ + .name = "Output determinism error", + .hint = hint }); @@ -4145,12 +4139,11 @@ void DerivationGoal::handleChildOutput(int fd, const string & data) { logSize += data.size(); if (settings.maxLogSize && logSize > settings.maxLogSize) { - logError( - ErrorInfo { - .name = "Max log size exceeded", - .hint = hintfmt( - "%1% killed after writing more than %2% bytes of log output", - getName(), settings.maxLogSize) + logError({ + .name = "Max log size exceeded", + .hint = hintfmt( + "%1% killed after writing more than %2% bytes of log output", + getName(), settings.maxLogSize) }); killChild(); done(BuildResult::LogLimitExceeded); @@ -4467,12 +4460,11 @@ void SubstitutionGoal::tryNext() && !sub->isTrusted && !info->checkSignatures(worker.store, worker.store.getPublicKeys())) { - logWarning( - ErrorInfo { - .name = "Invalid path signature", - .hint = hintfmt("substituter '%s' does not have a valid signature for path '%s'", - sub->getUri(), worker.store.printStorePath(storePath)) - }); + logWarning({ + .name = "Invalid path signature", + .hint = hintfmt("substituter '%s' does not have a valid signature for path '%s'", + sub->getUri(), worker.store.printStorePath(storePath)) + }); tryNext(); return; } @@ -4954,12 +4946,11 @@ void Worker::waitForInput() j->respectTimeouts && after - j->lastOutput >= std::chrono::seconds(settings.maxSilentTime)) { - logError( - ErrorInfo { - .name = "Silent build timeout", - .hint = hintfmt( - "%1% timed out after %2% seconds of silence", - goal->getName(), settings.maxSilentTime) + logError({ + .name = "Silent build timeout", + .hint = hintfmt( + "%1% timed out after %2% seconds of silence", + goal->getName(), settings.maxSilentTime) }); goal->timedOut(); } @@ -4969,12 +4960,11 @@ void Worker::waitForInput() j->respectTimeouts && after - j->timeStarted >= std::chrono::seconds(settings.buildTimeout)) { - logError( - ErrorInfo { - .name = "Build timeout", - .hint = hintfmt( - "%1% timed out after %2% seconds", - goal->getName(), settings.buildTimeout) + logError({ + .name = "Build timeout", + .hint = hintfmt( + "%1% timed out after %2% seconds", + goal->getName(), settings.buildTimeout) }); goal->timedOut(); } @@ -5035,10 +5025,9 @@ bool Worker::pathContentsGood(const StorePath & path) } pathContentsGoodCache.insert_or_assign(path.clone(), res); if (!res) - logError( - ErrorInfo { - .name = "Corrupted path", - .hint = hintfmt("path '%s' is corrupted or missing!", store.printStorePath(path)) + logError({ + .name = "Corrupted path", + .hint = hintfmt("path '%s' is corrupted or missing!", store.printStorePath(path)) }); return res; } diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 89588d662..6ca3393ab 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -599,12 +599,11 @@ struct curlFileTransfer : public FileTransfer workerThreadMain(); } catch (nix::Interrupted & e) { } catch (std::exception & e) { - logError( - ErrorInfo { - .name = "File transfer", - .hint = hintfmt("unexpected error in download thread: %s", - e.what()) - }); + logError({ + .name = "File transfer", + .hint = hintfmt("unexpected error in download thread: %s", + e.what()) + }); } { diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index abcfe5271..53fb4ce68 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -87,12 +87,11 @@ LocalStore::LocalStore(const Params & params) struct group * gr = getgrnam(settings.buildUsersGroup.get().c_str()); if (!gr) - logError( - ErrorInfo { - .name = "'build-users-group' not found", - .hint = hintfmt( - "warning: the group '%1%' specified in 'build-users-group' does not exist", - settings.buildUsersGroup) + logError({ + .name = "'build-users-group' not found", + .hint = hintfmt( + "warning: the group '%1%' specified in 'build-users-group' does not exist", + settings.buildUsersGroup) }); else { struct stat st; @@ -1242,12 +1241,11 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) Path linkPath = linksDir + "/" + link.name; string hash = hashPath(htSHA256, linkPath).first.to_string(Base32, false); if (hash != link.name) { - logError( - ErrorInfo { - .name = "Invalid hash", - .hint = hintfmt( - "link '%s' was modified! expected hash '%s', got '%s'", - linkPath, link.name, hash) + logError({ + .name = "Invalid hash", + .hint = hintfmt( + "link '%s' was modified! expected hash '%s', got '%s'", + linkPath, link.name, hash) }); if (repair) { if (unlink(linkPath.c_str()) == 0) @@ -1281,11 +1279,10 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair) auto current = hashSink->finish(); if (info->narHash != nullHash && info->narHash != current.first) { - logError( - ErrorInfo { - .name = "Invalid hash - path modified", - .hint = hintfmt("path '%s' was modified! expected hash '%s', got '%s'", - printStorePath(i), info->narHash.to_string(), current.first.to_string()) + logError({ + .name = "Invalid hash - path modified", + .hint = hintfmt("path '%s' was modified! expected hash '%s', got '%s'", + printStorePath(i), info->narHash.to_string(), current.first.to_string()) }); if (repair) repairPath(i); else errors = true; } else { @@ -1337,10 +1334,9 @@ void LocalStore::verifyPath(const Path & pathS, const StringSet & store, if (!done.insert(pathS).second) return; if (!isStorePath(pathS)) { - logError( - ErrorInfo { - .name = "Nix path not found", - .hint = hintfmt("path '%s' is not in the Nix store", pathS) + logError({ + .name = "Nix path not found", + .hint = hintfmt("path '%s' is not in the Nix store", pathS) }); return; } @@ -1364,10 +1360,9 @@ void LocalStore::verifyPath(const Path & pathS, const StringSet & store, auto state(_state.lock()); invalidatePath(*state, path); } else { - logError( - ErrorInfo { - .name = "Missing path with referrers", - .hint = hintfmt("path '%s' disappeared, but it still has valid referrers!", pathS) + logError({ + .name = "Missing path with referrers", + .hint = hintfmt("path '%s' disappeared, but it still has valid referrers!", pathS) }); if (repair) try { diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index c9ecca5a8..cc0507be2 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -130,10 +130,9 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, NixOS (example: $fontconfig/var/cache being modified). Skip those files. FIXME: check the modification time. */ if (S_ISREG(st.st_mode) && (st.st_mode & S_IWUSR)) { - logWarning( - ErrorInfo { - .name = "Suspicious file", - .hint = hintfmt("skipping suspicious writable file '%1%'", path) + logWarning({ + .name = "Suspicious file", + .hint = hintfmt("skipping suspicious writable file '%1%'", path) }); return; } @@ -198,10 +197,9 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, } if (st.st_size != stLink.st_size) { - logWarning( - ErrorInfo { - .name = "Corrupted link", - .hint = hintfmt("removing corrupted link '%1%'", linkPath) + logWarning({ + .name = "Corrupted link", + .hint = hintfmt("removing corrupted link '%1%'", linkPath) }); unlink(linkPath.c_str()); goto retry; @@ -237,10 +235,9 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, /* Atomically replace the old file with the new hard link. */ if (rename(tempLink.c_str(), path.c_str()) == -1) { if (unlink(tempLink.c_str()) == -1) - logError( - ErrorInfo { - .name = "Unlink error", - .hint = hintfmt("unable to unlink '%1%'", tempLink) + logError({ + .name = "Unlink error", + .hint = hintfmt("unable to unlink '%1%'", tempLink) }); if (errno == EMLINK) { /* Some filesystems generate too many links on the rename, diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index 071a7ec7e..41378b0db 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -251,10 +251,9 @@ bool handleJSONLogMessage(const std::string & msg, } } catch (std::exception & e) { - logError( - ErrorInfo { - .name = "Json log message", - .hint = hintfmt("bad log message from builder: %s", e.what()) + logError({ + .name = "Json log message", + .hint = hintfmt("bad log message from builder: %s", e.what()) }); } diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index bbce5dcda..8231a07a4 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -123,10 +123,9 @@ static void getAllExprs(EvalState & state, if (hasSuffix(attrName, ".nix")) attrName = string(attrName, 0, attrName.size() - 4); if (!attrs.insert(attrName).second) { - logError( - ErrorInfo { - .name = "Name collision", - .hint = hintfmt("warning: name collision in input Nix expressions, skipping '%1%'", path2) + logError({ + .name = "Name collision", + .hint = hintfmt("warning: name collision in input Nix expressions, skipping '%1%'", path2) }); continue; } @@ -875,11 +874,10 @@ static void queryJSON(Globals & globals, vector & elems) auto placeholder = metaObj.placeholder(j); Value * v = i.queryMeta(j); if (!v) { - logError( - ErrorInfo { - .name = "Invalid meta attribute", - .hint = hintfmt("derivation '%s' has invalid meta attribute '%s'", - i.queryName(), j) + logError({ + .name = "Invalid meta attribute", + .hint = hintfmt("derivation '%s' has invalid meta attribute '%s'", + i.queryName(), j) }); placeholder.write(nullptr); } else { @@ -1131,12 +1129,11 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs) attrs2["name"] = j; Value * v = i.queryMeta(j); if (!v) - logError( - ErrorInfo { - .name = "Invalid meta attribute", - .hint = hintfmt( - "derivation '%s' has invalid meta attribute '%s'", - i.queryName(), j) + logError({ + .name = "Invalid meta attribute", + .hint = hintfmt( + "derivation '%s' has invalid meta attribute '%s'", + i.queryName(), j) }); else { if (v->type == tString) { diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index 6b83093b1..5e42736fc 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -704,7 +704,7 @@ static void opVerify(Strings opFlags, Strings opArgs) else throw UsageError("unknown flag '%1%'", i); if (store->verifyStore(checkContents, repair)) { - logWarning(ErrorInfo { + logWarning({ .name = "Store consistency", .description = "not all errors were fixed" }); @@ -729,14 +729,13 @@ static void opVerifyPath(Strings opFlags, Strings opArgs) store->narFromPath(path, sink); auto current = sink.finish(); if (current.first != info->narHash) { - logError( - ErrorInfo { - .name = "Hash mismatch", - .hint = hintfmt( - "path '%s' was modified! expected hash '%s', got '%s'", - store->printStorePath(path), - info->narHash.to_string(), - current.first.to_string()) + logError({ + .name = "Hash mismatch", + .hint = hintfmt( + "path '%s' was modified! expected hash '%s', got '%s'", + store->printStorePath(path), + info->narHash.to_string(), + current.first.to_string()) }); status = 1; } diff --git a/src/nix/verify.cc b/src/nix/verify.cc index 8c845bfc2..8ecd9a8f3 100644 --- a/src/nix/verify.cc +++ b/src/nix/verify.cc @@ -99,15 +99,14 @@ struct CmdVerify : StorePathsCommand if (hash.first != info->narHash) { corrupted++; act2.result(resCorruptedPath, store->printStorePath(info->path)); - logError( - ErrorInfo { - .name = "Hash error - path modified", - .hint = hintfmt( - "path '%s' was modified! expected hash '%s', got '%s'", - store->printStorePath(info->path), - info->narHash.to_string(), - hash.first.to_string()) - }); + logError({ + .name = "Hash error - path modified", + .hint = hintfmt( + "path '%s' was modified! expected hash '%s', got '%s'", + store->printStorePath(info->path), + info->narHash.to_string(), + hash.first.to_string()) + }); } } @@ -156,11 +155,10 @@ struct CmdVerify : StorePathsCommand if (!good) { untrusted++; act2.result(resUntrustedPath, store->printStorePath(info->path)); - logError( - ErrorInfo { - .name = "Untrusted path", - .hint = hintfmt("path '%s' is untrusted", - store->printStorePath(info->path)) + logError({ + .name = "Untrusted path", + .hint = hintfmt("path '%s' is untrusted", + store->printStorePath(info->path)) }); } diff --git a/src/resolve-system-dependencies/resolve-system-dependencies.cc b/src/resolve-system-dependencies/resolve-system-dependencies.cc index dcea72529..82feacb3d 100644 --- a/src/resolve-system-dependencies/resolve-system-dependencies.cc +++ b/src/resolve-system-dependencies/resolve-system-dependencies.cc @@ -39,19 +39,17 @@ std::set runResolver(const Path & filename) throw SysError("statting '%s'", filename); if (!S_ISREG(st.st_mode)) { - logError( - ErrorInfo { - .name = "Regular MACH file", - .hint = hintfmt("file '%s' is not a regular file", filename) + logError({ + .name = "Regular MACH file", + .hint = hintfmt("file '%s' is not a regular file", filename) }); return {}; } if (st.st_size < sizeof(mach_header_64)) { - logError( - ErrorInfo { - .name = "File too short", - .hint = hintfmt("file '%s' is too short for a MACH binary", filename) + logError({ + .name = "File too short", + .hint = hintfmt("file '%s' is too short for a MACH binary", filename) }); return {}; } @@ -74,20 +72,18 @@ std::set runResolver(const Path & filename) } } if (mach64_offset == 0) { - logError( - ErrorInfo { - .name = "No mach64 blobs", - .hint = hintfmt("Could not find any mach64 blobs in file '%1%', continuing...", filename) + logError({ + .name = "No mach64 blobs", + .hint = hintfmt("Could not find any mach64 blobs in file '%1%', continuing...", filename) }); return {}; } } else if (magic == MH_MAGIC_64 || magic == MH_CIGAM_64) { mach64_offset = 0; } else { - logError( - ErrorInfo { - .name = "Magic number", - .hint = hintfmt("Object file has unknown magic number '%1%', skipping it...", magic) + logError({ + .name = "Magic number", + .hint = hintfmt("Object file has unknown magic number '%1%', skipping it...", magic) }); return {}; } From 156d4f8bc892baa4e25814d5f79a332994c750d1 Mon Sep 17 00:00:00 2001 From: Ben Burdette Date: Tue, 2 Jun 2020 08:45:37 -0600 Subject: [PATCH 51/58] remove extra space in SysErrors --- src/libutil/error.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/error.hh b/src/libutil/error.hh index b374c2780..0ca0c8b15 100644 --- a/src/libutil/error.hh +++ b/src/libutil/error.hh @@ -155,7 +155,7 @@ public: { errNo = errno; auto hf = hintfmt(args...); - err.hint = hintfmt("%1% : %2%", normaltxt(hf.str()), strerror(errNo)); + err.hint = hintfmt("%1%: %2%", normaltxt(hf.str()), strerror(errNo)); } virtual const char* sname() const override { return "SysError"; } From 01572c2198de49071827f0be9f5db202bac21703 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 3 Jun 2020 06:15:22 -0400 Subject: [PATCH 52/58] Missing `#include ` in `lru-cache.hh` (#3654) This was a latent bug that just appeared because of the tests that were added. Remember to wait for CI! :) --- src/libutil/lru-cache.hh | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libutil/lru-cache.hh b/src/libutil/lru-cache.hh index 8b83f842c..6ef4a3e06 100644 --- a/src/libutil/lru-cache.hh +++ b/src/libutil/lru-cache.hh @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include From f97576c5d91d5278155a67e2b8982d2233d4335a Mon Sep 17 00:00:00 2001 From: Ben Burdette Date: Wed, 3 Jun 2020 14:47:00 -0600 Subject: [PATCH 53/58] newline-as-prefix; no final newline in output. --- src/libutil/error.cc | 73 +++++++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 34 deletions(-) diff --git a/src/libutil/error.cc b/src/libutil/error.cc index 59628d875..539a7543f 100644 --- a/src/libutil/error.cc +++ b/src/libutil/error.cc @@ -57,20 +57,20 @@ void printCodeLines(std::ostream &out, const string &prefix, const NixCode &nixC { // previous line of code. if (nixCode.prevLineOfCode.has_value()) { - out << fmt("%1% %|2$5d|| %3%", - prefix, - (nixCode.errPos.line - 1), - *nixCode.prevLineOfCode) - << std::endl; + out << std::endl + << fmt("%1% %|2$5d|| %3%", + prefix, + (nixCode.errPos.line - 1), + *nixCode.prevLineOfCode); } if (nixCode.errLineOfCode.has_value()) { // line of code containing the error. - out << fmt("%1% %|2$5d|| %3%", - prefix, - (nixCode.errPos.line), - *nixCode.errLineOfCode) - << std::endl; + out << std::endl + << fmt("%1% %|2$5d|| %3%", + prefix, + (nixCode.errPos.line), + *nixCode.errLineOfCode); // error arrows for the column range. if (nixCode.errPos.column > 0) { int start = nixCode.errPos.column; @@ -81,20 +81,21 @@ void printCodeLines(std::ostream &out, const string &prefix, const NixCode &nixC std::string arrows("^"); - out << fmt("%1% |%2%" ANSI_RED "%3%" ANSI_NORMAL, - prefix, - spaces, - arrows) << std::endl; + out << std::endl + << fmt("%1% |%2%" ANSI_RED "%3%" ANSI_NORMAL, + prefix, + spaces, + arrows); } } // next line of code. if (nixCode.nextLineOfCode.has_value()) { - out << fmt("%1% %|2$5d|| %3%", - prefix, - (nixCode.errPos.line + 1), - *nixCode.nextLineOfCode) - << std::endl; + out << std::endl + << fmt("%1% %|2$5d|| %3%", + prefix, + (nixCode.errPos.line + 1), + *nixCode.nextLineOfCode); } } @@ -167,46 +168,50 @@ std::ostream& operator<<(std::ostream &out, const ErrorInfo &einfo) levelString, einfo.name, dashes, - einfo.programName.value_or("")) - << std::endl; + einfo.programName.value_or("")); else out << fmt("%1%%2%" ANSI_BLUE " -----%3% %4%" ANSI_NORMAL, prefix, levelString, dashes, - einfo.programName.value_or("")) - << std::endl; + einfo.programName.value_or("")); - // filename, line, column. + bool nl = false; // intersperse newline between sections. if (einfo.nixCode.has_value()) { if (einfo.nixCode->errPos.file != "") { - out << fmt("%1%in file: " ANSI_BLUE "%2% %3%" ANSI_NORMAL, + // filename, line, column. + out << std::endl << fmt("%1%in file: " ANSI_BLUE "%2% %3%" ANSI_NORMAL, prefix, einfo.nixCode->errPos.file, - showErrPos(einfo.nixCode->errPos)) << std::endl; - out << prefix << std::endl; + showErrPos(einfo.nixCode->errPos)); } else { - out << fmt("%1%from command line argument", prefix) << std::endl; - out << prefix << std::endl; + out << std::endl << fmt("%1%from command line argument", prefix); } + nl = true; } // description if (einfo.description != "") { - out << prefix << einfo.description << std::endl; - out << prefix << std::endl; + if (nl) + out << std::endl << prefix; + out << std::endl << prefix << einfo.description; + nl = true; } // lines of code. if (einfo.nixCode.has_value() && einfo.nixCode->errLineOfCode.has_value()) { + if (nl) + out << std::endl << prefix; printCodeLines(out, prefix, *einfo.nixCode); - out << prefix << std::endl; + nl = true; } // hint if (einfo.hint.has_value()) { - out << prefix << *einfo.hint << std::endl; - out << prefix << std::endl; + if (nl) + out << std::endl << prefix; + out << std::endl << prefix << *einfo.hint; + nl = true; } return out; From 721943e1d44b63d1a4055309fad39e1f4c0f9f0d Mon Sep 17 00:00:00 2001 From: Ben Burdette Date: Wed, 3 Jun 2020 17:32:57 -0600 Subject: [PATCH 54/58] update error grep --- tests/fetchGitRefs.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/fetchGitRefs.sh b/tests/fetchGitRefs.sh index 93993ae90..23934698e 100644 --- a/tests/fetchGitRefs.sh +++ b/tests/fetchGitRefs.sh @@ -56,7 +56,7 @@ invalid_ref() { else (! git check-ref-format --branch "$1" >/dev/null 2>&1) fi - nix --debug eval --raw "(builtins.fetchGit { url = $repo; ref = ''$1''; }).outPath" 2>&1 | grep 'error: invalid Git branch/tag name' >/dev/null + nix --debug eval --raw "(builtins.fetchGit { url = $repo; ref = ''$1''; }).outPath" 2>&1 | grep 'invalid Git branch/tag name' >/dev/null } From 94427ffee3493077206f960e3cda9bbb659583be Mon Sep 17 00:00:00 2001 From: Ben Burdette Date: Thu, 4 Jun 2020 11:53:19 -0600 Subject: [PATCH 55/58] add some comments --- src/libutil/error.cc | 6 +++++- src/libutil/error.hh | 21 +++++++++++++++++++-- src/libutil/logging.hh | 33 ++++++++++++++++++--------------- 3 files changed, 42 insertions(+), 18 deletions(-) diff --git a/src/libutil/error.cc b/src/libutil/error.cc index 539a7543f..1fcb8111c 100644 --- a/src/libutil/error.cc +++ b/src/libutil/error.cc @@ -10,13 +10,16 @@ namespace nix { const std::string nativeSystem = SYSTEM; - +// addPrefix is used for show-trace. Strings added with addPrefix +// will print ahead of the error itself. BaseError & BaseError::addPrefix(const FormatOrString & fs) { prefix_ = fs.s + prefix_; return *this; } +// c++ std::exception descendants must have a 'const char* what()' function. +// This stringifies the error and caches it for use by what(), or similarly by msg(). const string& BaseError::calcWhat() const { if (what_.has_value()) @@ -53,6 +56,7 @@ string showErrPos(const ErrPos &errPos) } } +// if nixCode contains lines of code, print them to the ostream, indicating the error column. void printCodeLines(std::ostream &out, const string &prefix, const NixCode &nixCode) { // previous line of code. diff --git a/src/libutil/error.hh b/src/libutil/error.hh index 0ca0c8b15..8a48fa105 100644 --- a/src/libutil/error.hh +++ b/src/libutil/error.hh @@ -22,6 +22,23 @@ namespace nix { +/* + +This file defines two main structs/classes used in nix error handling. + +ErrorInfo provides a standard payload of error information, with conversion to string +happening in the logger rather than at the call site. + +BaseError is the ancestor of nix specific exceptions (and Interrupted), and contains +an ErrorInfo. + +ErrorInfo structs are sent to the logger as part of an exception, or directly with the +logError or logWarning macros. + +See the error-demo.cc program for usage examples. + +*/ + typedef enum { lvlError = 0, lvlWarn, @@ -32,6 +49,7 @@ typedef enum { lvlVomit } Verbosity; +// ErrPos indicates the location of an error in a nix file. struct ErrPos { int line = 0; int column = 0; @@ -42,6 +60,7 @@ struct ErrPos { return line != 0; } + // convert from the Pos struct, found in libexpr. template ErrPos& operator=(const P &pos) { @@ -65,8 +84,6 @@ struct NixCode { std::optional nextLineOfCode; }; -// ------------------------------------------------- -// ErrorInfo. struct ErrorInfo { Verbosity level; string name; diff --git a/src/libutil/logging.hh b/src/libutil/logging.hh index 39692a291..eeb7233e9 100644 --- a/src/libutil/logging.hh +++ b/src/libutil/logging.hh @@ -150,9 +150,23 @@ bool handleJSONLogMessage(const std::string & msg, extern Verbosity verbosity; /* suppress msgs > this */ -/* Print a message if the current log level is at least the specified - level. Note that this has to be implemented as a macro to ensure - that the arguments are evaluated lazily. */ +/* Print a message with the standard ErrorInfo format. + In general, use these 'log' macros for reporting problems that may require user + intervention or that need more explanation. Use the 'print' macros for more + lightweight status messages. */ +#define logErrorInfo(level, errorInfo...) \ + do { \ + if (level <= nix::verbosity) { \ + logger->logEI(level, errorInfo); \ + } \ + } while (0) + +#define logError(errorInfo...) logErrorInfo(lvlError, errorInfo) +#define logWarning(errorInfo...) logErrorInfo(lvlWarn, errorInfo) + +/* Print a string message if the current log level is at least the specified + level. Note that this has to be implemented as a macro to ensure that the + arguments are evaluated lazily. */ #define printMsg(level, args...) \ do { \ if (level <= nix::verbosity) { \ @@ -166,18 +180,7 @@ extern Verbosity verbosity; /* suppress msgs > this */ #define debug(args...) printMsg(lvlDebug, args) #define vomit(args...) printMsg(lvlVomit, args) -#define logErrorInfo(level, errorInfo...) \ - do { \ - if (level <= nix::verbosity) { \ - logger->logEI(level, errorInfo); \ - } \ - } while (0) - -#define logError(errorInfo...) logErrorInfo(lvlError, errorInfo) -#define logWarning(errorInfo...) logErrorInfo(lvlWarn, errorInfo) - - - +/* if verbosity >= lvlWarn, print a message with a yellow 'warning:' prefix. */ template inline void warn(const std::string & fs, const Args & ... args) { From 952e72c804ce79ffeee8f58261d4cbdeb9d56ac6 Mon Sep 17 00:00:00 2001 From: Tobias Pflug Date: Sat, 6 Jun 2020 10:22:32 +0200 Subject: [PATCH 56/58] Add tests for logging.hh --- src/libutil/tests/local.mk | 2 +- src/libutil/tests/logging.cc | 251 +++++++++++++++++++++++++++++++++++ 2 files changed, 252 insertions(+), 1 deletion(-) create mode 100644 src/libutil/tests/logging.cc diff --git a/src/libutil/tests/local.mk b/src/libutil/tests/local.mk index a297edb64..815e18560 100644 --- a/src/libutil/tests/local.mk +++ b/src/libutil/tests/local.mk @@ -8,7 +8,7 @@ libutil-tests_INSTALL_DIR := libutil-tests_SOURCES := $(wildcard $(d)/*.cc) -libutil-tests_CXXFLAGS += -I src/libutil +libutil-tests_CXXFLAGS += -I src/libutil -I src/libexpr libutil-tests_LIBS = libutil diff --git a/src/libutil/tests/logging.cc b/src/libutil/tests/logging.cc new file mode 100644 index 000000000..ac015a16d --- /dev/null +++ b/src/libutil/tests/logging.cc @@ -0,0 +1,251 @@ +#include "logging.hh" +#include "nixexpr.hh" +#include "util.hh" + +#include + +namespace nix { + + /* ---------------------------------------------------------------------------- + * logEI + * --------------------------------------------------------------------------*/ + + TEST(logEI, catpuresBasicProperties) { + + MakeError(TestError, Error); + ErrorInfo::programName = std::optional("error-unit-test"); + + try { + throw TestError("an error for testing purposes"); + } catch (Error &e) { + testing::internal::CaptureStderr(); + logger->logEI(e.info()); + auto str = testing::internal::GetCapturedStderr(); + + ASSERT_STREQ(str.c_str(),"\x1B[31;1merror:\x1B[0m\x1B[34;1m --- TestError ------------------------------------ error-unit-test\x1B[0m\nan error for testing purposes\n"); + } + } + + TEST(logEI, appendingHintsToPreviousError) { + + MakeError(TestError, Error); + ErrorInfo::programName = std::optional("error-unit-test"); + + try { + auto e = Error("initial error"); + throw TestError(e.info()); + } catch (Error &e) { + ErrorInfo ei = e.info(); + ei.hint = hintfmt("%s; subsequent error message.", normaltxt(e.info().hint ? e.info().hint->str() : "")); + + testing::internal::CaptureStderr(); + logger->logEI(ei); + auto str = testing::internal::GetCapturedStderr(); + + ASSERT_STREQ(str.c_str(), "\x1B[31;1merror:\x1B[0m\x1B[34;1m --- TestError ------------------------------------ error-unit-test\x1B[0m\n\x1B[33;1m\x1B[0minitial error\x1B[0m; subsequent error message.\n"); + } + + } + + TEST(logEI, picksUpSysErrorExitCode) { + + MakeError(TestError, Error); + ErrorInfo::programName = std::optional("error-unit-test"); + + try { + auto x = readFile(-1); + } + catch (SysError &e) { + testing::internal::CaptureStderr(); + logError(e.info()); + auto str = testing::internal::GetCapturedStderr(); + + ASSERT_STREQ(str.c_str(), "\x1B[31;1merror:\x1B[0m\x1B[34;1m --- SysError ------------------------------------- error-unit-test\x1B[0m\n\x1B[33;1m\x1B[0mstatting file\x1B[0m: \x1B[33;1mBad file descriptor\x1B[0m\n"); + + } + } + + TEST(logEI, loggingErrorOnInfoLevel) { + testing::internal::CaptureStderr(); + + logger->logEI({ .level = lvlInfo, + .name = "Info name", + .description = "Info description", + }); + + auto str = testing::internal::GetCapturedStderr(); + ASSERT_STREQ(str.c_str(), "\x1B[32;1minfo:\x1B[0m\x1B[34;1m --- Info name ------------------------------------- error-unit-test\x1B[0m\nInfo description\n"); + } + + TEST(logEI, loggingErrorOnTalkativeLevel) { + testing::internal::CaptureStderr(); + + logger->logEI({ .level = lvlTalkative, + .name = "Talkative name", + .description = "Talkative description", + }); + + auto str = testing::internal::GetCapturedStderr(); + // XXX: why is this the empty string? + ASSERT_STREQ(str.c_str(), ""); + } + + TEST(logEI, loggingErrorOnChattyLevel) { + testing::internal::CaptureStderr(); + + logger->logEI({ .level = lvlChatty, + .name = "Chatty name", + .description = "Talkative description", + }); + + auto str = testing::internal::GetCapturedStderr(); + // XXX: why is this the empty string? + ASSERT_STREQ(str.c_str(), ""); + } + + TEST(logEI, loggingErrorOnDebugLevel) { + testing::internal::CaptureStderr(); + + logger->logEI({ .level = lvlDebug, + .name = "Debug name", + .description = "Debug description", + }); + + auto str = testing::internal::GetCapturedStderr(); + // XXX: why is this the empty string? + ASSERT_STREQ(str.c_str(), ""); + } + + TEST(logEI, loggingErrorOnVomitLevel) { + testing::internal::CaptureStderr(); + + logger->logEI({ .level = lvlVomit, + .name = "Vomit name", + .description = "Vomit description", + }); + + auto str = testing::internal::GetCapturedStderr(); + // XXX: why is this the empty string? + ASSERT_STREQ(str.c_str(), ""); + } + + /* ---------------------------------------------------------------------------- + * logError + * --------------------------------------------------------------------------*/ + + + TEST(logError, logErrorWithoutHintOrCode) { + testing::internal::CaptureStderr(); + + logError({ + .name = "name", + .description = "error description", + }); + + auto str = testing::internal::GetCapturedStderr(); + ASSERT_STREQ(str.c_str(), "\x1B[31;1merror:\x1B[0m\x1B[34;1m --- name ----------------------------------------- error-unit-test\x1B[0m\nerror description\n"); + } + + TEST(logError, logErrorWithPreviousAndNextLinesOfCode) { + SymbolTable testTable; + auto problem_file = testTable.create("myfile.nix"); + + testing::internal::CaptureStderr(); + + logError({ + .name = "error name", + .description = "error with code lines", + .hint = hintfmt("this hint has %1% templated %2%!!", + "yellow", + "values"), + .nixCode = NixCode { + .errPos = Pos(problem_file, 40, 13), + .prevLineOfCode = "previous line of code", + .errLineOfCode = "this is the problem line of code", + .nextLineOfCode = "next line of code", + }}); + + + auto str = testing::internal::GetCapturedStderr(); + ASSERT_STREQ(str.c_str(), "\x1B[31;1merror:\x1B[0m\x1B[34;1m --- error name ----------------------------------- error-unit-test\x1B[0m\nin file: \x1B[34;1mmyfile.nix (40:13)\x1B[0m\n\nerror with code lines\n\n 39| previous line of code\n 40| this is the problem line of code\n | \x1B[31;1m^\x1B[0m\n 41| next line of code\n\nthis hint has \x1B[33;1myellow\x1B[0m templated \x1B[33;1mvalues\x1B[0m!!\n"); + } + + TEST(logError, logErrorWithoutLinesOfCode) { + SymbolTable testTable; + auto problem_file = testTable.create("myfile.nix"); + testing::internal::CaptureStderr(); + + logError({ + .name = "error name", + .description = "error without any code lines.", + .hint = hintfmt("this hint has %1% templated %2%!!", + "yellow", + "values"), + .nixCode = NixCode { + .errPos = Pos(problem_file, 40, 13) + }}); + + auto str = testing::internal::GetCapturedStderr(); + ASSERT_STREQ(str.c_str(), "\x1B[31;1merror:\x1B[0m\x1B[34;1m --- error name ----------------------------------- error-unit-test\x1B[0m\nin file: \x1B[34;1mmyfile.nix (40:13)\x1B[0m\n\nerror without any code lines.\n\nthis hint has \x1B[33;1myellow\x1B[0m templated \x1B[33;1mvalues\x1B[0m!!\n"); + } + + TEST(logError, logErrorWithOnlyHintAndName) { + SymbolTable testTable; + auto problem_file = testTable.create("myfile.nix"); + testing::internal::CaptureStderr(); + + logError({ + .name = "error name", + .hint = hintfmt("hint %1%", "only"), + .nixCode = NixCode { + .errPos = Pos(problem_file, 40, 13) + }}); + + auto str = testing::internal::GetCapturedStderr(); + ASSERT_STREQ(str.c_str(), "\x1B[31;1merror:\x1B[0m\x1B[34;1m --- error name ----------------------------------- error-unit-test\x1B[0m\nin file: \x1B[34;1mmyfile.nix (40:13)\x1B[0m\n\nhint \x1B[33;1monly\x1B[0m\n"); + + } + + /* ---------------------------------------------------------------------------- + * logWarning + * --------------------------------------------------------------------------*/ + + TEST(logWarning, logWarningWithNameDescriptionAndHint) { + testing::internal::CaptureStderr(); + + logWarning({ + .name = "name", + .description = "error description", + .hint = hintfmt("there was a %1%", "warning"), + }); + + auto str = testing::internal::GetCapturedStderr(); + ASSERT_STREQ(str.c_str(), "\x1B[33;1mwarning:\x1B[0m\x1B[34;1m --- name --------------------------------------- error-unit-test\x1B[0m\nerror description\n\nthere was a \x1B[33;1mwarning\x1B[0m\n"); + } + + TEST(logWarning, logWarningWithFileLineNumAndCode) { + + SymbolTable testTable; + auto problem_file = testTable.create("myfile.nix"); + + testing::internal::CaptureStderr(); + + logWarning({ + .name = "warning name", + .description = "warning description", + .hint = hintfmt("this hint has %1% templated %2%!!", + "yellow", + "values"), + .nixCode = NixCode { + .errPos = Pos(problem_file, 40, 13), + .prevLineOfCode = std::nullopt, + .errLineOfCode = "this is the problem line of code", + .nextLineOfCode = std::nullopt + }}); + + + auto str = testing::internal::GetCapturedStderr(); + ASSERT_STREQ(str.c_str(), "\x1B[33;1mwarning:\x1B[0m\x1B[34;1m --- warning name ------------------------------- error-unit-test\x1B[0m\nin file: \x1B[34;1mmyfile.nix (40:13)\x1B[0m\n\nwarning description\n\n 40| this is the problem line of code\n | \x1B[31;1m^\x1B[0m\n\nthis hint has \x1B[33;1myellow\x1B[0m templated \x1B[33;1mvalues\x1B[0m!!\n"); + } + +} From e60747b5fbe8b9c646ffe07ef8d28d949302efa6 Mon Sep 17 00:00:00 2001 From: Tobias Pflug Date: Sat, 6 Jun 2020 10:23:12 +0200 Subject: [PATCH 57/58] Remove error-demo/error-demo.cc The logging.hh superseeds the demo --- src/error-demo/error-demo.cc | 157 ----------------------------------- src/error-demo/local.mk | 12 --- 2 files changed, 169 deletions(-) delete mode 100644 src/error-demo/error-demo.cc delete mode 100644 src/error-demo/local.mk diff --git a/src/error-demo/error-demo.cc b/src/error-demo/error-demo.cc deleted file mode 100644 index f1e17b3de..000000000 --- a/src/error-demo/error-demo.cc +++ /dev/null @@ -1,157 +0,0 @@ -#include "logging.hh" -#include "nixexpr.hh" -#include "util.hh" - -#include -#include - -using namespace nix; - -MakeError(DemoError, Error); - -int main() -{ - verbosity = lvlVomit; - - // In each program where errors occur, this has to be set. - ErrorInfo::programName = std::optional("error-demo"); - - // 'DemoError' appears as error name. - try { - throw DemoError("demo error was thrown"); - } catch (Error &e) { - logger->logEI(e.info()); - } - - // appending to the hint from the previous error - try { - auto e = Error("initial error"); - throw DemoError(e.info()); - } catch (Error &e) { - ErrorInfo ei = e.info(); - // using normaltxt to avoid the default yellow highlighting. - ei.hint = hintfmt("%s; subsequent error message.", - normaltxt(e.info().hint ? e.info().hint->str() : "")); - logger->logEI(ei); - } - - // SysError; picks up errno - try { - auto x = readFile(-1); - } - catch (SysError &e) { - std::cout << "errno was: " << e.errNo << std::endl; - logError(e.info()); - } - - // current exception - try { - throw DemoError("DemoError handled as a %1%", "std::exception"); - } - catch (...) { - const std::exception_ptr &eptr = std::current_exception(); - try - { - std::rethrow_exception(eptr); - } - catch (std::exception& e) - { - std::cerr << e.what() << std::endl; - } - } - - // For completeness sake, show 'info' through 'vomit' levels. - // But this is maybe a heavy format for those. - logger->logEI({ .level = lvlInfo, - .name = "Info name", - .description = "Info description", - }); - - logger->logEI({ .level = lvlTalkative, - .name = "Talkative name", - .description = "Talkative description", - }); - - logger->logEI({ .level = lvlChatty, - .name = "Chatty name", - .description = "Chatty description", - }); - - logger->logEI({ .level = lvlDebug, - .name = "Debug name", - .description = "Debug description", - }); - - logger->logEI({ .level = lvlVomit, - .name = "Vomit name", - .description = "Vomit description", - }); - - // Error in a program; no hint and no nix code. - logError({ - .name = "name", - .description = "error description", - }); - - // Warning with name, description, and hint. - // The hintfmt function makes all the substituted text yellow. - logWarning({ - .name = "name", - .description = "error description", - .hint = hintfmt("there was a %1%", "warning"), - }); - - // Warning with nix file, line number, column, and the lines of - // code where a warning occurred. - SymbolTable testTable; - auto problem_file = testTable.create("myfile.nix"); - - logWarning({ - .name = "warning name", - .description = "warning description", - .hint = hintfmt("this hint has %1% templated %2%!!", - "yellow", - "values"), - .nixCode = NixCode { - .errPos = Pos(problem_file, 40, 13), - .prevLineOfCode = std::nullopt, - .errLineOfCode = "this is the problem line of code", - .nextLineOfCode = std::nullopt - }}); - - // Error with previous and next lines of code. - logError({ - .name = "error name", - .description = "error with code lines", - .hint = hintfmt("this hint has %1% templated %2%!!", - "yellow", - "values"), - .nixCode = NixCode { - .errPos = Pos(problem_file, 40, 13), - .prevLineOfCode = "previous line of code", - .errLineOfCode = "this is the problem line of code", - .nextLineOfCode = "next line of code", - }}); - - - // Error without lines of code. - logError({ - .name = "error name", - .description = "error without any code lines.", - .hint = hintfmt("this hint has %1% templated %2%!!", - "yellow", - "values"), - .nixCode = NixCode { - .errPos = Pos(problem_file, 40, 13) - }}); - - // Error with only hint and name.. - logError({ - .name = "error name", - .hint = hintfmt("hint %1%", "only"), - .nixCode = NixCode { - .errPos = Pos(problem_file, 40, 13) - }}); - - return 0; -} diff --git a/src/error-demo/local.mk b/src/error-demo/local.mk deleted file mode 100644 index 2c528490a..000000000 --- a/src/error-demo/local.mk +++ /dev/null @@ -1,12 +0,0 @@ -programs += error-demo - -error-demo_DIR := $(d) - -error-demo_SOURCES := \ - $(wildcard $(d)/*.cc) \ - -error-demo_CXXFLAGS += -I src/libutil -I src/libexpr - -error-demo_LIBS = libutil libexpr - -error-demo_LDFLAGS = -pthread $(SODIUM_LIBS) $(EDITLINE_LIBS) $(BOOST_LDFLAGS) -lboost_context -lboost_thread -lboost_system From 94c347577ecea5dcd10a31ebfadf94db6ca5ab0d Mon Sep 17 00:00:00 2001 From: Ben Burdette Date: Sun, 7 Jun 2020 07:24:49 -0600 Subject: [PATCH 58/58] set verbosity levels --- src/libutil/tests/logging.cc | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/libutil/tests/logging.cc b/src/libutil/tests/logging.cc index ac015a16d..fbdc91253 100644 --- a/src/libutil/tests/logging.cc +++ b/src/libutil/tests/logging.cc @@ -78,6 +78,8 @@ namespace nix { } TEST(logEI, loggingErrorOnTalkativeLevel) { + verbosity = lvlTalkative; + testing::internal::CaptureStderr(); logger->logEI({ .level = lvlTalkative, @@ -86,11 +88,12 @@ namespace nix { }); auto str = testing::internal::GetCapturedStderr(); - // XXX: why is this the empty string? - ASSERT_STREQ(str.c_str(), ""); + ASSERT_STREQ(str.c_str(), "\x1B[32;1mtalk:\x1B[0m\x1B[34;1m --- Talkative name -------------------------------- error-unit-test\x1B[0m\nTalkative description\n"); } TEST(logEI, loggingErrorOnChattyLevel) { + verbosity = lvlChatty; + testing::internal::CaptureStderr(); logger->logEI({ .level = lvlChatty, @@ -99,11 +102,12 @@ namespace nix { }); auto str = testing::internal::GetCapturedStderr(); - // XXX: why is this the empty string? - ASSERT_STREQ(str.c_str(), ""); + ASSERT_STREQ(str.c_str(), "\x1B[32;1mchat:\x1B[0m\x1B[34;1m --- Chatty name ----------------------------------- error-unit-test\x1B[0m\nTalkative description\n"); } TEST(logEI, loggingErrorOnDebugLevel) { + verbosity = lvlDebug; + testing::internal::CaptureStderr(); logger->logEI({ .level = lvlDebug, @@ -112,11 +116,12 @@ namespace nix { }); auto str = testing::internal::GetCapturedStderr(); - // XXX: why is this the empty string? - ASSERT_STREQ(str.c_str(), ""); + ASSERT_STREQ(str.c_str(), "\x1B[33;1mdebug:\x1B[0m\x1B[34;1m --- Debug name ----------------------------------- error-unit-test\x1B[0m\nDebug description\n"); } TEST(logEI, loggingErrorOnVomitLevel) { + verbosity = lvlVomit; + testing::internal::CaptureStderr(); logger->logEI({ .level = lvlVomit, @@ -125,8 +130,7 @@ namespace nix { }); auto str = testing::internal::GetCapturedStderr(); - // XXX: why is this the empty string? - ASSERT_STREQ(str.c_str(), ""); + ASSERT_STREQ(str.c_str(), "\x1B[32;1mvomit:\x1B[0m\x1B[34;1m --- Vomit name ----------------------------------- error-unit-test\x1B[0m\nVomit description\n"); } /* ----------------------------------------------------------------------------