Skip to content

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

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

Merged
merged 2 commits into from
Aug 10, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
950de7c
Use `--keep-going` cargo flag when building build scripts
Veykril Aug 9, 2022
25d4fbe
Re-try build script building with --keep-going
Veykril Aug 10, 2022
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
26 changes: 25 additions & 1 deletion crates/project-model/src/build_scripts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use cargo_metadata::{camino::Utf8Path, Message};
use la_arena::ArenaMap;
use paths::AbsPathBuf;
use rustc_hash::FxHashMap;
use semver::Version;
use serde::Deserialize;

use crate::{cfg_flag::CfgFlag, CargoConfig, CargoWorkspace, Package};
Expand Down Expand Up @@ -77,9 +78,32 @@ impl WorkspaceBuildScripts {
config: &CargoConfig,
workspace: &CargoWorkspace,
progress: &dyn Fn(String),
toolchain: &Option<Version>,
) -> io::Result<WorkspaceBuildScripts> {
let mut cmd = Self::build_command(config);
const RUST_1_62: Version = Version::new(1, 62, 0);

match Self::run_(Self::build_command(config), config, workspace, progress) {
Ok(WorkspaceBuildScripts { error: Some(error), .. })
if toolchain.as_ref().map_or(false, |it| *it >= RUST_1_62) =>
{
// building build scripts failed, attempt to build with --keep-going so
// that we potentially get more build data
let mut cmd = Self::build_command(config);
cmd.args(&["-Z", "unstable-options", "--keep-going"]).env("RUSTC_BOOTSTRAP", "1");
Copy link
Member

@bjorn3 bjorn3 Aug 9, 2022 •
edited
Loading

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 •
edited
Loading

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!

let mut res = Self::run_(cmd, config, workspace, progress)?;
res.error = Some(error);
Ok(res)
}
res => res,
}
}

fn run_(
mut cmd: Command,
config: &CargoConfig,
workspace: &CargoWorkspace,
progress: &dyn Fn(String),
) -> io::Result<WorkspaceBuildScripts> {
if config.wrap_rustc_in_build_scripts {
// Setup RUSTC_WRAPPER to point to `rust-analyzer` binary itself. We use
// that to compile only proc macros and build scripts during the initial
Expand Down
1 change: 1 addition & 0 deletions crates/project-model/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ fn load_cargo_with_overrides(file: &str, cfg_overrides: CfgOverrides) -> CrateGr
rustc: None,
rustc_cfg: Vec::new(),
cfg_overrides,
toolchain: None,
};
to_crate_graph(project_workspace)
}
Expand Down
18 changes: 14 additions & 4 deletions crates/project-model/src/workspace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use base_db::{
use cfg::{CfgDiff, CfgOptions};
use paths::{AbsPath, AbsPathBuf};
use rustc_hash::{FxHashMap, FxHashSet};
use semver::Version;
use stdx::always;

use crate::{
Expand Down Expand Up @@ -77,6 +78,7 @@ pub enum ProjectWorkspace {
/// different target.
rustc_cfg: Vec<CfgFlag>,
cfg_overrides: CfgOverrides,
toolchain: Option<Version>,
},
/// Project workspace was manually specified using a `rust-project.json` file.
Json { project: ProjectJson, sysroot: Option<Sysroot>, rustc_cfg: Vec<CfgFlag> },
Expand Down Expand Up @@ -105,6 +107,7 @@ impl fmt::Debug for ProjectWorkspace {
rustc,
rustc_cfg,
cfg_overrides,
toolchain,
} => f
.debug_struct("Cargo")
.field("root", &cargo.workspace_root().file_name())
Expand All @@ -116,6 +119,7 @@ impl fmt::Debug for ProjectWorkspace {
)
.field("n_rustc_cfg", &rustc_cfg.len())
.field("n_cfg_overrides", &cfg_overrides.len())
.field("toolchain", &toolchain)
.finish(),
ProjectWorkspace::Json { project, sysroot, rustc_cfg } => {
let mut debug_struct = f.debug_struct("Json");
Expand Down Expand Up @@ -160,6 +164,9 @@ impl ProjectWorkspace {
cmd.arg("--version");
cmd
})?;
let toolchain = cargo_version
.get("cargo ".len()..)
.and_then(|it| Version::parse(it.split_whitespace().next()?).ok());

let meta = CargoWorkspace::fetch_metadata(
&cargo_toml,
Expand All @@ -169,9 +176,9 @@ impl ProjectWorkspace {
)
.with_context(|| {
format!(
"Failed to read Cargo metadata from Cargo.toml file {}, {}",
"Failed to read Cargo metadata from Cargo.toml file {}, {:?}",
cargo_toml.display(),
cargo_version
toolchain
)
})?;
let cargo = CargoWorkspace::new(meta);
Expand Down Expand Up @@ -219,6 +226,7 @@ impl ProjectWorkspace {
rustc,
rustc_cfg,
cfg_overrides,
toolchain,
}
}
};
Expand Down Expand Up @@ -271,8 +279,8 @@ impl ProjectWorkspace {
progress: &dyn Fn(String),
) -> Result<WorkspaceBuildScripts> {
match self {
ProjectWorkspace::Cargo { cargo, .. } => {
WorkspaceBuildScripts::run(config, cargo, progress).with_context(|| {
ProjectWorkspace::Cargo { cargo, toolchain, .. } => {
WorkspaceBuildScripts::run(config, cargo, progress, toolchain).with_context(|| {
format!("Failed to run build scripts for {}", &cargo.workspace_root().display())
})
}
Expand Down Expand Up @@ -320,6 +328,7 @@ impl ProjectWorkspace {
rustc_cfg: _,
cfg_overrides: _,
build_scripts,
toolchain: _,
} => {
cargo
.packages()
Expand Down Expand Up @@ -425,6 +434,7 @@ impl ProjectWorkspace {
rustc_cfg,
cfg_overrides,
build_scripts,
toolchain: _,
} => cargo_to_crate_graph(
rustc_cfg.clone(),
cfg_overrides,
Expand Down
1 change: 1 addition & 0 deletions crates/rust-analyzer/src/reload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ impl GlobalState {
cfg_overrides,

build_scripts: _,
toolchain: _,
} => Some((cargo, sysroot, rustc, rustc_cfg, cfg_overrides)),
_ => None,
};
Expand Down