Simplify `ProcessBuilder` modification #456

Xanewok · 2017-08-22T15:30:49Z

As with #449, this depends on rust-lang/cargo#4424.
Will change Cargo.toml back when the PR is merged.

Xanewok · 2017-08-22T15:32:18Z

src/build/cargo.rs

-                // 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" {


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

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

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?

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).

Xanewok · 2017-08-22T15:33:37Z

src/build/cargo.rs

-        let rustc_exe = env::var("RUSTC").unwrap_or(env::args().next().unwrap());
-        let mut cmd = Command::new(&rustc_exe);
+        // Cargo should handle `$RUSTC` wrapper override already
+        let mut cmd = cargo_cmd.clone();


This will use the same program that's inside cargo_cmd, is it correct wrt shim (and env::args().next())?

No, so for shim, we want to call the RLS itself, which acts as a shim (sorry, I should really have documented that). We have to avoid using the shim in favour of $RUSTC at least for tests (both in our repo and in the Rust repo, where there is a different shim).

So on top of that, we'd still need some API for ProcessBuilder to change the program, right? And then we'd need to substitute with previous rustc_exe?

@nrc apart from other fixes needed, I guess this is also blocked on new ProcessBuilder api to set a program?

Xanewok · 2017-08-22T15:34:31Z

src/build/cargo.rs

            }
+            return cmd.exec();


In both cases I'm running the command with cmd.env(::RUSTC_SHIM_ENV_VAR_NAME, "1"); applied, will this caues any problems?

It is necessary - that env var causes the RLS to operate as a rustc shim

Previously, if the crate was blacklisted, a command with cmd.env(::RUSTC_SHIM_ENV_VAR_NAME, "1"); was run, and if the crate wasn't blacklisted, the original cargo_cmd (without cmd.env(::RUSTC_SHIM_ENV_VAR_NAME, "1");) was run.
My change uses the one with the shim env var name set in both cases.
From what I understand, being on a blacklist should only change the save analysis argument and not change the process that's run, right?

From what I understand, being on a blacklist should only change the save analysis argument and not change the process that's run, right?

Yeah, correct.

Calling the shim is the same as calling rustc -Zsave-analysis. If you're running rustc, the shim env var doesn't make difference either way, but if you're running the shim (which we must if we want to collect data), then it must be set

TODO: Don't use shim when the crate is blacklisted - we don't need to, because we won't pass -Zsave-analysis (or any other unstable option?)

Xanewok · 2017-08-22T15:38:13Z

src/build/cargo.rs

@@ -405,7 +389,7 @@ impl Executor for RlsExecutor {
                _ => {}
            }
        } else {
-            cmd.status().expect("Couldn't execute rustc");
+            cmd.exec()?;


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))?

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

Xanewok · 2017-08-22T18:56:09Z

The Cargo PR landed already, reverted Cargo.toml to Cargo master and force-pushed.

nrc · 2017-08-22T21:30:54Z

src/build/cargo.rs

-        let rustc_exe = env::var("RUSTC").unwrap_or(env::args().next().unwrap());
-        let mut cmd = Command::new(&rustc_exe);
+        // Cargo should handle `$RUSTC` wrapper override already
+        let mut cmd = cargo_cmd.clone();


No, so for shim, we want to call the RLS itself, which acts as a shim (sorry, I should really have documented that). We have to avoid using the shim in favour of $RUSTC at least for tests (both in our repo and in the Rust repo, where there is a different shim).

nrc · 2017-08-22T21:31:43Z

src/build/cargo.rs

            }
+            return cmd.exec();


It is necessary - that env var causes the RLS to operate as a rustc shim

nrc · 2017-08-22T21:33:43Z

src/build/cargo.rs

-                return match cmd.status() {
-                    Ok(ref e) if e.success() => Ok(()),
-                    _ => Err(CargoErrorKind::ProcessErrorKind(process_error(
-                                "process didn't exit successfully", None, None)).into()),


Do we not need the env vars and current dir?

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

nrc · 2017-08-22T21:34:39Z

src/build/cargo.rs

-                // 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" {


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

Xanewok · 2017-08-27T17:01:17Z

We need to substitute the program for the shim, so that's still blocked on the Cargo side: rust-lang/cargo#4442.
Until then, it uses my branch of Cargo.

Xanewok · 2017-08-27T17:02:51Z

src/build/cargo.rs

@@ -296,37 +295,35 @@ 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:


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?

No, looks like a good comment!

Xanewok · 2017-08-28T12:27:46Z

Depends on #466 for compilation fix.

Xanewok · 2017-08-28T16:31:57Z

Updated cargo dep as it needs rust-lang/cargo@814c389, force-pushed.

Xanewok · 2017-08-29T07:44:39Z

src/build/cargo.rs

        if !self.is_primary_crate(id) || is_build_script {
-            let mut cmd = Command::new(&env::var("RUSTC").unwrap_or("rustc".to_owned()));


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

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

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).

nrc · 2017-08-31T05:14:54Z

Thanks!

Xanewok commented Aug 22, 2017

View reviewed changes

Xanewok force-pushed the simplify-process-builder branch from 217396d to 6d7ca16 Compare August 22, 2017 18:54

nrc reviewed Aug 22, 2017

View reviewed changes

Xanewok force-pushed the simplify-process-builder branch 2 times, most recently from 7aeb4b2 to 24a7e1c Compare August 27, 2017 16:45

Xanewok commented Aug 27, 2017

View reviewed changes

Xanewok closed this Aug 27, 2017

Xanewok reopened this Aug 27, 2017

Xanewok force-pushed the simplify-process-builder branch 2 times, most recently from 7c29aff to 4ceae4d Compare August 28, 2017 12:27

Xanewok force-pushed the simplify-process-builder branch from 4ceae4d to 60a2e87 Compare August 28, 2017 16:31

Xanewok force-pushed the simplify-process-builder branch 4 times, most recently from cf8223e to 68efd54 Compare August 29, 2017 07:43

Xanewok commented Aug 29, 2017

View reviewed changes

Xanewok added 2 commits August 30, 2017 20:02

Simplify ProcessBuilder modification

2d896cd

Run original compiler command for blacklisted crates

68ffac0

Xanewok force-pushed the simplify-process-builder branch from 68efd54 to 68ffac0 Compare August 30, 2017 18:10

nrc merged commit 7f87fb4 into rust-lang:master Aug 31, 2017

Xanewok deleted the simplify-process-builder branch September 24, 2017 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify `ProcessBuilder` modification #456

Simplify `ProcessBuilder` modification #456

Xanewok commented Aug 22, 2017 •

edited

Loading

Xanewok Aug 22, 2017

nrc Aug 22, 2017

Xanewok Aug 22, 2017

nrc Aug 28, 2017

Xanewok Aug 22, 2017

nrc Aug 22, 2017

Xanewok Aug 22, 2017

Xanewok Aug 23, 2017

Xanewok Aug 22, 2017

nrc Aug 22, 2017

Xanewok Aug 22, 2017

nrc Aug 28, 2017

Xanewok Aug 28, 2017

Xanewok Aug 22, 2017

Xanewok Aug 22, 2017 •

edited

Loading

Xanewok commented Aug 22, 2017

nrc Aug 22, 2017

nrc Aug 22, 2017

nrc Aug 22, 2017

Xanewok Aug 22, 2017

nrc Aug 22, 2017

Xanewok commented Aug 27, 2017

Xanewok Aug 27, 2017

nrc Aug 28, 2017

Xanewok commented Aug 28, 2017

Xanewok commented Aug 28, 2017

Xanewok Aug 29, 2017 •

edited

Loading

nrc Aug 31, 2017

nrc Aug 31, 2017

nrc commented Aug 31, 2017

		if !self.is_primary_crate(id) \|\| is_build_script {
		let mut cmd = Command::new(&env::var("RUSTC").unwrap_or("rustc".to_owned()));

Simplify ProcessBuilder modification #456

Simplify ProcessBuilder modification #456

Conversation

Xanewok commented Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xanewok Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Xanewok commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xanewok commented Aug 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xanewok commented Aug 28, 2017

Xanewok commented Aug 28, 2017

Xanewok Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Aug 31, 2017

Simplify `ProcessBuilder` modification #456

Simplify `ProcessBuilder` modification #456

Xanewok commented Aug 22, 2017 •

edited

Loading

Xanewok Aug 22, 2017 •

edited

Loading

Xanewok Aug 29, 2017 •

edited

Loading