Support distributed cross-compilation on Windows by aidanhs · Pull Request #271 · mozilla/sccache

aidanhs · 2018-08-06T17:27:38Z

For review after #249

aidanhs · 2018-08-06T17:37:24Z

src/compiler/msvc.rs

+            }
+        }
+    }
+


For the purposes of distributed compilation it's not sufficient to just 'pass through' any -Xclang options as suggested in #262 - it's important to filter out the preprocessor-only args, which the dependency stuff falls under.

I'm not sure what the best solution long-term is - by the time you've created a clang (or rather, gcc) ParseArguments you've lost a lot of information (e.g. "there is another input file" turns into "here's an arg") which reduces our accuracy. I think you almost want an interim LosslessParsedArgs so you can inspect it e.g. within this function to see how it affects the msvc compilation.

For now it just grabs the pieces that can be easily partitioned into common or preprocessor args without any side effects and bails otherwise.

This seems like a reasonable way to handle things for now. It might be worth refactoring things in the future so that we could use something less leaky like parse_arguments, but I won't lose any sleep over it. clang-cl is an odd duck, and dealing with -Xclang is even odder. I'd probably want to read the clang-cl source to figure things out before anyone spent any more time on this.

aidanhs · 2018-08-06T17:38:35Z

src/compiler/msvc.rs

        depfile: depfile.map(|d| d.into()),
        outputs: outputs,
-        preprocessor_args: vec!(),
+        preprocessor_args: preprocessor_args,


I was a little unclear why all of these used to go in common args - it's pessimistic so wouldn't be a problem, and I couldn't see anything in the git history indicating why it's like this.

I couldn't figure this out either, TBH. This is definitely better, though.

aidanhs · 2018-08-06T18:02:30Z

src/compiler/clang.rs

-    take_arg!("-Xclang", String, Separated, PassThrough),
+    // TODO: should be extracted and reprocessed, though bear in mind some
+    // flags are not valid under a -Xclang
+    take_arg!("-Xclang", String, Separated, TooHard),


It felt a bit dubious to me to just pass these through blindly, though there is one test below that depended on it working. This isn't necessary for this PR, but I stumbled across it and thought I might tweak it, particularly given the work that msvc.rs now does indicates there could be all kinds of things hidden in here.

I didn't realize this was even here, honestly. I don't think I've ever seen this used outside of clang-cl.

aidanhs · 2018-08-16T20:14:55Z

src/config.rs

+    pub archive: PathBuf,
+    pub archive_compiler_executable: String,
+}
+


This is the way you specify a cross compiler toolchain - the compiler executable you want to override, an archive and a path within the archive you use to override the path of the compiler executable.

OK, we can see how people like this in practice. I don't suppose it's worse than the current way people create cross-toolchains for icecream.

aidanhs · 2018-08-16T20:15:41Z

Oh, I hadn't realised this needed a rebase - odd, I was already rebasing on top of the last PR.

luser · 2018-08-23T16:52:18Z

src/bin/sccache-dist/main.rs

 enum Void {}

+// Only supported on x86_64 Linux machines
+#[cfg(all(target_os = "linux", target_arch = "x86_64"))]


Is there a way to control this at the cargo level so that we don't build it on other platforms?

luser · 2018-08-23T18:44:07Z

src/compiler/c.rs

-    fn into_inputs_creator(self: Box<Self>) -> Result<Box<FnMut(&mut io::Write)>> {
+    fn into_dist_inputs_creator(self: Box<Self>, path_transformer: &mut dist::PathTransformer) -> Result<Box<FnMut(&mut io::Write)>> {
+        let input_path = self.cwd.join(&self.parsed_args.input);
+        let dist_input_path = path_transformer.to_dist(&input_path).unwrap();


Any particular reason this has an unwrap?

luser · 2018-08-23T18:44:39Z

src/compiler/c.rs

+        let input_path = self.cwd.join(&self.parsed_args.input);
+        let dist_input_path = path_transformer.to_dist(&input_path).unwrap();
+        // tar-rs imposes that `set_path` takes a relative path
+        assert!(dist_input_path.starts_with("/"));


These shouldn't be actual assert!s, should they? Those will take down the whole server.

luser · 2018-08-23T18:46:04Z

src/compiler/c.rs

+        info!("Packaging C compiler");
+        // TODO: write our own, since this is GPL
+        let curdir = env::current_dir().unwrap();
+        env::set_current_dir("/tmp").unwrap();


Wouldn't it be cleaner to set the cwd of the Command instead?

luser · 2018-08-23T18:49:49Z

src/compiler/compiler.rs

-    let (compile_cmd, _dist_compile_cmd, cacheable) = compilation.generate_compile_commands().unwrap();
+
+    let mut path_transformer = dist::PathTransformer::new();
+    let (compile_cmd, _dist_compile_cmd, cacheable) = compilation.generate_compile_commands(&mut path_transformer).unwrap();


I missed this previously, but this should propagate errors instead of using unwrap.

luser · 2018-08-23T18:51:22Z

src/compiler/compiler.rs

    let compile_out_pretty = out_pretty.clone();
    let compile_out_pretty2 = out_pretty.clone();
-    let (compile_cmd, dist_compile_cmd, cacheable) = compilation.generate_compile_commands().unwrap();
+    let compile_out_pretty3 = out_pretty.clone();


I guess it would be nice to figure out something smarter here, eh?

luser · 2018-08-23T18:52:31Z

src/compiler/compiler.rs

+            let dist_output_paths = compilation.outputs()
+                .map(|(_key, path)| path_transformer.to_dist_abs(&cwd.join(path)))
+                .collect::<Option<_>>()
+                .unwrap();


Another unwrap.

luser · 2018-08-23T18:53:16Z

src/compiler/compiler.rs

+        .and_then(move |(path_transformer, mut dist_compile_cmd, dist_inputs_creator, dist_output_paths)| {
+            debug!("[{}]: Identifying toolchain", compile_out_pretty2);
+            let toolchain_creator_cb = BoxFnOnce::from(move |f| toolchain_creator.write_pkg(f));
            // TODO: put on a thread


If you don't have a fix for this TODO in a later patch, can you file a follow-up issue for it so we don't forget?

luser · 2018-08-23T18:55:37Z

src/compiler/compiler.rs


-    let test = b"#if defined(_MSC_VER)
+    let test = b"#if defined(_MSC_VER) && defined(__clang__)
+msvc-clang


I would use clang-cl here since that's how it's generally referred to.

luser · 2018-08-24T16:47:35Z

src/compiler/msvc.rs

    flag!("-Zi", DebugInfo),
    flag!("-c", DoCompilation),
    take_arg!("-deps", Path, Concatenated, DepFile),
+    take_arg!("-o", Path, Separated, Output), // Deprecated but valid


Is this needed for something in particular?

luser · 2018-08-24T16:54:10Z

src/compiler/msvc.rs

        }
    }
+
+    // TODO: doing this here reorders the arguments, hopefully that doesn't affect the meaning


We'd probably have to look at the clang-cl source to figure this out, but hopefully it's not an issue. Maybe we can just make a note of it in the docs somewhere?

luser · 2018-08-24T17:59:16Z

src/compiler/msvc.rs

+
+    let dist_command = (|| {
+        // http://releases.llvm.org/6.0.0/tools/clang/docs/UsersManual.html#clang-cl
+        // TODO: Use /T... for language?


This would be a useful followup. It looks like you could just replace -c with -Tc or -Tp:
https://docs.microsoft.com/en-us/cpp/build/reference/tc-tp-tc-tp-specify-source-file-type

luser · 2018-08-24T18:07:15Z

src/compiler/pkg.rs

+use std::fs::File;
+
+pub trait CompilerPackager {
+    fn write_pkg(self: Box<Self>, f: File) -> io::Result<()>;


Is there a particular reason this uses io::Result instead of our custom Error/Result?

luser · 2018-08-24T18:13:18Z

src/dist/cache.rs

 use errors::*;

+#[derive(Clone, Debug)]
+pub struct CustomToolchain {


It seems a little unfortunate to have two CustomToolchain structs that differ by only one member. I gather that since this one gets stored in a HashMap the other member is the key, but it's still not great.

luser · 2018-08-24T18:14:04Z

src/dist/cache.rs

+        let mut custom_toolchain_paths = HashMap::new();
+        for ct in config_custom_toolchains.into_iter() {
+            if custom_toolchain_paths.contains_key(&ct.compiler_executable) {
+                panic!("Multiple toolchains for {:?}", ct.compiler_executable)


This doesn't seem good.

luser · 2018-08-24T18:16:14Z

src/dist/cache.rs

-            Err(e) => panic!("{}", e),
+    // TODO: by this point the toolchain should be known to exist
+    pub fn get_toolchain(&self, tc: &Toolchain) -> Option<Vec<u8>> {
+        // TODO: be more relaxed about path casing and slashes on Windows


File a followup for this? Maybe just running the paths through fs::canonicalize would be good enough.

luser · 2018-08-24T18:16:50Z

src/dist/cache.rs

+            match self.cache.lock().unwrap().get(tc) {
+                Ok(rdr) => rdr,
+                Err(LruError::FileNotInCache) => return None,
+                Err(e) => panic!("{}", e),


This is also not good.

luser · 2018-08-24T18:17:25Z

src/dist/cache.rs

+        rdr.read_to_end(&mut ret).unwrap();
        Some(ret)
    }
    // TODO: It's more correct to have a FnBox or Box<FnOnce> here


Well, you can remove this TODO now!

luser · 2018-08-24T18:18:00Z

src/dist/cache.rs

+    pub fn put_toolchain(&self, compiler_path: &Path, weak_key: &str, create: BoxFnOnce<(fs::File,), io::Result<()>>) -> Result<(Toolchain, Option<String>)> {
+        if let Some(tc_and_compiler_path) = self.get_custom_toolchain(compiler_path) {
+            debug!("Using custom toolchain for {:?}", compiler_path);
+            let (tc, compiler_path) = tc_and_compiler_path.unwrap();


Another unwrap.

luser · 2018-08-24T18:23:21Z

src/dist/cache.rs

+                    Err(e) => return Some(Err(e)),
+                };
+                let tc = Toolchain { archive_id };
+                *maybe_tc = Some(tc.clone());


Would this be better written using the entry API?

luser · 2018-08-24T18:26:06Z

src/dist/mod.rs

 mod test;

+// TODO: paths (particularly outputs, which are accessed by an unsandboxed program)
+// should be some pre-sanitised AbsPath type


Maybe as a first pass you could just have a newtype around PathBuf that implements AsRef?

luser · 2018-08-24T18:26:30Z

src/dist/mod.rs

+    impl PathTransformer {
+        pub fn new() -> Self { PathTransformer }
+        pub fn to_dist_abs(&mut self, p: &Path) -> Option<String> {
+            if !p.is_absolute() { panic!("non absolute path {:?}", p) }


This shouldn't panic.

luser · 2018-08-24T18:27:35Z

src/dist/mod.rs

+            }
+        }
+        pub fn to_dist_abs(&mut self, p: &Path) -> Option<String> {
+            if !p.is_absolute() { panic!("non absolute path {:?}", p) }


This shouldn't panic.

luser · 2018-08-24T18:30:24Z

src/dist/mod.rs

+                    Component::Prefix(_) |
+                    Component::RootDir => panic!("unexpected part in path {:?}", p),
+                    Component::Normal(osstr) => osstr.to_str()?,
+                    // TODO: should be forbidden


Did you run into cases where these were necessary in practice? If not, it seems like it'd be better to forbid them now before anyone relies on this working.

luser

Overall everything in here looks sensible. I'd like to see all of the panic / assert / unwrap usage outside of tests cleaned up, but that can be done after everything lands.

aidanhs force-pushed the aphs-dist-sccache-cross-windows branch from 43c36ea to 6a4e9a3 Compare August 6, 2018 17:32

aidanhs commented Aug 6, 2018

View reviewed changes

aidanhs force-pushed the aphs-dist-sccache-cross-windows branch 3 times, most recently from e0fe8db to ad4ea62 Compare August 12, 2018 14:20

aidanhs changed the title ~~Support distributed cross-compilation on Windows, add some authorization~~ Support distributed cross-compilation on Windows Aug 12, 2018

aidanhs mentioned this pull request Aug 13, 2018

Add authentication throughout sccache-dist, including simple token auth #274

Merged

aidanhs added 6 commits August 16, 2018 09:52

Use a trait to represent compiler packagers

e19b7bb

Support custom toolchain overrides

aa1cfdc

Support windows with a path transformer

1000612

Fix some issues in bubblewrap implementation

b4c044c

Assorted fixes for clang-cl and windows in general

cd1d27c

Test fixups

2707dfd

aidanhs force-pushed the aphs-dist-sccache-cross-windows branch from ad4ea62 to 2707dfd Compare August 16, 2018 20:06

Doc tweaks

794c2a7

aidanhs commented Aug 16, 2018

View reviewed changes

luser reviewed Aug 23, 2018

View reviewed changes

luser reviewed Aug 24, 2018

View reviewed changes

luser mentioned this pull request Aug 24, 2018

Handle -Xclang for clang-cl #262

Closed

luser reviewed Aug 24, 2018

View reviewed changes

luser approved these changes Aug 24, 2018

View reviewed changes

luser merged commit d502791 into mozilla:master Aug 24, 2018

luser mentioned this pull request Aug 28, 2018

Don't pass /Brepro to the preprocessor for MSVC/clang-cl #264

Closed

Conversation

aidanhs commented Aug 6, 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!

aidanhs Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

aidanhs commented Aug 16, 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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

aidanhs Aug 6, 2018 •

edited

Loading