From 29f93e1e0d405440b35f1aa7c601dc5f601f9a90 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Fri, 3 May 2024 01:39:07 +0200 Subject: [PATCH 1/6] libutil: throw EndOfFile at sourceToSink end ... how has this never broken anything before Change-Id: If3789c02028e8f929481514f63d76b0b46bfc182 --- src/libutil/serialise.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index 692144b75..a6cc919d2 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -230,7 +230,7 @@ std::unique_ptr sourceToSink(std::function fun) if (cur.empty()) { yield(); if (yield.get()) { - return (size_t)0; + throw EndOfFile("coroutine exhausted"); } } From 230860dbb8fc061fd55b98cedd134cdc5097ea69 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sat, 27 Apr 2024 18:51:39 +0200 Subject: [PATCH 2/6] libstore: limit CA realisation info substitution concurrency this seems to be an oversight, considering that regular substitutions are concurrency-limited. while not particularly necessary at present, once we've removed the `Callback` based interfaces it will be needed. Change-Id: Ide2d08169fcc24752cbd07a1d33fb8482f7034f5 --- src/libstore/build/drv-output-substitution-goal.cc | 14 ++++++++++++++ src/libstore/build/drv-output-substitution-goal.hh | 7 +++++++ src/libstore/build/worker.hh | 1 + 3 files changed, 22 insertions(+) diff --git a/src/libstore/build/drv-output-substitution-goal.cc b/src/libstore/build/drv-output-substitution-goal.cc index b30957c84..0e85650a7 100644 --- a/src/libstore/build/drv-output-substitution-goal.cc +++ b/src/libstore/build/drv-output-substitution-goal.cc @@ -3,6 +3,7 @@ #include "worker.hh" #include "substitution-goal.hh" #include "callback.hh" +#include "signals.hh" namespace nix { @@ -38,6 +39,18 @@ void DrvOutputSubstitutionGoal::tryNext() { trace("trying next substituter"); + /* Make sure that we are allowed to start a substitution. Note that even + if maxSubstitutionJobs == 0, we still allow a substituter to run. This + prevents infinite waiting. */ + if (worker.runningCASubstitutions >= std::max(1U, settings.maxSubstitutionJobs.get())) { + worker.waitForBuildSlot(shared_from_this()); + return; + } + + maintainRunningSubstitutions = + std::make_unique>(worker.runningCASubstitutions); + worker.updateProgress(); + if (subs.size() == 0) { /* None left. Terminate this goal and let someone else deal with it. */ @@ -87,6 +100,7 @@ void DrvOutputSubstitutionGoal::tryNext() void DrvOutputSubstitutionGoal::realisationFetched() { worker.childTerminated(this); + maintainRunningSubstitutions.reset(); try { outputInfo = downloadState->promise.get_future().get(); diff --git a/src/libstore/build/drv-output-substitution-goal.hh b/src/libstore/build/drv-output-substitution-goal.hh index da2426e5e..ab6fb796e 100644 --- a/src/libstore/build/drv-output-substitution-goal.hh +++ b/src/libstore/build/drv-output-substitution-goal.hh @@ -41,6 +41,13 @@ class DrvOutputSubstitutionGoal : public Goal { */ std::shared_ptr sub; + /** + * The substituter thread. + */ + std::thread thr; + + std::unique_ptr> maintainRunningSubstitutions; + struct DownloadState { Pipe outPipe; diff --git a/src/libstore/build/worker.hh b/src/libstore/build/worker.hh index 23ad87914..ba4cd88d7 100644 --- a/src/libstore/build/worker.hh +++ b/src/libstore/build/worker.hh @@ -166,6 +166,7 @@ public: uint64_t doneSubstitutions = 0; uint64_t failedSubstitutions = 0; uint64_t runningSubstitutions = 0; + uint64_t runningCASubstitutions = 0; uint64_t expectedDownloadSize = 0; uint64_t doneDownloadSize = 0; uint64_t expectedNarSize = 0; From 964ac8b0e88fb5789b87e33273e42363958d0afb Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sat, 27 Apr 2024 01:02:22 +0200 Subject: [PATCH 3/6] libutil: de-callback-ify computeClosure only two users of this function exist. only one used it in a way that even bears resemblance to asynchronicity, and even that one didn't do it right. fully async and parallel computation would have only worked if any getEdgesAsync never calls the continuation it receives itself, only from more derived callbacks running on other threads. calling it directly would cause the decoupling promise to be awaited immediately *on the original thread*, completely negating all nice async effects. Change-Id: I0aa640950cf327533a32dee410105efdabb448df --- src/libstore/misc.cc | 32 +++++------------ src/libstore/realisation.cc | 15 +------- src/libutil/closure.hh | 68 ++++++++--------------------------- tests/unit/libutil/closure.cc | 35 +++--------------- 4 files changed, 27 insertions(+), 123 deletions(-) diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index c8646083b..22f6b67ee 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -14,10 +14,9 @@ namespace nix { void Store::computeFSClosure(const StorePathSet & startPaths, StorePathSet & paths_, bool flipDirection, bool includeOutputs, bool includeDerivers) { - std::function(const StorePath & path, std::future> &)> queryDeps; + std::function(const StorePath & path, ref)> queryDeps; if (flipDirection) - queryDeps = [&](const StorePath& path, - std::future> & fut) { + queryDeps = [&](const StorePath& path, ref) { StorePathSet res; StorePathSet referrers; queryReferrers(path, referrers); @@ -36,10 +35,8 @@ void Store::computeFSClosure(const StorePathSet & startPaths, return res; }; else - queryDeps = [&](const StorePath& path, - std::future> & fut) { + queryDeps = [&](const StorePath& path, ref info) { StorePathSet res; - auto info = fut.get(); for (auto& ref : info->references) if (ref != path) res.insert(ref); @@ -54,24 +51,11 @@ void Store::computeFSClosure(const StorePathSet & startPaths, return res; }; - computeClosure( - startPaths, paths_, - [&](const StorePath& path, - std::function>&)> - processEdges) { - std::promise> promise; - std::function>)> - getDependencies = - [&](std::future> fut) { - try { - promise.set_value(queryDeps(path, fut)); - } catch (...) { - promise.set_exception(std::current_exception()); - } - }; - queryPathInfo(path, getDependencies); - processEdges(promise); - }); + paths_.merge(computeClosure( + startPaths, + [&](const StorePath& path) -> std::set { + return queryDeps(path, queryPathInfo(path)); + })); } void Store::computeFSClosure(const StorePath & startPath, diff --git a/src/libstore/realisation.cc b/src/libstore/realisation.cc index 93ddb5b20..50c911da0 100644 --- a/src/libstore/realisation.cc +++ b/src/libstore/realisation.cc @@ -43,20 +43,7 @@ void Realisation::closure(Store & store, const std::set & startOutp return res; }; - computeClosure( - startOutputs, res, - [&](const Realisation& current, - std::function>&)> - processEdges) { - std::promise> promise; - try { - auto res = getDeps(current); - promise.set_value(res); - } catch (...) { - promise.set_exception(std::current_exception()); - } - return processEdges(promise); - }); + res.merge(computeClosure(startOutputs, getDeps)); } nlohmann::json Realisation::toJSON() const { diff --git a/src/libutil/closure.hh b/src/libutil/closure.hh index 16e3b93e4..146931fc8 100644 --- a/src/libutil/closure.hh +++ b/src/libutil/closure.hh @@ -1,72 +1,32 @@ #pragma once ///@file +#include #include -#include -#include "sync.hh" - -using std::set; namespace nix { template -using GetEdgesAsync = std::function> &)>)>; - -template -void computeClosure( - const set startElts, - set & res, - GetEdgesAsync getEdgesAsync +std::set computeClosure( + std::set startElts, + std::function(const T &)> getEdges ) { - struct State - { - size_t pending; - set & res; - std::exception_ptr exc; - }; + std::set res, queue = std::move(startElts); - Sync state_(State{0, res, 0}); + while (!queue.empty()) { + std::set next; - std::function enqueue; - - std::condition_variable done; - - enqueue = [&](const T & current) -> void { - { - auto state(state_.lock()); - if (state->exc) return; - if (!state->res.insert(current).second) return; - state->pending++; + for (auto & e : queue) { + if (res.insert(e).second) { + next.merge(getEdges(e)); + } } - getEdgesAsync(current, [&](std::promise> & prom) { - try { - auto children = prom.get_future().get(); - for (auto & child : children) - enqueue(child); - { - auto state(state_.lock()); - assert(state->pending); - if (!--state->pending) done.notify_one(); - } - } catch (...) { - auto state(state_.lock()); - if (!state->exc) state->exc = std::current_exception(); - assert(state->pending); - if (!--state->pending) done.notify_one(); - }; - }); - }; - - for (auto & startElt : startElts) - enqueue(startElt); - - { - auto state(state_.lock()); - while (state->pending) state.wait(done); - if (state->exc) std::rethrow_exception(state->exc); + queue = std::move(next); } + + return res; } } diff --git a/tests/unit/libutil/closure.cc b/tests/unit/libutil/closure.cc index 7597e7807..b4eaad6f9 100644 --- a/tests/unit/libutil/closure.cc +++ b/tests/unit/libutil/closure.cc @@ -16,15 +16,11 @@ map> testGraph = { }; TEST(closure, correctClosure) { - set aClosure; set expectedClosure = {"A", "B", "C", "F", "G"}; - computeClosure( + set aClosure = computeClosure( {"A"}, - aClosure, - [&](const string currentNode, function> &)> processEdges) { - promise> promisedNodes; - promisedNodes.set_value(testGraph[currentNode]); - processEdges(promisedNodes); + [&](const string currentNode) { + return testGraph[currentNode]; } ); @@ -33,12 +29,10 @@ TEST(closure, correctClosure) { TEST(closure, properlyHandlesDirectExceptions) { struct TestExn {}; - set aClosure; EXPECT_THROW( computeClosure( {"A"}, - aClosure, - [&](const string currentNode, function> &)> processEdges) { + [&](const string currentNode) -> set { throw TestExn(); } ), @@ -46,25 +40,4 @@ TEST(closure, properlyHandlesDirectExceptions) { ); } -TEST(closure, properlyHandlesExceptionsInPromise) { - struct TestExn {}; - set aClosure; - EXPECT_THROW( - computeClosure( - {"A"}, - aClosure, - [&](const string currentNode, function> &)> processEdges) { - promise> promise; - try { - throw TestExn(); - } catch (...) { - promise.set_exception(std::current_exception()); - } - processEdges(promise); - } - ), - TestExn - ); -} - } From 4f98d21b71d02c09cab9df9778fcc03df4bcbd4e Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 6 May 2024 22:50:32 -0600 Subject: [PATCH 4/6] flake: move the pre-commit definition to its own file It's a good hundred LOC, and wasn't coupled to the actual flake logic at all. Change-Id: Iebb4667b3197dbd8cb2b019014e99fa651848832 --- flake.nix | 83 +------------------------------------ misc/pre-commit.nix | 99 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 82 deletions(-) create mode 100644 misc/pre-commit.nix diff --git a/flake.nix b/flake.nix index 1c090cadf..6594961ff 100644 --- a/flake.nix +++ b/flake.nix @@ -286,89 +286,8 @@ system: let pkgs = nixpkgsFor.${system}.native; - # Import pre-commit bypassing the flake because flakes don't let - # you have overlays. Also their implementation forces an - # unnecessary reimport of nixpkgs for our use cases. - tools = import (pre-commit-hooks + "/nix/call-tools.nix") pkgs; - pre-commit-run = pkgs.callPackage (pre-commit-hooks + "/nix/run.nix") { - inherit tools; - isFlakes = true; - # unused! - gitignore-nix-src = builtins.throw "gitignore-nix-src is unused"; - }; in - pre-commit-run { - src = self; - hooks = { - no-commit-to-branch = { - enable = true; - settings.branch = [ "main" ]; - }; - check-case-conflicts.enable = true; - check-executables-have-shebangs = { - enable = true; - stages = [ "commit" ]; - }; - check-shebang-scripts-are-executable = { - enable = true; - stages = [ "commit" ]; - }; - check-symlinks = { - enable = true; - excludes = [ "^tests/functional/lang/symlink-resolution/broken$" ]; - }; - check-merge-conflicts.enable = true; - end-of-file-fixer = { - enable = true; - excludes = [ - "\\.drv$" - "^tests/functional/lang/" - ]; - }; - mixed-line-endings = { - enable = true; - excludes = [ "^tests/functional/lang/" ]; - }; - release-notes = { - enable = true; - package = pkgs.build-release-notes; - files = "^doc/manual/rl-next(-dev)?"; - pass_filenames = false; - entry = '' - ${lib.getExe pkgs.build-release-notes} doc/manual/rl-next doc/manual/rl-next-dev - ''; - }; - check-headers = { - enable = true; - package = pkgs.check-headers; - files = "^src/"; - types = [ - "c++" - "file" - "header" - ]; - # generated files; these will never actually be seen by this - # check, and are left here as documentation - excludes = [ - "(parser|lexer)-tab\\.hh$" - "\\.gen\\.hh$" - ]; - entry = lib.getExe pkgs.check-headers; - }; - # TODO: Once the test suite is nicer, clean up and start - # enforcing trailing whitespace on tests that don't explicitly - # check for it. - trim-trailing-whitespace = { - enable = true; - stages = [ "commit" ]; - excludes = [ "^tests/functional/lang/" ]; - }; - treefmt = { - enable = true; - settings.formatters = [ pkgs.nixfmt ]; - }; - }; - } + import ./misc/pre-commit.nix { inherit self pkgs pre-commit-hooks; } ); }; diff --git a/misc/pre-commit.nix b/misc/pre-commit.nix new file mode 100644 index 000000000..b287f3cec --- /dev/null +++ b/misc/pre-commit.nix @@ -0,0 +1,99 @@ +{ + /** + Path to Lix's source, normally the flake's "self" argument + */ + self ? pkgs.lib.cleanSource ./., + /** + Already instantiated Nixpkgs + */ + pkgs, + /** + pre-commit-hooks source path, normally from the flake input + */ + pre-commit-hooks, +}: +let + inherit (pkgs) lib; + # Import pre-commit bypassing the flake because flakes don't let + # you have overlays. Also their implementation forces an + # unnecessary reimport of nixpkgs for our use cases. + tools = import (pre-commit-hooks + "/nix/call-tools.nix") pkgs; + pre-commit-run = pkgs.callPackage (pre-commit-hooks + "/nix/run.nix") { + inherit tools; + isFlakes = true; + # unused! + gitignore-nix-src = builtins.throw "gitignore-nix-src is unused"; + }; +in +pre-commit-run { + src = self; + hooks = { + no-commit-to-branch = { + enable = true; + settings.branch = [ "main" ]; + }; + check-case-conflicts.enable = true; + check-executables-have-shebangs = { + enable = true; + stages = [ "commit" ]; + }; + check-shebang-scripts-are-executable = { + enable = true; + stages = [ "commit" ]; + }; + check-symlinks = { + enable = true; + excludes = [ "^tests/functional/lang/symlink-resolution/broken$" ]; + }; + check-merge-conflicts.enable = true; + end-of-file-fixer = { + enable = true; + excludes = [ + "\\.drv$" + "^tests/functional/lang/" + ]; + }; + mixed-line-endings = { + enable = true; + excludes = [ "^tests/functional/lang/" ]; + }; + release-notes = { + enable = true; + package = pkgs.build-release-notes; + files = "^doc/manual/rl-next(-dev)?"; + pass_filenames = false; + entry = '' + ${lib.getExe pkgs.build-release-notes} doc/manual/rl-next doc/manual/rl-next-dev + ''; + }; + check-headers = { + enable = true; + package = pkgs.check-headers; + files = "^src/"; + types = [ + "c++" + "file" + "header" + ]; + # generated files; these will never actually be seen by this + # check, and are left here as documentation + excludes = [ + "(parser|lexer)-tab\\.hh$" + "\\.gen\\.hh$" + ]; + entry = lib.getExe pkgs.check-headers; + }; + # TODO: Once the test suite is nicer, clean up and start + # enforcing trailing whitespace on tests that don't explicitly + # check for it. + trim-trailing-whitespace = { + enable = true; + stages = [ "commit" ]; + excludes = [ "^tests/functional/lang/" ]; + }; + treefmt = { + enable = true; + settings.formatters = [ pkgs.nixfmt ]; + }; + }; +} From aac32327d5d974cf03fee622558210a37bcdd0e6 Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 6 May 2024 22:54:18 -0600 Subject: [PATCH 5/6] flake: fix eval of checks & devshell on i686-linux Change-Id: I62da3161327051005e3f48f83974140efef4417e --- flake.nix | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/flake.nix b/flake.nix index 6594961ff..3dc9b3ac3 100644 --- a/flake.nix +++ b/flake.nix @@ -96,6 +96,10 @@ ]; forAllSystems = lib.genAttrs systems; + # Same as forAllSystems, but removes nulls, in case something is broken + # on that system. + forAvailableSystems = + f: lib.filterAttrs (name: value: value != null && value != { }) (forAllSystems f); forAllCrossSystems = lib.genAttrs crossSystems; @@ -282,18 +286,21 @@ ); }; - pre-commit = forAllSystems ( + pre-commit = forAvailableSystems ( system: let pkgs = nixpkgsFor.${system}.native; + pre-commit-check = import ./misc/pre-commit.nix { inherit self pkgs pre-commit-hooks; }; + # dotnet-sdk_6, a nativeBuildInputs of pre-commit, is broken on i686-linux. + available = lib.meta.availableOn { inherit system; } pkgs.dotnet-sdk_6; in - import ./misc/pre-commit.nix { inherit self pkgs pre-commit-hooks; } + lib.optionalAttrs available pre-commit-check ); }; # NOTE *do not* add fresh derivations to checks, always add them to # hydraJobs first (so CI will pick them up) and only link them here - checks = forAllSystems ( + checks = forAvailableSystems ( system: { binaryTarball = self.hydraJobs.binaryTarball.${system}; @@ -301,6 +308,7 @@ nixpkgsLibTests = self.hydraJobs.tests.nixpkgsLibTests.${system}; rl-next = self.hydraJobs.rl-next.${system}.user; rl-next-dev = self.hydraJobs.rl-next.${system}.dev; + # Will be empty attr set on i686-linux, and filtered out by forAvailableSystems. pre-commit = self.hydraJobs.pre-commit.${system}; } // (lib.optionalAttrs (builtins.elem system linux64BitSystems)) { From 851b42f6b4122d942d2ddc968b1d82648e001cbf Mon Sep 17 00:00:00 2001 From: Qyriad Date: Mon, 6 May 2024 22:59:41 -0600 Subject: [PATCH 6/6] flake: fix devShell on i686-linux by disabling ClangBuildAnalyzer on it ClangBuildAnalyzer doesn't build on i686-linux due to `long long int`/`size_t` conversion errors, so let's just exclude it from the devshell on that platform Change-Id: If1077a7b3860db4381999c8e304f6d4b2bc96a05 --- flake.nix | 3 ++- misc/clangbuildanalyzer.nix | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/flake.nix b/flake.nix index 3dc9b3ac3..fd9a7cfa3 100644 --- a/flake.nix +++ b/flake.nix @@ -408,7 +408,8 @@ pkgs.buildPackages.meson pkgs.buildPackages.ninja pkgs.buildPackages.cmake - + ] + ++ lib.optionals (lib.meta.availableOn pkgs.system pkgs.buildPackages.clangbuildanalyzer) [ pkgs.buildPackages.clangbuildanalyzer ]; diff --git a/misc/clangbuildanalyzer.nix b/misc/clangbuildanalyzer.nix index cee8a0d77..31d6204ce 100644 --- a/misc/clangbuildanalyzer.nix +++ b/misc/clangbuildanalyzer.nix @@ -25,6 +25,8 @@ stdenv.mkDerivation (finalAttrs: { maintainers = with lib.maintainers; [ lf- ]; license = lib.licenses.unlicense; platforms = lib.platforms.unix; + # `long long int` != `size_t` + badPlatforms = lib.platforms.i686; mainProgram = "ClangBuildAnalyzer"; }; })