Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tell in detail why remote building fails on -v #3927

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 20 additions & 14 deletions src/build-remote/build-remote.cc
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,13 @@ static int main_build_remote(int argc, char * * argv)
auto amWilling = readInt(source);
auto neededSystem = readString(source);
drvPath = store->parseStorePath(readString(source));

std::string drvstr;
if (drvPath.has_value())
drvstr = drvPath->to_string();
else
drvstr = "<unknown>";

auto requiredFeatures = readStrings<std::set<std::string>>(source);

/* It would be possible to build locally after some builds clear out,
Expand All @@ -137,14 +144,19 @@ 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"
|| std::find(m.systemTypes.begin(),
m.systemTypes.end(),
neededSystem) != m.systemTypes.end()) &&
m.allSupported(requiredFeatures) &&
m.mandatoryMet(requiredFeatures))
{
if (!m.enabled) {
debug("declined remote machine '%s' because it is disabled", storeUri);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
debug("declined remote machine '%s' because it is disabled", storeUri);
debug("avoiding remote machine '%s' because it is disabled", storeUri);

How about avoid for all of these?

The problem with decline is that it seems like the machine is initiating the remote build or something.

} else if (neededSystem != "builtin" &&
std::find(m.systemTypes.begin(), m.systemTypes.end(), neededSystem)
== m.systemTypes.end()) {
debug("declined remote machine '%s' for building '%s' because it does not have the needed system type '%s", m.storeUri, drvstr, neededSystem);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which one does it have then?

} else if (!m.allSupported(requiredFeatures)) {
std::string joinedFeatures = concatStringsSep(" ", requiredFeatures); // includes leading space
debug("declined remote machine '%s' for building '%s' because it does not have all required features: [ %s ]", m.storeUri, drvstr, joinedFeatures);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be explicit on what was expected and what is available.

} else if (!m.mandatoryMet(requiredFeatures)) {
std::string joinedFeatures = concatStringsSep(" ", requiredFeatures); // includes leading space
debug("declined remote machine '%s' for building '%s' because the derivation does not have all mandatory features to build on that machine: [ %s ]", m.storeUri, drvstr, joinedFeatures);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what this means. I thought a machine may be lacking features to build a drv, how would it work the other way round?

Copy link
Contributor

@tomberek tomberek Sep 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likely the message is backwards

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's accurate as is, but "decline" might not be the best word perhaps (for all of these)

} else {
rightType = true;
AutoCloseFD free;
uint64_t load = 0;
Expand Down Expand Up @@ -199,12 +211,6 @@ static int main_build_remote(int argc, char * * argv)
errorText += "\n([%s], %s, [%s], [%s])";

// add the template values.
std::string drvstr;
if (drvPath.has_value())
drvstr = drvPath->to_string();
else
drvstr = "<unknown>";

auto error = hintformat(errorText);
error
% drvstr
Expand Down
11 changes: 8 additions & 3 deletions src/libstore/build/worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -283,9 +283,14 @@ void Worker::run(const Goals & _topGoals)
"or enable remote builds."
"\nhttps://nixos.org/manual/nix/stable/advanced-topics/distributed-builds.html");
else
throw Error("unable to start any build; remote machines may not have "
"all required system features."
"\nhttps://nixos.org/manual/nix/stable/advanced-topics/distributed-builds.html");
throw Error("unable to start any build; either increase '--max-jobs' "
"to also allow building locally, or ensure that the configured"
"remote builder machines have all required system features."
"For example, to enable the 'big-parallel' feature, use:"
"\n --builders 'yourbuilderhostname - - - - big-parallel'"
"\nhttps://nixos.org/nix/manual/stable/advanced-topics/distributed-builds"
Comment on lines +289 to +291
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should instead refer to the features section in man nix.conf as we did for other hints in error messages recently.

"\nYou can enable -v verbosity to see why any machine "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"\nYou can enable -v verbosity to see why any machine "
"\nEnable -v verbosity to show why any machine "

"was declined for any given build.");

}
assert(!awake.empty());
Expand Down