Richer argument handling for compilers by aidanhs · Pull Request #277 · mozilla/sccache

aidanhs · 2018-08-22T21:43:23Z

This now correctly pulls apart arguments like rustc -l static=a into their constituent parts at parse time. There should be no functional changes in this PR.

This is the majority of the major argument handling rework I mentioned, pulled out of the rust distributed work for ease of review. Each of the commits can be considered in isolation.

Should be reviewed after #274 (or just examine the commits after the scheduler auth one).

luser · 2018-08-27T16:23:31Z

src/compiler/args.rs

+type ArgResult<T> = StdResult<T, ArgError>;
+
+#[derive(Debug, PartialEq)]
+pub enum ArgError {


When we finish merging all of your work my first order of business is to switch from error-chain to failure. 😃

luser · 2018-08-27T19:36:52Z

src/compiler/args.rs

    UnknownFlag(OsString),
    /// Known flag argument ; e.g. "-bar"
-    Flag(&'static str),
+    Flag(&'static str, T),


It took me a lot more code reading to figure out why Flag needed to store the additional value here. I think the comment above Argument<T> could stand an additional sentence or two explaining what the type parameter is used for.

luser · 2018-08-27T19:38:01Z

src/compiler/args.rs


 /// Helper macro used to define ArgInfo::Flag's.
+/// Variant is an enum variant, e.g. enum ArgType { Variant(()) }
 ///     flag!("-foo")


You removed the single argument macro rule below, so the comment here should be removed as well.

luser · 2018-08-27T19:41:54Z

src/compiler/gcc.rs

-    Coverage,
+ArgData!{ pub
+    TooHardFlag(()),
+    TooHard(OsString),


This is a little awkward, but it's not really terrible.

luser · 2018-08-27T19:43:59Z

src/compiler/gcc.rs

+    SplitDwarf(()),
+    ProfileGenerate(()),
+    TestCoverage(()),
+    Coverage(()),


I don't know if it's feasible, but it would be nicer if the macro could take $x:ident without a type and handle it as ().

To answer my own question: I don't think it's possible with current stable Rust. RFC 2298 adds a ? repetition operator to macros which would make this feasible. This code works on Nightly (although I couldn't get it to work on the playground for some reason):
https://gist.github.com/luser/0284cecdec8f7a2f789e37ffc52f963e

Oh, it needed Edition: 2018 to work on the playground. (My local nightly is a few months out of date.)

luser · 2018-08-27T19:45:29Z

src/compiler/gcc.rs


-    for item in ArgsIter::new(it, arg_info) {
+    for arg in ArgsIter::new(it, arg_info) {
+        let arg = try_arg!(arg.map_err(|e| e.static_description()));


You seem to have this same map_err clause in all the places you use try_arg!, so why not just bake it into the macro?

luser · 2018-08-28T13:01:20Z

src/compiler/msvc.rs

+ArgData!{
+    TooHardFlag(()),
+    TooHard(OsString),
+    TooHardPath(PathBuf),


Since we don't really need the argument for TooHardPath, can we just drop this and use TooHard(OsString) for those arguments?

Further musing has me wonder if we could combine them all into TooHard(Option<OsString>) and just impl the right traits for Option<OsString>.

luser · 2018-08-28T14:53:06Z

src/compiler/compiler.rs

    Ok(T),
    /// Cannot cache this compilation.
-    CannotCache(&'static str),
+    CannotCache(&'static str, Option<String>),


Does this buy us a whole lot? Do we wind up with anything but "failed to parse argument value" in the second field? I have a WIP patch for #263 that saves counts of the CannotCache reasons in the server stats. I don't think this change will be a problem for it, but I'm not sure how I'd expose this new field.

Ah, I think you're inverting the way you're looking at this - "argument parse" is the consistent value that always gets put in the first field (exactly because of the cannotcache reason counting), but it didn't seem ideal to just throw away all nuance of that. Specifically, take a look at ArgParseError - Display creates a description of what went wrong with what part of the argument, which isn't possible in the bounds of a static string. This nice description will get printed in the logs.

luser · 2018-08-28T15:36:41Z

src/compiler/rust.rs

@@ -72,7 +72,7 @@ pub struct RustHasher {
 #[derive(Debug, Clone, PartialEq)]
 pub struct ParsedArguments {
    /// The full commandline, with arguments and their values as pairs.


Change the comment here to match?

luser · 2018-08-28T15:41:28Z

src/compiler/rust.rs

                let toolchain_creator = Box::new(RustCompilerPackager { sysroot: sysroot.clone() });
+                let mut arguments = arguments;
+                // Always request color output, the client will strip colors if needed.
+                arguments.push(Argument::WithValue("--color", ArgData::Color("always".into()), ArgDisposition::Separated));


Would it be simpler to move this into generate_compile_commands? I don't remember exactly why I put it here in the first place.

luser · 2018-08-28T15:42:04Z

src/compiler/rust.rs

        let me = *self;
        let RustHasher { executable, sysroot, compiler_shlibs_digests, parsed_args: ParsedArguments { arguments, output_dir, externs, staticlibs, crate_name, dep_info, color_mode: _ } } = me;
        trace!("[{}]: generate_hash_key", crate_name);
+        // TODO: this doesn't produce correct arguments if they should be concatenated - should use iter_os_strings


Is this fixed in a followup? If not, will this cause problems in practice?

I will observe that this behaviour has always been the case (it's just being flagged now) so I suspect in practice it does not cause issues :)

luser · 2018-08-28T15:47:37Z

src/compiler/rust.rs

+            // If no kind is specified, the default is dylib.
+            (name, None) => ("dylib".to_owned(), name),
+        };
+        Ok(ArgLinkLibrary { kind: kind, name: name })


nit: this could just be { kind, name }.

luser · 2018-08-28T15:51:24Z

src/compiler/rust.rs

+}
+
+#[derive(Clone, Debug, PartialEq)]
+struct ArgLinkLibrary {


All of these could stand to have a one-line descriptive comment, like "Represents -l [KIND=]PATH link arguments to rustc".

luser · 2018-08-28T16:21:03Z

src/compiler/rust.rs

+            // auto-added for target json discovery
+            return Ok(ArgTarget::Unsure(arg))
+        }
+        // The file doesn't exist so it can't be a path, safe to assume it's a name


I wonder if we'd be better off running rustc --print=target-list in the initial compiler detection and matching against that?

luser · 2018-08-28T16:23:26Z

src/compiler/rust.rs

-                    }
+            Some(LinkLibrary(ArgLinkLibrary { kind, name })) => {
+                if kind == "static" {
+                    static_lib_names.push(name.to_owned())


We could presumably push this logic down into hash generation now, since the arguments there will have enough context. (It's not a big deal, though.)

luser

I left a number of comments, but I don't think any of them need to block merging. Feel free to merge this once you rebase and merge the previous PR.

aidanhs · 2018-08-30T02:18:41Z

I suddenly felt motivated, so I implemented a better macro that allows enum variants without data. It was actually nowhere near as bad as I thought it was going to be (which was why I avoided it originally).

luser · 2018-08-30T12:29:20Z

src/compiler/gcc.rs

    PassThrough(OsString),
    PassThroughPath(PathBuf),
-    PreprocessorArgumentFlag(()),
+    PreprocessorArgumentFlag,


That's much nicer! I fiddled with this for a bit but couldn't find a working solution except with the unstable ? macro repetition operator.

luser · 2018-08-30T12:32:10Z

The AppVeyor failures look to be network failures, but the stable and beta builds were successful so I'm going to merge this anyway.

luser · 2018-08-30T12:41:07Z

Rebased locally and merged: 105ab91...080e1a7

Co-authored-by: Lucio Franco <luciofranco14@gmail.com>

luser reviewed Aug 27, 2018

View reviewed changes

luser reviewed Aug 28, 2018

View reviewed changes

luser mentioned this pull request Aug 28, 2018

Use Reqwest to access s3 to follow redirects #279

Merged

luser reviewed Aug 28, 2018

View reviewed changes

luser approved these changes Aug 28, 2018

View reviewed changes

luser mentioned this pull request Aug 28, 2018

Use redis-0.9 for async requests #280

Closed

aidanhs added 8 commits August 29, 2018 22:35

Implement scheduler auth for clients and servers, add job tokens

36741e8

Backport conditional compilation, fix markdown

f104074

Be more pessimistic about @ arguments

ce1dbee

Make parsed argument data more typed

0e36db3

More nuanced reporting of argument parsing failures

351de9a

No implicit iteration, rename some methods for clarity

26d4deb

Preserve the types of Rust arguments as far as possible

c8c7605

Review Rust arguments typing and move parsing out of handling loop

8a79aca

aidanhs force-pushed the aphs-dist-sccache-rust-args branch 2 times, most recently from 5583ead to 7127eaf Compare August 30, 2018 02:18

Better (and uglier) macro, some review comment tweaks

b72bccc

aidanhs force-pushed the aphs-dist-sccache-rust-args branch from 7127eaf to b72bccc Compare August 30, 2018 08:40

luser reviewed Aug 30, 2018

View reviewed changes

luser closed this Aug 30, 2018

luser mentioned this pull request Aug 30, 2018

Handle clang-specific PGO flags #276

Closed

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

chore: Add blocknig server example (mozilla#277)

9e7d35a

Co-authored-by: Lucio Franco <luciofranco14@gmail.com>

Conversation

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

luser left a comment

Choose a reason for hiding this comment

Uh oh!

aidanhs commented Aug 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luser commented Aug 30, 2018

Uh oh!

luser commented Aug 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants