Skip to content
This repository was archived by the owner on Dec 29, 2022. It is now read-only.

Simplify ProcessBuilder modification #456

Merged
merged 2 commits into from
Aug 31, 2017
Merged

Simplify ProcessBuilder modification #456

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d896cd
Simplify `ProcessBuilder` modification
Xanewok Aug 22, 2017
68ffac0
Run original compiler command for blacklisted crates
Xanewok Aug 28, 2017
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
94 changes: 41 additions & 53 deletions src/build/cargo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,9 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use cargo::core::{PackageId, Shell, Target, Workspace, Verbosity};
use cargo::core::{PackageId, Shell, Target, TargetKind, Workspace, Verbosity};
use cargo::ops::{compile_with_exec, Executor, Context, Packages, CompileOptions, CompileMode, CompileFilter, Unit};
use cargo::util::{Config as CargoConfig, process_builder, ProcessBuilder, homedir, important_paths, ConfigValue, CargoResult};
use cargo::util::errors::{CargoErrorKind, process_error};
use cargo::util::{Config as CargoConfig, ProcessBuilder, homedir, important_paths, ConfigValue, CargoResult};
use serde_json;

use data::Analysis;
Expand Down Expand Up @@ -296,38 +295,42 @@ impl Executor for RlsExecutor {
}
}

let rustc_exe = env::var("RUSTC").unwrap_or(env::args().next().unwrap());
let mut cmd = Command::new(&rustc_exe);
// Prepare our own call to `rustc` as follows:
Copy link
Member Author

Choose a reason for hiding this comment

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

Am I commenting too much? Wanted to draw a high level plan, mostly why we need to use the rls executable, so it made sense to present more high-level overview of what'll be done, but the later parts are also commented. Should I only retain a comment regarding the shim and why it's needed?

Copy link
Member

Choose a reason for hiding this comment

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

No, looks like a good comment!

// 1. Use $RUSTC wrapper if specified, otherwise use RLS executable
// as an rustc shim (needed to distribute via the stable channel)
// 2. For non-primary packages or build scripts, execute the call
// 3. Otherwise, we'll want to use the compilation to drive the analysis:
// i. Modify arguments to account for the RLS settings (e.g.
// compiling under cfg(test) mode or passing a custom sysroot)
// ii. Execute the call and store the final args/envs to be used for
// later in-process execution of the compiler
let mut cmd = cargo_cmd.clone();
let rls_executable = env::args().next().unwrap();
cmd.program(env::var("RUSTC").unwrap_or(rls_executable));
cmd.env(::RUSTC_SHIM_ENV_VAR_NAME, "1");

// We only want to intercept rustc call targeting current crate to cache
// args/envs generated by cargo so we can run only rustc later ourselves
// Currently we don't cache nor modify build script args
let is_build_script = crate_name == "build_script_build";
let is_build_script = *target.kind() == TargetKind::CustomBuild;
if !self.is_primary_crate(id) || is_build_script {
let mut cmd = Command::new(&env::var("RUSTC").unwrap_or("rustc".to_owned()));
Copy link
Member Author

@Xanewok Xanewok Aug 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@nrc we should still run shim for non-blacklisted crates that are deps/build scripts, right?
Running it here

Copy link
Member

Choose a reason for hiding this comment

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

Yes, any crate we want save-analysis from we must use the shim

Copy link
Member

Choose a reason for hiding this comment

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

We should, but not for black-listed crates. Note that currently we only use cmd for blacklisted crates, so this is not wrong atm (but it should really be inside the next if).

let build_script_notice = if is_build_script {" (build script)"} else {""};
trace!("rustc not intercepted - {}{}", id.name(), build_script_notice);

// Recreate the original command, minus -Zsave-analysis. Since the
// shim sets it internally, be sure not to use it.
if ::CRATE_BLACKLIST.contains(&&*crate_name) {
// Recreate the command, minus -Zsave-analysis.
for a in cargo_args {
if a != "-Zsave-analysis" {
cmd.arg(a);
}
}
cmd.envs(cargo_cmd.get_envs().iter().filter_map(|(k, v)| v.as_ref().map(|v| (k, v))));
if let Some(cwd) = cargo_cmd.get_cwd() {
cmd.current_dir(cwd);
}
return match cmd.status() {
Ok(ref e) if e.success() => Ok(()),
_ => Err(CargoErrorKind::ProcessErrorKind(process_error(
"process didn't exit successfully", None, None)).into()),
Copy link
Member

Choose a reason for hiding this comment

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

Do we not need the env vars and current dir?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do - cmd is the clone of the exec argument, cargo_cmd, so it has it already

};
} else {
let mut cargo_cmd = cargo_cmd.clone();
let args: Vec<_> = cargo_cmd.get_args().iter().cloned()
.filter(|x| x != "Zsave-analysis").collect();
cargo_cmd.args_replace(&args);

return cargo_cmd.exec();
}
// TODO: Make sure we don't pass any unstable options (incl. -Zsave-analysis)
// to the shim. For stable toolchains it won't accept those as arguments,
// but rather it sets them internally instead to work around that
return cmd.exec();
}

trace!("rustc intercepted - args: {:?} envs: {:?}", cargo_args, cargo_cmd.get_envs());
Expand Down Expand Up @@ -370,43 +373,28 @@ impl Executor for RlsExecutor {
// so the dep-info is ready by the time we return from this callback.
// NB: In `workspace_mode` regular compilation is performed here (and we don't
// only calculate dep-info) so it should fix the problem mentioned above.
// Since ProcessBuilder doesn't allow to modify args, we need to create
// our own command here from scratch here.
for a in &args {
// Emitting only dep-info is possible only for final crate type, as
// as others may emit required metadata for dependent crate types
if a.starts_with("--emit") && is_final_crate_type && !self.workspace_mode {
cmd.arg("--emit=dep-info");
} else if a != "-Zsave-analysis" {
Copy link
Member Author

Choose a reason for hiding this comment

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

Does this mean we don't ever pass "-Zsave-analysis" to the deps?

Copy link
Member

Choose a reason for hiding this comment

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

No, we pass it to the deps, but when we save the args for our own rustc call, then we don't use -Zsave-analysis

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, that's for the primary crate units, my bad. But does that mean that when compiling lib for the final bin target, we don't generate save-analysis as well? Shouldn't we? Or am I missing something still?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, to be clear, when running on the deps, we use the RLS as a rustc shim which effectively always has -Zsave-analysis set. So we never explicitly set -Zsave-analysis - for black-listed deps, we don't want it, for non-black-listed-deps we use the RLS as a shim, and for the primary crates we call rustc directly (where again collecting save-analysis data does not require the flag).

cmd.arg(a);
}
}
cmd.envs(cargo_cmd.get_envs().iter().filter_map(|(k, v)| v.as_ref().map(|v| (k, v))));
if let Some(cwd) = cargo_cmd.get_cwd() {
cmd.current_dir(cwd);
}
let modified = args.iter()
.filter(|a| *a != "-Zsave-analysis")
.map(|a| {
// Emitting only dep-info is possible only for final crate type, as
// as others may emit required metadata for dependent crate types
if a.starts_with("--emit") && is_final_crate_type && !self.workspace_mode {
"--emit=dep-info"
} else { a }
})
.collect::<Vec<_>>();
cmd.args_replace(&modified);
}

// Cache executed command for the build plan
{
let mut cx = self.compilation_cx.lock().unwrap();

let mut store_cmd = process_builder::process(&rustc_exe);
store_cmd.args(&args.iter().map(OsString::from).collect::<Vec<_>>());
for (k, v) in cargo_cmd.get_envs().clone() {
if let Some(v) = v {
store_cmd.env(&k, v);
}
}
if let Some(cwd) = cargo_cmd.get_cwd() {
cmd.current_dir(cwd);
}

cx.build_plan.cache_compiler_job(id, target, &store_cmd);
cx.build_plan.cache_compiler_job(id, target, &cmd);
}

// Prepare modified cargo-generated args/envs for future rustc calls
args.insert(0, rustc_exe);
let rustc = cargo_cmd.get_program().to_owned().into_string().unwrap();
args.insert(0, rustc);
let envs = cargo_cmd.get_envs().clone();

if self.workspace_mode {
Expand All @@ -426,7 +414,7 @@ impl Executor for RlsExecutor {
_ => {}
}
} else {
cmd.status().expect("Couldn't execute rustc");
cmd.exec()?;
Copy link
Member Author

Choose a reason for hiding this comment

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

This changes the semantics, I believe - it will still error out when the process can't be run, but also when the exit code is non 0 (e.g. dep can't be compiled). Maybe this will somehow prevent the spontaneous fingerprint crashes (as per my #443 (comment))?

Copy link
Member Author

@Xanewok Xanewok Aug 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Just as a note, it does roughly the same what this does, minus returning from the function on success - we still store the cached args/envs at the end of the function

}

// Finally, store the modified cargo-generated args/envs for future rustc calls
Expand Down