From 739032762addcb3d88490040b388ff63b155bb16 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 23 Jan 2024 12:30:26 -0500 Subject: [PATCH 1/3] Make `Machine::systemTypes` a set not vector This is more conceptually correct (the order does not matter), and also matches what Hydra already does. (Nix and Hydra matching is needed for dedup https://github.com/NixOS/hydra/issues/1164) --- src/build-remote/build-remote.cc | 6 ++---- src/libstore/machines.cc | 2 +- src/libstore/machines.hh | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index d69d3a0c2..b6704152a 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -139,9 +139,7 @@ static int main_build_remote(int argc, char * * argv) if (m.enabled && (neededSystem == "builtin" - || std::find(m.systemTypes.begin(), - m.systemTypes.end(), - neededSystem) != m.systemTypes.end()) && + || m.systemTypes.count(neededSystem) > 0) && m.allSupported(requiredFeatures) && m.mandatoryMet(requiredFeatures)) { @@ -214,7 +212,7 @@ static int main_build_remote(int argc, char * * argv) for (auto & m : machines) error - % concatStringsSep>(", ", m.systemTypes) + % concatStringsSep(", ", m.systemTypes) % m.maxJobs % concatStringsSep(", ", m.supportedFeatures) % concatStringsSep(", ", m.mandatoryFeatures); diff --git a/src/libstore/machines.cc b/src/libstore/machines.cc index 512115893..8a1da84cd 100644 --- a/src/libstore/machines.cc +++ b/src/libstore/machines.cc @@ -145,7 +145,7 @@ static Machine parseBuilderLine(const std::string & line) return { tokens[0], - isSet(1) ? tokenizeString>(tokens[1], ",") : std::vector{settings.thisSystem}, + isSet(1) ? tokenizeString>(tokens[1], ",") : std::set{settings.thisSystem}, isSet(2) ? tokens[2] : "", isSet(3) ? parseUnsignedIntField(3) : 1U, isSet(4) ? parseUnsignedIntField(4) : 1U, diff --git a/src/libstore/machines.hh b/src/libstore/machines.hh index 1adeaf1f0..d25fdf1b3 100644 --- a/src/libstore/machines.hh +++ b/src/libstore/machines.hh @@ -10,7 +10,7 @@ class Store; struct Machine { const std::string storeUri; - const std::vector systemTypes; + const std::set systemTypes; const std::string sshKey; const unsigned int maxJobs; const unsigned int speedFactor; From 870acc2892661d1d2c9f9f39c43d79cb4bbaacb0 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 23 Jan 2024 12:50:48 -0500 Subject: [PATCH 2/3] Add API docs to `Machine` methods --- src/libstore/machines.hh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/libstore/machines.hh b/src/libstore/machines.hh index d25fdf1b3..7dd812cf0 100644 --- a/src/libstore/machines.hh +++ b/src/libstore/machines.hh @@ -19,8 +19,15 @@ struct Machine { const std::string sshPublicHostKey; bool enabled = true; + /** + * @return Whether `features` is a subset of the union of `supportedFeatures` and + * `mandatoryFeatures` + */ bool allSupported(const std::set & features) const; + /** + * @return @Whether `mandatoryFeatures` is a subset of `features` + */ bool mandatoryMet(const std::set & features) const; Machine(decltype(storeUri) storeUri, From 0aa85088dee30615adcc7a2933fb94ea8767ec35 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 23 Jan 2024 12:52:54 -0500 Subject: [PATCH 3/3] Factor out `Machine::systemSupported` There's just enough logic (the `"builtin"` special case) that makes this worthy of its own method. --- src/build-remote/build-remote.cc | 5 ++--- src/libstore/machines.cc | 5 +++++ src/libstore/machines.hh | 6 ++++++ 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index b6704152a..519e03242 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -137,9 +137,8 @@ static int main_build_remote(int argc, char * * argv) for (auto & m : machines) { debug("considering building on remote machine '%s'", m.storeUri); - if (m.enabled - && (neededSystem == "builtin" - || m.systemTypes.count(neededSystem) > 0) && + if (m.enabled && + m.systemSupported(neededSystem) && m.allSupported(requiredFeatures) && m.mandatoryMet(requiredFeatures)) { diff --git a/src/libstore/machines.cc b/src/libstore/machines.cc index 8a1da84cd..561d8d557 100644 --- a/src/libstore/machines.cc +++ b/src/libstore/machines.cc @@ -38,6 +38,11 @@ Machine::Machine(decltype(storeUri) storeUri, sshPublicHostKey(sshPublicHostKey) {} +bool Machine::systemSupported(const std::string & system) const +{ + return system == "builtin" || (systemTypes.count(system) > 0); +} + bool Machine::allSupported(const std::set & features) const { return std::all_of(features.begin(), features.end(), diff --git a/src/libstore/machines.hh b/src/libstore/machines.hh index 7dd812cf0..1bca74c28 100644 --- a/src/libstore/machines.hh +++ b/src/libstore/machines.hh @@ -19,6 +19,12 @@ struct Machine { const std::string sshPublicHostKey; bool enabled = true; + /** + * @return Whether `system` is either `"builtin"` or in + * `systemTypes`. + */ + bool systemSupported(const std::string & system) const; + /** * @return Whether `features` is a subset of the union of `supportedFeatures` and * `mandatoryFeatures`