Unstable --keep-going flag #10383

dtolnay · 2022-02-12T08:07:44Z

Summary

This PR adds an unstable --keep-going flag documented as follows:

cargo build --keep-going (and similarly for check, test etc) will build as many crates in the dependency graph as possible, rather than aborting the build at the first one that fails to build.

For example if the current package depends on dependencies fails and works, one of which fails to build, cargo check -j1 may or may not build the one that succeeds (depending on which one of the two builds Cargo picked to run first), whereas cargo check -j1 --keep-going would definitely run both builds, even if the one run first fails.

The -Z unstable-options command-line option must be used in order to use --keep-going while it is not yet stable:
cargo check --keep-going -Z unstable-options

Prior art

Buck and Bazel and Make all have this flag (though Bazel calls it --keep_going 🤮) with exactly this behavior.

Motivation

I need this in order to make https://github.com/dtolnay/trybuild not super slow.

Trybuild wants to run Cargo on a bunch of test cases, each of which is a bin crate. The bad options currently available are:

Give each test case its own target dir and run build on them in parallel. This is bad because all the test cases have the same dependencies in common (whatever dev-dependencies are declared by the project). If there are 100 test cases, all the dependencies would end up getting built 100 times, which is 100x slower than necessary despite the parallelism.
Reuse a single target dir for all the test cases. Two Cargos can't operate in parallel on the same target directory, so this forces the test cases to be built serially. This is much slower than necessary on a many-core system, and compounds all of the overheads in Cargo because the project structure must be reloaded by each invocation.

The good option I'd like to switch to is:

Run cargo build --bins --keep-going --message-format=json to build all the test cases in parallel. Use the filepaths in the JSON messages to ascribe diagnostics to which bin they're from.

rust-highfive · 2022-02-12T08:07:47Z

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

alexcrichton

Thanks for this! I've often wanted a flag similar to this myself, typically for the reason of "please go keep doing work while I go figure out that one error"

I think though that the error handling should be improved here before landing. For example if you use make -j 10 -k then it will still print the errors for all failed compiles, but here all errors after the first are simply swallowed up and not actually emitted anywhere. In the same manner how we display an error and then punt upwards some arbitrary error I think it would be better to display all errors here (in some fashion that makes them distinctly understandable) and then punt up perhaps something like "N build errors occurred".

Other than that though this seems reasonable to me.

src/bin/cargo/commands/test.rs

alexcrichton · 2022-02-16T16:12:52Z

src/cargo/core/compiler/job_queue.rs

                return Some(e);
+            } else {
+                crate::display_error(&e, &mut cx.bcx.config.shell());


I realize that this was preexisting, but reading over this again I'm finding it a bit confusing to follow why above cases use handle_error but self.timings.finish and the writeln! below both use diferent custom logic. I think, however, that it should be fine to use handle_error in both cases?

Or rather put another way I think it'd be good to consistently use handle_error here if possible.

alexcrichton · 2022-02-16T16:15:51Z

src/cargo/core/compiler/job_queue.rs

+                    drop(shell.warn("build failed, waiting for other jobs to finish..."));
+                    *first_error = Some(anyhow::format_err!("build failed"));
+                } else {
+                    *first_error = Some(new_err);


I keep getting somewhat confused when I think about this personally, and while I realize that this is preserving what's already happening here I think it might be best to restructure this differently. I believe the original intention of this code was, when possible, to return upwards the error we encountered during compilation if only one was encountered. That being said, however, I don't believe we can reliably return upwards an error so I think it would be better to unconditionally, for any errors encountered during the build, to use crate::display_error and print them here. That way there's much less juggling of trying to preserve some "main" error to return while also ensuring that all other errors are printed.

With a refactoring like that I think it might make sense to change DrainErrors from an enum to a struct with a keep_going flag or similar since the only change in behavior would be whether or not more jobs are spawned but otherwise errors are handled consistently between --keep-going-vs-not

alexcrichton · 2022-02-16T16:16:56Z

r? @alexcrichton

@rfcbot fcp merge

This is an unstable flag but personally I'd also be ok landing this insta-stable since this flag has precedent in other build systems. In any case I also wanted to get feedback from other Cargo folks if y'all have it.

rfcbot · 2022-02-16T16:16:57Z

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

fee1-dead · 2022-02-25T12:33:20Z

Another prior art: portage, Gentoo Linux's package manager also has --keep-going. Looks like applications that do fallible operations to a graph have a lot in common..

alexcrichton · 2022-03-08T17:59:36Z

Thanks for the PR, but I'm going to be stepping down from the Cargo team so I'm going to un-assign myself from this. The Cargo team will help review this when they get a chance.

Consistently use crate::display_error on errors during drain As suggested in #10383 (comment) and #10383 (comment).

bors · 2022-03-21T19:37:32Z

☔ The latest upstream changes (presumably #10394) made this pull request unmergeable. Please resolve the merge conflicts.

ehuss · 2022-03-21T20:54:08Z

@dtolnay I'm not sure what the status is here, but if you rebase it now that #10394 is merged, I can re-review it.

dtolnay · 2022-03-22T01:39:57Z

Rebased to resolve conflict with #10394.

The meaningful change in this PR is back down to just one line:

-       if errors.count == 0 {
+       if errors.count == 0 || cx.bcx.build_config.keep_going {

i.e. none of 7e890c33fe012cb267d6fff9698b6dc9e3c4ca76 is needed any longer following #10394.

ehuss · 2022-03-23T22:05:23Z

@rfcbot cancel

rfcbot · 2022-03-23T22:05:24Z

@ehuss proposal cancelled.

ehuss · 2022-03-23T22:08:48Z

@rfcbot merge

@rust-lang/cargo This is adds an unstable flag --keep-going which is described above. There seems to be some precedent for this in other tools. Restarting FCP since this is a new feature which we haven't discussed previously.

rfcbot · 2022-03-23T22:08:49Z

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

Mark-Simulacrum · 2022-03-30T14:17:19Z

@bors retry

(Triaging bors problems, don't mind me.)

Mark-Simulacrum · 2022-03-30T14:19:36Z

@bors r- r=ehuss

bors · 2022-03-30T14:19:39Z

📌 Commit 4e45f58 has been approved by ehuss

Mark-Simulacrum · 2022-03-30T14:45:01Z

@bors r- r=ehuss

bors · 2022-03-30T14:45:04Z

📌 Commit 4e45f58 has been approved by ehuss

bors · 2022-03-30T15:25:34Z

⌛ Testing commit 4e45f58 with merge 9090042...

bors · 2022-03-30T16:08:13Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 9090042 to master...

Update cargo 13 commits in 109bfbd055325ef87a6e7f63d67da7e838f8300b..1ef1e0a12723ce9548d7da2b63119de9002bead8 2022-03-17 21:43:09 +0000 to 2022-03-31 00:17:18 +0000 - Support `-Zmultitarget` in cargo config (rust-lang/cargo#10473) - doc: Fix document url for libcurl format (rust-lang/cargo#10515) - Fix wrong info in "Environment variables" docs (rust-lang/cargo#10513) - Use the correct flag in --locked --offline error message (rust-lang/cargo#10512) - Don't treat host/target duplicates as duplicates (rust-lang/cargo#10466) - Unstable --keep-going flag (rust-lang/cargo#10383) - Part 1 of RFC2906 - Packages can inherit fields from their root workspace (rust-lang/cargo#10497) - Remove unused profile support for -Zpanic-abort-tests (rust-lang/cargo#10495) - HTTP registry implementation (rust-lang/cargo#10470) - Add a notice about review capacity. (rust-lang/cargo#10501) - Add tests for ignoring symlinks (rust-lang/cargo#10047) - Update doc string for deps_of/compute_deps. (rust-lang/cargo#10494) - Consistently use crate::display_error on errors during drain (rust-lang/cargo#10394)

rust-highfive assigned ehuss Feb 12, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 12, 2022

alexcrichton reviewed Feb 14, 2022

View reviewed changes

src/bin/cargo/commands/test.rs Outdated Show resolved Hide resolved

alexcrichton reviewed Feb 16, 2022

View reviewed changes

alexcrichton added the T-cargo Team: Cargo label Feb 16, 2022

rust-highfive unassigned ehuss Feb 16, 2022

rfcbot added the proposed-final-comment-period An FCP proposal has started, but not yet signed off. label Feb 16, 2022

rust-highfive assigned alexcrichton Feb 16, 2022

rfcbot added the disposition-merge FCP with intent to merge label Feb 16, 2022

dtolnay mentioned this pull request Feb 16, 2022

Consistently use crate::display_error on errors during drain #10394

Merged

alexcrichton removed their assignment Mar 8, 2022

bors added a commit that referenced this pull request Mar 21, 2022

Auto merge of #10394 - dtolnay-contrib:displayerror, r=ehuss

5e09899

Consistently use crate::display_error on errors during drain As suggested in #10383 (comment) and #10383 (comment).

ehuss self-assigned this Mar 21, 2022

dtolnay mentioned this pull request Mar 22, 2022

Tracking issue for --keep-going #10496

Closed

2 tasks

Unstable --keep-going flag

4e45f58

dtolnay force-pushed the keepgoing branch from 7e890c3 to 4e45f58 Compare March 22, 2022 01:37

rfcbot removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Mar 23, 2022

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 29, 2022

dtolnay mentioned this pull request Mar 30, 2022

Obtain rendered diagnostics through --message-format=json dtolnay/trybuild#156

Merged

bors added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 30, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Mar 30, 2022

Mark-Simulacrum closed this Mar 30, 2022

Mark-Simulacrum reopened this Mar 30, 2022

bors added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 30, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Mar 30, 2022

bors merged commit 9090042 into rust-lang:master Mar 30, 2022

dtolnay deleted the keepgoing branch March 30, 2022 18:22

ehuss mentioned this pull request Mar 31, 2022

Update cargo rust-lang/rust#95507

Merged

This was referenced Apr 2, 2022

Add logic to check for --keep-going support dtolnay/trybuild#159

Closed

Add logic to check for --keep-going support dtolnay/trybuild#160

Merged

rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Apr 3, 2022

ehuss added this to the 1.61.0 milestone Apr 7, 2022

dtolnay mentioned this pull request Aug 26, 2023

Consider whether --keep-going deserves a short flag (-k?) #12571

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unstable --keep-going flag #10383

Unstable --keep-going flag #10383

dtolnay commented Feb 12, 2022 •

edited

Loading

rust-highfive commented Feb 12, 2022

alexcrichton left a comment

alexcrichton Feb 16, 2022

alexcrichton Feb 16, 2022

alexcrichton commented Feb 16, 2022

rfcbot commented Feb 16, 2022 •

edited

Loading

fee1-dead commented Feb 25, 2022

alexcrichton commented Mar 8, 2022

bors commented Mar 21, 2022

ehuss commented Mar 21, 2022

dtolnay commented Mar 22, 2022

ehuss commented Mar 23, 2022

rfcbot commented Mar 23, 2022

ehuss commented Mar 23, 2022

rfcbot commented Mar 23, 2022 •

edited by weihanglo

Loading

Mark-Simulacrum commented Mar 30, 2022

Mark-Simulacrum commented Mar 30, 2022

bors commented Mar 30, 2022

Mark-Simulacrum commented Mar 30, 2022

bors commented Mar 30, 2022

bors commented Mar 30, 2022

bors commented Mar 30, 2022

Unstable --keep-going flag #10383

Unstable --keep-going flag #10383

Conversation

dtolnay commented Feb 12, 2022 • edited Loading

Summary

Prior art

Motivation

rust-highfive commented Feb 12, 2022

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Feb 16, 2022

Choose a reason for hiding this comment

alexcrichton Feb 16, 2022

Choose a reason for hiding this comment

alexcrichton commented Feb 16, 2022

rfcbot commented Feb 16, 2022 • edited Loading

fee1-dead commented Feb 25, 2022

alexcrichton commented Mar 8, 2022

bors commented Mar 21, 2022

ehuss commented Mar 21, 2022

dtolnay commented Mar 22, 2022

ehuss commented Mar 23, 2022

rfcbot commented Mar 23, 2022

ehuss commented Mar 23, 2022

rfcbot commented Mar 23, 2022 • edited by weihanglo Loading

Mark-Simulacrum commented Mar 30, 2022

Mark-Simulacrum commented Mar 30, 2022

bors commented Mar 30, 2022

Mark-Simulacrum commented Mar 30, 2022

bors commented Mar 30, 2022

bors commented Mar 30, 2022

bors commented Mar 30, 2022

dtolnay commented Feb 12, 2022 •

edited

Loading

rfcbot commented Feb 16, 2022 •

edited

Loading

rfcbot commented Mar 23, 2022 •

edited by weihanglo

Loading