Tracking issue for --keep-going

[dtolnay]

Summary

Implementation: #10383
Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#keep-going

This flag makes Cargo build as many crates in the dependency graph as possible, rather than aborting the build at the first one that fails to build.

Unresolved Issues

• Add -k short flag? According to @joshtriplett: "similar to make -k; the nature of this option is such that it'd should be easy for people to invoke"
• Reimplement https://github.com/dtolnay/trybuild using this feature, since this was the original motivating use case. See "Motivation" section of Unstable --keep-going flag #10383 (comment).

Future Extensions

• Josh suggests: "we may want to extend this option in the future to have more kinds of "keep going" behavior, such as deferring some compile-time errors to runtime panics so that the test suite can run"

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

[ehuss]

Just a small question, I was wondering if there might be confusion on the difference between --no-fail-fast and --keep-going for cargo test. There seems to be a little overlap conceptually. I'm wondering how that compares to other tools like buck or bazel?

[dtolnay]

I'm wondering how that compares to other tools like buck or bazel?

@ehuss, at least for Buck, --no-fail-fast is the default behavior and there is no flag to invert it — that is not a concern of the build system, but of the infrastructure which runs the build system. In other words buck test … will always run all tests in the targets specified. There is also no separate build and run phase as in Cargo; each test becomes runnable as soon as its executable is done building, even if other tests are not built yet.

Instead of a --fail-fast setting, infrastructure is in charge of whether to kill the build. This needs to exist anyway because, for example when a "pull request" is amended by the author, your code review tool (Phabricator, Gerrit, etc) probably wants to kill any running buck test instances and start testing the new code. Buck produces a report that lets infrastructure find out about test results in real time as they run, so that individual unit test functions can be shown as passing or failing in Phabricator even while the rest of tests run. At least in my use case, there is never a case where you'd not want Phabricator to keep running all the rest of the tests after a failure: it costs effectively nothing, and fine-grained failure signals are already displayed in real time which is better than "fail fast". Other use cases might choose to kill the buck test job after seeing the first failed test result in the report, just the same as they'd kill a job that is obsoleted by an amended commit.

[dtolnay]

there might be confusion on the difference between --no-fail-fast and --keep-going for cargo test

For my own use case of --keep-going in https://github.com/dtolnay/trybuild, I am only interested in using it with cargo check, never cargo test, so it wouldn't matter to me whether it is a different flag from test's --no-fail-fast or not. We could just use the existing --no-fail-fast name to mean both "keep building after build failure" and "keep testing after test failure".

If some current users of --no-fail-fast would not want it to have "keep building after build failure" behavior, a harmonious pair of names could be --keep-building and --keep-testing.

[dtolnay]

I've just released version 1.0.60 of the trybuild crate which leverages --keep-going on toolchains which support it. Here is the difference that the new implementation makes on serde's test suite, which becomes 16x faster on my machine:

Before:

$ time cargo test --test compiletest

real	0m21.442s
user	0m15.288s
sys	0m6.317s

After:

$ time cargo test --test compiletest

real	0m1.290s
user	0m20.761s
sys	0m6.962s

[poliorcetics]

At $work we intend to use --keep-going to run clippy on the whole codebase while still using -- -D warnings in CI. I have not found any bug with it currently, we'll report if we do

EDIT: what is missing to move toward stabilization ?

[weihanglo]

what is missing to move toward stabilization ?

As it seems not pretty controversial, what we need is more feedback from users, so that we will be confident in this being useful in practice. Personally I'd like to see something like

• Use case people have and how --keep-going helps them.
• If --keep-going didn't pay well, how?
• Any extension people wish --keep-going could have.

Besides, there are some tiny concerns but not blockers:

• Imagine people run cargo test --keep-going --no-fail-fast. Doesn't look good. Should think about the education among the control between build graph, test suites (binaries), and test functions. (t-testing?)
• -k short alias. 100% sure we can do it later.

[chrisburel]

I use --keep-going when I am making changes to a workspace with lots of crates in it, and I know those changes are going to be breaking changes, and want the compiler to tell me quickly where all those places are.

For example, in my codebase, we deny clippy::correctness, but then selectively allow some 100+ lints from that group due to pre-existing code that triggers those lints. Over time we can remove individual lints from that allow list, and fix the resulting code. If there are lots of places that trigger a lint in many different crates, without --keep-going you can't get a list of all the places. You have to build some, fix some, build some, fix some, and repeat. It is a much nicer developer experience to build all, then fix all. For this, I could make an intermediate step of changing the lint to only warn, then build and collect the warnings, fix the warnings, then change the lint to deny. But it could be a breaking API change that you're working on, that would result in many actual compile errors.

My understanding of rust-analyzer is that it is based on cargo check, which is also affected by this. cargo check will stop processing further crates if it encounters a compile error earlier in the dependency graph. This means that rust-analyzer will not be able to give information to your IDE for crates at the end of the graph. Having rust-analyzer specify --keep-going seems to be best, so that it can report as much information as it is able to gather.

[ehuss]

The Cargo team briefly discussed moving this towards stabilization. One thing some of us were still uncertain about is the behavior of cargo test --keep-going, and the fact that it currently stops on the first test binary that fails. I would like to propose that cargo test --keep-going be changed to mean the same thing as cargo test --keep-going --no-fail-fast. I think that might help with keeping a consistent understanding that --keep-going means "do as much as you can" without needing to explain the subtle differences between "running rustc", "running a test binary", and "running a test within a test binary".

That change would make the use case of "build as much as possible, but stop on the first test binary that fails" more awkward. We couldn't think of that as a significant use case that we would expect people to have, but we were also uncertain. I think if people want something like that behavior, they can do something like cargo build --tests --keep-going && cargo test --no-fail-fast.

Does anyone have any objections to making that change?

[dtolnay]

Counterproposal: delete --keep-going support from cargo test?

The intended use cases for --keep-going definitely involve build/check/clippy, not test.

Deleting the flag leaves room for cargo test --keep-going to provide guidance in the direction of cargo test --no-fail-fast or cargo build --tests --keep-going && cargo test, rather than doing something the user might not have intended.

[ehuss]

That also sounds reasonable to me!

[weihanglo]

At the cargo meeting last week, we talked about --keep-going and agreed on the major blocker is the confusion of --keep-going and --no-fail-fast in cargo test, as described in #10496 (comment).

David proposed a nice solution in #10496 (comment): delete --keep-going support from cargo test/cargo bench. This has been implemented with #12478.

I propose to stabilize --keep-going in 1.74. I believe we have sufficient time for people to play with the last minute change, though we expect it is pretty rare.

For -k short alias, I'll create an issue for it after the FCP.

@rfcbot fcp merge

[rfcbot]

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

• @Eh2406
• @Muscraft
• @arlosi
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

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.

[epage]

I've recently been experimenting with --keep-going while contributing to gitoxide.

First, could someone clarify to me when we "keep going" today and when we don't? Many times I do a build and I see failures from a lot of different packages but at other times, I only see them from the first (which is why I experimented with --keep-going).

Similarly, I assume we don't "keep going" when the failure is just a lint and in theory the dependents could build. What should be the semantics in those cases and the messaging to be clear about this?

[dtolnay]

could someone clarify to me when we "keep going" today and when we don't?

One Cargo execution makes multiple executions of rustc, in general.

Some of these rustc executions need to be serial, for example if one has an input rlib/rmeta artifact that is the output of the other. Some rustc executions can be done in parallel because neither one produces artifacts that are a transitive dependency of the other. Some rustc executions that can happen in parallel, are done serially anyway, because there are not enough cores on the machine to serve the inherent parallelism in the build graph.

In general there is a directed acyclic graph of dependencies among the rustc executions in the build graph. Other build tools like Buck also have such a DAG.

When some rustc returns a failure, the rest of the currently running rustc processes continue to run, and may also produce other failures. After some rustc returns a failure, additional rustc subprocesses are not started by cargo, even if they would not need to depend on any outputs from the failed rustc.

Many times I do a build and I see failures from a lot of different packages but at other times, I only see them from the first

It depends on whether the 2 failing packages were buildable in parallel or not. This depends on whether they depend on one another, and also how the DAG is mapped to cores, with the latter being nondeterministic.

Similarly, I assume we don't "keep going" when the failure is just a lint and in theory the dependents could build

The dependents couldn't build because when a deny lint is triggered, an output artifact is not produced by rustc.

It may be reasonable to change Cargo to parse -D out of RUSTFLAGS / build.rustflags, transform it to -W, receive the JSON diagnostics as warnings from rustc, and transparently re-render them as errors while the build can continue.

It may also be reasonable to introduce a rustc flag that makes it produce artifacts despite any denied lints, although I anticipate there would be pushback against this.

There would be no problem making either of these changes in the future after initial stabilization of --keep-going.

[epage]

I'm a bit worried about how unclear the semantics are though improvements can be made later

• For make, they separate ignore-errors from keep-going, making it a superset
• A part of me would want to set a flag on the first error that prevents errors from other units from being reported unless keep-going isn't set to help make the behavior clearer. The big downside is the loss of information which I assume would weigh out over other benefits.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[dtolnay]

• Stabilization PR: Stabilize --keep-going #12568

[weihanglo]

Closing since this was stabilized in #12568.

Thanks everyone for your input on this 🎉🎉🎉

[dtolnay]

For -k short alias, I'll create an issue for it after the FCP.

#12571