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

Use --keep-going cargo flag when building build scripts #12984

Merged
merged 2 commits into from
Aug 10, 2022

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Aug 9, 2022


match toolchain {
Some(v) if v >= &RUST_1_62 => {
cmd.args(&["-Z", "unstable-options", "--keep-going"]).env("RUSTC_BOOTSTRAP", "1");
Copy link
Member

@bjorn3 bjorn3 Aug 9, 2022

Choose a reason for hiding this comment

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

I don't think we should use RUSTC_BOOTSTRAP. If you really need to prefer __CARGO_TEST_CHANNEL_OVERRIDE_DO_NOT_USE_THIS=nightly I think to only enable nightly features for cargo and not for rustc. Version detection build scripts might get confused by RUSTC_BOOTSTRAP. In addition this will need to check if --keep-going still exists and isn't for example removed.

Copy link
Member Author

@Veykril Veykril Aug 9, 2022

Choose a reason for hiding this comment

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

We already use RUSTC_BOOTSTRAP for a few different unstable cargo features we'd like to use. What would be the alternative, we can't rely on people running nightly toolchains so +nightly isn't an option. So same problems arise there already.

Though for the other cases we fallback to not using the unstable feature if it fails. I don't think we can easily do that here though since this command can fail for other reasons than the flag not existing.

Copy link
Member

Choose a reason for hiding this comment

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

In those cases we gracefully degrade if it fails with RUSTC_BOOTSTRAP. In this case it would completely break a core feature if --keep-going doesn't work anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ye, that is indeed a good point. Is there a simple way to detect if the command fails because of the flag not working? Retrying the non bootstrap command if hte bootstrap one fails sounds bad, given it can fail due to the buildscripts not working properly

Copy link
Member

Choose a reason for hiding this comment

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

It gives an error like

error: Found argument '--foo' which wasn't expected, or isn't valid in this context

        If you tried to supply `--foo` as a value rather than a flag, use `-- --foo`

USAGE:
    cargo check [OPTIONS]

For more information try --help

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess searching the error message is the best we can do here

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could just rerun once with --keep-going if the build fails? 🤔 Presumably it'd pick up where the previous build left off, and if the flag doesn't exist we'd be no worse off...

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could just rerun once with --keep-going if the build fails?

Wow, indeed!

@Veykril
Copy link
Member Author

Veykril commented Aug 10, 2022

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 10, 2022

📌 Commit 25d4fbe has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Aug 10, 2022

⌛ Testing commit 25d4fbe with merge b1e9bcd...

@bors
Copy link
Collaborator

bors commented Aug 10, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing b1e9bcd to master...

@bors bors merged commit b1e9bcd into rust-lang:master Aug 10, 2022
@Veykril Veykril deleted the keep-going branch October 28, 2022 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants