chore: Cleanup cargo config queries #20178
Conversation
rustbot
added
the
S-waiting-on-review
ShoyuVanilla
force-pushed
the
cargo-config-cleanup
branch
from
cc66815 to
c9da218
Compare
| let config_file_ = config_file.clone(); | ||
| let toolchain_config = QueryConfig::Cargo(&sysroot, cargo_toml, &config_file_); |
There was a problem hiding this comment.
| let config_file_ = config_file.clone(); | |
| let toolchain_config = QueryConfig::Cargo(&sysroot, cargo_toml, &config_file_); | |
| let toolchain_config = QueryConfig::Cargo(&sysroot, cargo_toml, &config_file.clone()); |
does this not work? I'd assume this constructs a temporary
There was a problem hiding this comment.
Unfortunately, since this is not a function call but a tuple ctor, that doesn't work 😅
QueryConfig::Cargo {
0: &sysroot,
1: cargo_toml,
2: &config_file.clone(),
}might work but I think it's not desirable
| fn cargo_target_dir( | ||
| manifest: &ManifestPath, | ||
| extra_env: &FxHashMap<String, Option<String>>, | ||
| sysroot: &Sysroot, | ||
| ) -> Option<Utf8PathBuf> { | ||
| let cargo = sysroot.tool(Tool::Cargo, manifest.parent(), extra_env); | ||
| let mut meta = cargo_metadata::MetadataCommand::new(); | ||
| meta.cargo_path(cargo.get_program()); | ||
| meta.manifest_path(manifest); | ||
| // `--no-deps` doesn't (over)write lockfiles as it doesn't do any package resolve. | ||
| // So we can use it to get `target_directory` before copying lockfiles | ||
| let mut other_options = vec!["--no-deps".to_owned()]; | ||
| if manifest.is_rust_manifest() { | ||
| meta.env("RUSTC_BOOTSTRAP", "1"); | ||
| other_options.push("-Zscript".to_owned()); | ||
| } | ||
| meta.other_options(other_options); | ||
| meta.exec().map(|m| m.target_directory).ok() | ||
| } |
There was a problem hiding this comment.
We can be even smarter here. For one this code path will be exercised in the majority of cases. And note how we fallback to --no-deps when the command otherwise fails to semi support offline modes and the like. So I think it would make sense to just run this command proper (as we would usually) with --no-deps. If that succeeds, then run it again without while using the target dir info and stuff, and if this then fails, fallback to using the --no-deps version (this way we save one metadata invocation).
| .or_else(|| { | ||
| cargo_config_build_target_dir(project_json.manifest()?, &config.extra_env, &sysroot) | ||
| }) | ||
| .or_else(|| cargo_target_dir(project_json.manifest()?, &config.extra_env, &sysroot)) |
There was a problem hiding this comment.
I've done decreasing number of metadata fetches via the way you suggested but this still remains because we don't do any other metadata fetch in this function.
| @@ -563,51 +556,52 @@ impl ProjectWorkspace { | |||
| let target_dir = config | |||
| .target_dir | |||
| .clone() | |||
| .or_else(|| cargo_config_build_target_dir(detached_file, &config.extra_env, &sysroot)) | |||
| .or_else(|| cargo_target_dir(detached_file, &config.extra_env, &sysroot)) | |||
There was a problem hiding this comment.
This still remains though we have couple of metadata fetches inside this function but I think this would be more safer, because... - I'll continue on the comment below
| &|_| (), | ||
| ); | ||
| if let Some(loaded_sysroot) = loaded_sysroot { | ||
| sysroot.set_workspace(loaded_sysroot); | ||
| } | ||
|
|
||
| let cargo_script = CargoWorkspace::fetch_metadata( | ||
| let fetch_metadata = FetchMetadata::new( |
There was a problem hiding this comment.
continue from the above - ... This fetches with the sysroot that might mutated from the above sysroot.load_workspace(..) call with target_dir. So, hositing this to the above lines to pre-fetch metadata for target_dir and reusing it might be incorrect due to the possible mutation of sysroot
Resolves #20081 and fixes #20133 also fixes the case in this comment
While looking into
cargocodes, I found thatcargo metadata --no-depsdoes not touch lockfiles:So, we are safe to use
cargo metadata --no-depsto querytarget-dir