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.

Sign up for GitHub

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

Make --keep-going work with drv that don't have a system #12168

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make --keep-going work with drv that don't have a system #12168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions src/libstore/build-result.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ struct BuildResult
NotDeterministic,
ResolvesToAlreadyValid,
NoSubstituters,
NoCompatibleBuilder,
} status = MiscFailure;

/**
Expand Down Expand Up @@ -64,6 +65,7 @@ struct BuildResult
case NotDeterministic: return "NotDeterministic";
case ResolvesToAlreadyValid: return "ResolvesToAlreadyValid";
case NoSubstituters: return "NoSubstituters";
case NoCompatibleBuilder: return "NoCompatibleBuilder";
default: return "Unknown";
};
}();
Expand Down
4 changes: 2 additions & 2 deletions src/libstore/build/worker.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ void Worker::run(const Goals & _topGoals)
waitForInput();
else if (awake.empty() && 0U == settings.maxBuildJobs) {
if (getMachines().empty())
throw Error(
throw NoCompatibleBuilder(
R"(
Unable to start any build;
either increase '--max-jobs' or enable remote builds.
Expand All @@ -348,7 +348,7 @@ void Worker::run(const Goals & _topGoals)
)"
);
else
throw Error(
throw NoCompatibleBuilder(
R"(
Unable to start any build;
remote machines may not have all required system features.
Expand Down
1 change: 1 addition & 0 deletions src/libstore/store-api.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ MakeError(InvalidPath, Error);
MakeError(Unsupported, Error);
MakeError(SubstituteGone, Error);
MakeError(SubstituterDisabled, Error);
MakeError(NoCompatibleBuilder, Error);

MakeError(InvalidStoreReference, Error);

Expand Down
6 changes: 5 additions & 1 deletion src/libstore/unix/build/local-derivation-goal.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,10 @@ Goal::Co LocalDerivationGoal::tryLocalBuild()
buildUser.reset();
worker.permanentFailure = true;
co_return done(BuildResult::InputRejected, {}, std::move(e));
} catch (NoCompatibleBuilder & e) {
outputLocks.unlock();
buildUser.reset();
co_return done(BuildResult::NoCompatibleBuilder, {}, std::move(e));
}

started();
Expand Down Expand Up @@ -532,7 +536,7 @@ void LocalDerivationGoal::startBuilder()

/* Right platform? */
if (!parsedDrv->canBuildLocally(worker.store))
throw Error("a '%s' with features {%s} is required to build '%s', but I am a '%s' with features {%s}",
throw NoCompatibleBuilder("a '%s' with features {%s} is required to build '%s', but I am a '%s' with features {%s}",
drv->platform,
concatStringsSep(", ", parsedDrv->getRequiredSystemFeatures()),
worker.store.printStorePath(drvPath),
Expand Down
33 changes: 33 additions & 0 deletions tests/functional/keep-going.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
with import ./config.nix;

rec {

# Hack to get the scheduler to do what we want: The `good` derivation can
# only be built after `delay_good` (which takes a long time to build) while
# the others don't have any dependency.
# This means that if we build this with parallelism (`-j2`), then we can be
# reasonably sure that the failing derivations will be scheduled _before_ the
# `good` one (and so we can check that `--keep-going` works fine)
Comment on lines +5 to +10
Copy link
Member

Choose a reason for hiding this comment

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

This could be synchronized by waiting for a file to appear in a mutable directory in extra-sandbox-paths. The test would have to monitor the CLI's log before creating it.
(doing it in the failing derivation would only decrease the length of the race condition, but not make it reliable)

Something along the lines of

nix build --extra-sandbox-paths "$TEST_ROOT/sync" 2>&1 \
  | while read ln; do
    echo "nix build: $ln"
    if echo "$ln" | grepQuiet ...; then
      touch "$TEST_ROOT/sync/failing-drv-failed"
    fi
  done

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like that might work? I plugged it in. It doesn't break the tests on my machine.

delay_good = mkDerivation {
name = "delay-good";
buildCommand = "sleep 3; touch $out";
};

good = mkDerivation {
name = "good";
buildCommand = "mkdir $out; echo foo > $out/bar";
delay = delay_good;
};

failing = mkDerivation {
name = "failing";
buildCommand = false;
};

requiresFooSystemFeature = mkDerivation {
name = "requires-foo-system-feature";
buildCommand = "mkdir $out; echo foo > $out/bar";
requiredSystemFeatures = [ "foo" ];
};

}
23 changes: 23 additions & 0 deletions tests/functional/keep-going.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#!/usr/bin/env bash

source common.sh

clearStore

mkdir -p "$TEST_ROOT/sync"
# XXX: These invocations of nix-build should always return 100 according to the manpage, but often return 1
Copy link
Member

Choose a reason for hiding this comment

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

Can you create an issue for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, @iFreilicht is the one who discovered the error. Probably could use more details before creating the issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, that's all I found out, I don't have more details for you 😬 But it basically happened during testing, so you should be able to reproduce that issue and can then create one with a proper log. This might also be why the tests are failing right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, alright. I did check the blame and it looks like @Ericson2314 did write some of the worker stuff around where I'm touching. Maybe John has some ideas into getting this to pass.

(! nix-build ./keep-going.nix -j2 --extra-sandbox-paths "$TEST_ROOT/sync" --no-out-link 2>&1 \
| while read -r ln; do
echo "nix build: $ln"
if echo "$ln" | grepQuiet ...; then
touch "$TEST_ROOT/sync/failing-drv-failed"
fi
done)
(! nix-build ./keep-going.nix -A good -j0 --no-out-link) || \
fail "Hello shouldn't have been built because of earlier errors"

clearStore

(! nix-build ./keep-going.nix --keep-going -j2 --no-out-link)
nix-build ./keep-going.nix -A good -j0 --no-out-link || \
fail "Hello should have been built despite the errors because of '--keep-going'"
1 change: 1 addition & 0 deletions tests/functional/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ suites = [
'debugger.sh',
'extra-sandbox-profile.sh',
'help.sh',
'keep-going.sh',
],
'workdir': meson.current_source_dir(),
},
Expand Down
Loading