Sccache support for building Rust on Windows #294

aidanhs · 2018-09-13T03:20:22Z

The RlibDepDecoder is the most important piece here - in short, you can't distribute dylibs cross-platform (without something like Wine), so we either need to be pessimistic (and disallow distributed compilation if there's every a dylib anywhere in the deps folder, which is almost certain after first compilation) or have some way of telling what is a dependency of a particular crate rlib (we know the direct rlib dependencies of the thing we're compiling as they're passed in explicitly, we're interested in going a step down). -Z ls does this.

As a nice side effect, it makes other compiles more efficient too as it reduces what you send over the wire.

I did some informal measurements distributing over Wireless G from Windows 4core i3 non-SSD laptop to an 8core i7 SSD laptop: Firefox clobber build went from ~100mins to ~55mins. YMMV.

Unfortunately, while I was playing I saw an oddity with generated .d files - it looks like remapping prefixes is perhaps not working in some situations. The firefox build works, so I need to look at the implications.

luser · 2018-09-17T17:01:38Z

src/commands.rs

+        }
+        #[cfg(not(feature = "dist-client"))]
+        Command::PackageToolchain(_executable, _out) => {
+            bail!("Toolchain packaging not compiled in")


This might be slightly better if it mentioned that you need to build the dist-client feature.

aidanhs · 2018-09-21T01:03:20Z

src/compiler/rust.rs

@@ -595,6 +686,7 @@ static ARGS: [ArgInfo<ArgData>; 33] = [
    take_arg!("--out-dir", PathBuf, CanBeSeparated('='), OutDir),
    take_arg!("--pretty", OsString, CanBeSeparated('='), NotCompilation),
    take_arg!("--print", OsString, CanBeSeparated('='), NotCompilation),
+    take_arg!("--remap-path-prefix", OsString, CanBeSeparated('='), TooHard),


Remapping path prefixes was already a failure case (since it would read incorrect things from the dep file), this addition just makes it graceful.

We should probably figure out how to deal with this longer-term but it's probably fine for now. Was it harmful in the non-distributed case, or only in the distributed case?

aidanhs · 2018-09-21T01:07:32Z

Fixed the dep file generation - essentially, remapping path prefixes only applies to inputs, but some of the paths in a .d file are outputs so aren't mapped.

I hadn't noticed it before because nothing actually uses the created .d files. The chosen solution does mean that the .d files retrieved from remote compilation use forward slashes (i.e. C:/a/b rather than C:\a\b) but the Windows APIs accept either so most reasonable apps should be fine.

luser · 2018-10-05T17:50:35Z

src/commands.rs

+            let packager = compiler.map(|c| c.get_toolchain_packager());
+            let res = packager.and_then(|p| p.write_pkg(out_file));
+            core.run(res)?
+        }


As a followup improvement, I think it would be nice if this command printed a status message when it started (since it could presumably take a little bit of time to write the package) and also printed the toolchain hash afterwards, for ease of configuring cross-compiling clients.

We also might want to audit potential error messages in the methods in use here, since most of them were only used internally in the server previously and this exposes them directly to command-line usage.

luser · 2018-10-05T18:08:00Z

src/compiler/gcc.rs

@@ -402,6 +402,9 @@ pub fn generate_compile_commands(path_transformer: &mut dist::PathTransformer,
                                env_vars: &[(OsString, OsString)])
                                -> Result<(CompileCommand, Option<dist::CompileCommand>, Cacheable)>
 {
+    #[cfg(not(feature = "dist-client"))]
+    let _ = path_transformer;


I don't love statement-level #[cfg] usage like this, but I don't know that there's a better pattern. Maybe using cfg-if would make it nicer?

luser · 2018-10-05T18:13:06Z

src/compiler/rust.rs

 use tempdir::TempDir;
 use util::{fmt_duration_as_secs, run_input_output, Digest};
 use util::{HashToDigest, OsStrExt, ref_env};

 use errors::*;

+/// Can dylibs (like proc macros) be distributed on this platform?
+#[cfg(all(feature = "dist-client", target_os = "linux", target_arch = "x86_64"))]


Are proc macro shared libraries portable enough that you can run them on different Linux systems? I guess we'll find out in practice once more people start using this code.

luser · 2018-10-05T18:13:50Z

src/compiler/rust.rs

+
+#[cfg(feature = "dist-client")]
+const RLIB_PREFIX: &str = "lib";
+#[cfg(feature = "dist-client")]


consts like these would probably be fine with just #[allow(unused)], but it's not a big deal.

luser · 2018-10-05T18:24:38Z

src/compiler/rust.rs

+            let rlib_dep_reader = match rlib_dep_reader {
+                Ok(r) => Some(Arc::new(r)),
+                Err(e) => {
+                    warn!("Failed to initialise RlibDepDecoder, distributed compiles will be inefficient: {}", e);


We're going to need to figure out a way to surface these sorts of things to the client at some point.

luser · 2018-10-05T18:28:46Z

src/compiler/rust.rs

-        }))
+        }));
+
+        #[cfg(not(feature = "dist-client"))]


I wonder if this would be more readable if you created a do-nothing future like f_ok(None) for the #[cfg(not(feature = "dist-client"))] branch, and then joined into the same code from either version?

luser · 2018-10-05T18:31:17Z

src/compiler/rust.rs

@@ -927,10 +1021,13 @@ impl<T> CompilerHasher<T> for RustHasher
                        (o, p)
                    })
                    .collect::<HashMap<_, _>>();
-                if let Some(dep_info) = dep_info {
+                let dep_info = if let Some(dep_info) = dep_info {


This feels like it'd be better written as dep_info.map(|dep_info| ...).

luser · 2018-10-05T18:34:58Z

src/compiler/rust.rs

+                }
+            }
+            // OUT_DIR was changed during transformation, check if this compilation is relying on anything
+            // inside it - if so, disallow distributed compilation (there are sometimes hardcoded paths present)


This would happen for things like "my build script compiled some C code" or "my build script generated Rust code", right? Is it too hard to ship those additional files across the wire in those cases, or are there other cases I'm missing?

luser · 2018-10-05T18:40:27Z

src/compiler/rust.rs

+
+// TODO: we do end up with slashes facing the wrong way, but Windows is agnostic so it's
+// mostly ok. We currently don't get mappings for every single path because it means we need to
+// figure out all prefixes and send them over the wire.


I think this is OK, sccache really only strives to handle the common cases of rustc invoked by cargo, and it seems unlikely that files will wind up on different drives there.

luser · 2018-10-05T18:43:21Z

src/compiler/rust.rs

+        // have been passed on the command line, allowing us to scan them all and find the
+        // complete list of crates we might need.
+        // If it's not a cargo build, we can't to extract the `extern crate` statements and
+        // so have no way to build a list of necessary crates - send all rlibs.


It's nice that you have fallback support here, but it would have been fine to just not support distributed compilation for the non-cargo case, honestly.

luser · 2018-10-05T18:45:24Z

src/compiler/rust.rs

+            for (_dep_info_dist_path, dep_info_local_path) in output_paths {
+                trace!("Comparing with {}", dep_info_local_path.display());
+                if dep_info == dep_info_local_path {
+                    error!("Replacing using the transformer {:?}", path_transformer);


This doesn't seem like an error.

luser · 2018-10-05T18:48:01Z

src/compiler/rust.rs

+
+        // TODO: unfortunately there is exactly nothing you can do with the k given the
+        // current trait bounds. Just use some kind of sane value;
+        //let k_size = mem::size_of::<PathBuf>() + k.capacity();


I think this was mostly to account for the memory usage case where you would want to measure both the keys and values. If you only care about the values this is fine.

luser · 2018-10-05T18:49:07Z

src/compiler/rust.rs

+        let cache = lru_cache::LruCache::with_meter(CACHE_SIZE, DepsSize);
+
+        let rlib_dep_reader = RlibDepReader { cache: Mutex::new(cache), executable };
+        if let Err(e) = rlib_dep_reader.discover_rlib_deps(env_vars, &temp_rlib) {


Is this just a sanity check to ensure that -Z ls works at all with the current rustc?

luser · 2018-10-05T18:57:10Z

src/compiler/rust.rs

+        cmd.args(&["-Z", "ls"]).arg(&rlib)
+            .env_clear()
+            .envs(ref_env(env_vars))
+            .env("RUSTC_BOOTSTRAP", "1"); // TODO: this is fairly naughty


Is this something we expect will ever become stable? If not it's probably a necessary evil, but it's a pretty minor thing.

luser · 2018-10-05T18:58:29Z

src/compiler/rust.rs

+
+        let process::Output { status, stdout, stderr } = cmd.output()?;
+
+        if !status.success() {


We do have run_input_output which handles a few of these things for you, but it returns a Future so you'd have to refactor this a bit:

sccache/src/util.rs

Line 142 in dda88a6

pub fn run_input_output<C>(mut command: C, input: Option<Vec<u8>>)

luser · 2018-10-05T19:10:32Z

src/compiler/rust.rs

+        assert!(line_splits.next().is_none());
+
+        let mut libstring_splits = libstring.rsplitn(2, '-');
+        // Rustc prints strict hash value (rather than extra filename as it likely should be)


You filed this upstream, right? If so it might be good to link the issue in this comment.

luser · 2018-10-05T19:11:36Z

Obviously a bunch of minor comments there but nothing that felt serious enough to block merging.

aidanhs added 9 commits September 13, 2018 04:01

Remove old comments

96cab2a

Make sure to always specify a target triple

7391965

Move pathtransformers into the inputs creator

1b007dc

Minimal rlib dep inspector

a07ea6c

Cache rlib dependency lookups

0f55839

Convert paths of some environment variables

240129d

Permit disabling toolchains

ea39094

Expose direct access to toolchain packagers

85c6732

Perform path remapping when compiling from Windows

7acbeea

luser reviewed Sep 17, 2018

View reviewed changes

luser mentioned this pull request Sep 19, 2018

Use the counted_array macro for the ARGS arrays #298

Merged

Rewrite .d files with correct paths after receiving them

433782f

aidanhs force-pushed the aphs-dist-sccache-rust-windows branch from ccc48d9 to 433782f Compare September 21, 2018 00:59

aidanhs commented Sep 21, 2018

View reviewed changes

luser reviewed Oct 5, 2018

View reviewed changes

luser merged commit 5d4d6d3 into mozilla:master Oct 5, 2018

aidanhs mentioned this pull request Nov 21, 2018

Add tests, remove unwraps, fix bugs, auto https #334

Merged


		let process::Output { status, stdout, stderr } = cmd.output()?;

		if !status.success() {

Sccache support for building Rust on Windows #294

Sccache support for building Rust on Windows #294

Uh oh!

Conversation

aidanhs commented Sep 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aidanhs commented Sep 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luser commented Oct 5, 2018

Uh oh!

Uh oh!