Changes for sccache+msvc+LLVM by alexcrichton · Pull Request #78 · mozilla/sccache

alexcrichton · 2017-03-08T18:53:48Z

This few commits were what I found was necessary to get sccache fully working on MSVC when compiling LLVM in rust-lang/rust. It basically boiled down to:

Fixes #61
Fixes #47

KalitaAlexey

I checked as much as I understood.

KalitaAlexey · 2017-03-15T07:04:42Z

src/compiler/clang.rs

+        let ParsedArguments {
+            input,
+            extension,
+            depfile: _depfile,


You can write _ instead of _depfile.
https://is.gd/7nHDiH

KalitaAlexey · 2017-03-15T07:06:16Z

src/compiler/clang.rs

-            }
-            o @ _ => assert!(false, format!("Got unexpected parse result: {:?}", o)),
-        }
+        let args = stringvec!["-c", "foo.cxx", "-arch", "xyz", "-fabc","-I", "include", "-o", "foo.o", "-include", "file"];


Missing space after `"-afbc".

KalitaAlexey · 2017-03-15T07:06:39Z

src/compiler/clang.rs

+        let ParsedArguments {
+            input,
+            extension,
+            depfile: _depfile,


The same as I suggested above.

KalitaAlexey · 2017-03-15T07:09:14Z

src/compiler/gcc.rs

+        let ParsedArguments {
+            input,
+            extension,
+            depfile: _depfile,


The same as I suggested above.

KalitaAlexey · 2017-03-15T07:10:35Z

src/compiler/msvc.rs

    let mut depfile = None;
+    let mut show_includes = false;

    //TODO: support arguments that start with / as well.


I suppose the comment is no longer needed.

KalitaAlexey · 2017-03-15T07:14:02Z

src/compiler/msvc.rs

+            v @ _ if v.starts_with("-deps") => {
+                depfile = Some(v[5..].to_owned());
+            }
+            // Arguments we can't handle.


The comment should be placed below because we will be able to handle "-showIncludes".

I've built a local copy with mozilla/sccache#79 and mozilla/sccache#78. Let's see if that helps rust-lang#40240!

travis: Update sccache binary I've built a local copy with mozilla/sccache#79 and mozilla/sccache#78. Let's see if that helps #40240!

luser · 2017-03-28T10:15:19Z

src/compiler/msvc.rs

-    let mut it = arguments.iter();
+    let mut it = arguments.iter().map(|i| {
+        if i.starts_with("/") {
+            format!("-{}", &i[1..])


This is a clever hack!

luser · 2017-03-28T10:17:45Z

src/compiler/msvc.rs

+                debug_info = true;
+                common_args.push(arg.clone());
+            }
+            v if v.starts_with("-Fd") => {


I was thinking it might be nice to refactor some of the other compiler argument parsing code to use the ArgsIter pattern I implemented for Rust:

sccache/src/compiler/rust.rs

Line 366 in 7ae4c25

let it = ArgsIter::new(arguments, &args_with_val);

It's probably not as useful here, but it was nice to be able to write all the match arms as strings instead of having v.starts_with(...) all over the place.

luser · 2017-03-28T10:19:06Z

src/compiler/c.rs

    executable: String,
    /// The output from running the preprocessor.
-    preprocessor_output: Vec<u8>,
+    preprocessor_result: process::Output,


This change ought to make it easier to fix #72 as well!

luser · 2017-03-28T10:20:35Z

src/compiler/clang.rs

+            msvc_show_includes,
+            common_args,
+        } = match _parse_arguments(&args) {
+            CompilerArguments::Ok(args) => args,


That is a much more sensible way to write this.

luser · 2017-03-28T10:31:02Z

src/compiler/msvc.rs

+    }
+    Box::new(ret.map(|(cacheable, mut output)| {
+        let prev = mem::replace(&mut output.stderr, extra_stderr);
+        output.stderr.extend(prev);


I don't think this is actually correct, and this is probably what breaks the configure test in the Firefox build. See my comment in detect_showincludes_prefix:

sccache/src/compiler/msvc.rs

Line 104 in 7ae4c25

// The MSDN docs say the -showIncludes output goes to stderr,

I just did a simple local test with MSVC2015 and the -showIncludes output goes to stdout when compiling:

ted@DESKTOP-32V9ND8 /c/build $ cl -Fonul -nologo -showIncludes -c test.c > /tmp/cl.stdout 2>/tmp/cl.stderr ted@DESKTOP-32V9ND8 /c/build $ cat /tmp/cl.stdout test.c Note: including file: C:\Program Files (x86)\Windows Kits\10\include\10.0.10586.0\ucrt\stdio.h Note: including file: C:\Program Files (x86)\Windows Kits\10\include\10.0.10586.0\ucrt\corecrt.h Note: including file: C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\vcruntime.h Note: including file: C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\sal.h Note: including file: C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\ConcurrencySal.h Note: including file: C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\vadefs.h Note: including file: C:\Program Files (x86)\Windows Kits\10\include\10.0.10586.0\ucrt\corecrt_wstdio.h Note: including file: C:\Program Files (x86)\Windows Kits\10\include\10.0.10586.0\ucrt\corecrt_stdio_config.h

When running the preprocessor it definitely goes to stderr.

Unfortunately I think this means we'll have to do something slightly more complex here, like parse the preprocessor stderr to get just the showIncludes lines, and then put just those in the compiler stdout.

alexcrichton · 2017-03-30T15:48:40Z

Thanks for the review! It'll take a bit I think to get time to cover the comments, but I'll dig in as soon as I get a chance.

alexcrichton · 2017-04-10T15:10:30Z

According to https://msdn.microsoft.com/en-us/library/hdkef6tk.aspx

The /showIncludes option emits to stderr, not stdout.

This seems empirically false...

Helps avoid some extra indentation

Just do a simple translation from `/foo` to `-foo`

Track the `-showIncludes` flag being passed down to the compilation itself, and then also be sure to thread the entire output of the preprocessor down to compilations instead of just stdout. Once that was done support was implemented by simply prepending the preprocessor stderr to the compiler stderr if the `-showIncludes` flag was passed.

luser · 2017-04-11T10:24:44Z

src/compiler/msvc.rs

+    // means that the "show includes" business when to stderr. Normally, though,
+    // the compiler emits `-showIncludes` output to stdout. To handle that we
+    // take the stderr of the preprocessor and prepend it to the stdout of the
+    // compilation.


I think at some point we'll want to do something like this for all compilers to fix #72, but we'll probably have to put the warnings on stderr for compatibility.

alexcrichton · 2017-04-11T14:22:31Z

Oh sorry I pushed an update but then confused myself as to how it could work at all! I'm not actually sure how this patch works but maybe I'm missing something?

We recreate stderr_bytes but don't add include lines to it, so I'm not sure how this reaches the final compiler's output. When manually executing sccache cl foo.c -c -showIncludes though it seems to work!

alexcrichton · 2017-04-11T14:39:43Z

Oh so it turns out my cl.exe was totally broken for other resons for totally unrelated reasons. I'll send a follow-up patch.

alexcrichton · 2017-04-11T14:55:23Z

Ok, I've sent a PR which I believe fixes the issue: #106

alexcrichton · 2017-04-11T14:55:30Z

Sorry for the runaround!

CI: publish sccache dist

alexcrichton requested a review from luser March 8, 2017 18:53

alexcrichton force-pushed the msvc-ninja branch from 6bb8785 to b61570d Compare March 13, 2017 21:21

KalitaAlexey reviewed Mar 15, 2017

View reviewed changes

alexcrichton force-pushed the msvc-ninja branch from b61570d to 17da12f Compare March 15, 2017 18:58

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 16, 2017

travis: Update sccache binary

ec829fe

I've built a local copy with mozilla/sccache#79 and mozilla/sccache#78. Let's see if that helps rust-lang#40240!

alexcrichton mentioned this pull request Mar 16, 2017

travis: Update sccache binary rust-lang/rust#40575

Merged

bors added a commit to rust-lang/rust that referenced this pull request Mar 16, 2017

Auto merge of #40575 - alexcrichton:update-sccache, r=alexcrichton

6bf1604

travis: Update sccache binary I've built a local copy with mozilla/sccache#79 and mozilla/sccache#78. Let's see if that helps #40240!

bors added a commit to rust-lang/rust that referenced this pull request Mar 17, 2017

Auto merge of #40575 - alexcrichton:update-sccache, r=alexcrichton

9fae040

travis: Update sccache binary I've built a local copy with mozilla/sccache#79 and mozilla/sccache#78. Let's see if that helps #40240!

alexcrichton force-pushed the msvc-ninja branch 2 times, most recently from e927bc8 to 4020e89 Compare March 27, 2017 17:14

luser suggested changes Mar 28, 2017

View reviewed changes

alexcrichton added 3 commits April 10, 2017 08:12

Refactor msvc slightly w/ while let

b6064a6

Helps avoid some extra indentation

Support /foo and -foo on MSVC

7b4c687

Just do a simple translation from `/foo` to `-foo`

alexcrichton force-pushed the msvc-ninja branch from 4020e89 to 8660519 Compare April 10, 2017 18:35

luser reviewed Apr 11, 2017

View reviewed changes

luser approved these changes Apr 11, 2017

View reviewed changes

luser merged commit 334359c into mozilla:master Apr 11, 2017

alexcrichton deleted the msvc-ninja branch April 11, 2017 14:22

crystalin pushed a commit to PureStake/sccache that referenced this pull request Jul 2, 2021

Merge pull request mozilla#78 from paritytech/3x8_publish_sccache-dist

87b1e81

CI: publish sccache dist

tottoto pushed a commit to tottoto/sccache that referenced this pull request Feb 6, 2026

Suggestion: Add edition to example Cargo.toml snippet (mozilla#78)

51ce0b9

Conversation

alexcrichton commented Mar 8, 2017

Uh oh!

KalitaAlexey left a comment

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!

alexcrichton commented Mar 30, 2017

Uh oh!

alexcrichton commented Apr 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Apr 11, 2017

Uh oh!

alexcrichton commented Apr 11, 2017

Uh oh!

alexcrichton commented Apr 11, 2017

Uh oh!

alexcrichton commented Apr 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants