Add MSVC support for Response Files #1618

temportalflux · 2023-02-23T15:51:57Z

Goal

Add support for response/command files in MSVC as defined by the MSVC and Visual Studio MSBuild docs.

Related Issues

History of Response File support in MSVC implementation

Describe how to use it with MSVC #107 mentions an interest in supporting response files for MSVC
The above discussion inspired Support MSVC response files #192 in 2017, it was abandoned by the author an elongated feedback cycle.
That PR was followed by another PR (Add basic support for response files #378), which was also abandoned after the author was redirected to Support MSVC response files #192.
Support rsp files on windows/msvc #1082 was opened in Dec 2021, which again requested support for response files in MSVC - it has not received any replies. This issue is still open.
MSVC - cannot cache with "@" (command files) #1183 repeated the same request in June 2022, and a comment was added referencing all of the above tickets. This issue is still open.
I started discussion Response File support for MSVC #1594 in preparation for this PR.

Implementation Details

This change adds an iterator layer between the command-line argument iterator and the ArgIter, which used to compare arguments against the supported flags/options. This new layer determines if an option is a response-file directive (@file), and if it is, reads the options from the file before continuing to iterate over the command-line args. This requires an additional file-parsing iterating (SplitArgs) to split the file contents into arguments in a way which is consistent with the file format.

The encoding crate is used to read utf-8 (default encoding in rust) & utf-16 (big and little endian) encodings. The latter is used by MSBuild when generating response files.

Please see the Response Files doc in this PR for an overview of how response files are used in MSVC, what the expected format is, how they differ from GCC, and how they are implemented for both GCC and MSVC in SCCache.

Flags

This pull request also includes a handful of changes/additions to the MSVC flags. These flags could be separated into a separate PR, but given how small the changes are, they are currently bundled in with the MSVC changes. Our environment requires these flags, hence the need to have them supported.

Updated Flags

/FC (previously marked as TooHardFlag) changed to PassThrough because it only affects how the compiler prints debug information. flag docs

Added Flags

Flags which allow users to pass "disable" feature flags which do not affect cachability:

/Gm-, disable minimal rebuilds. flag docs
/WX-, disable "treat all compiler warnings as errors". flag docs
/openmp-, disable the openmp directive. flag docs

Flags which enable features:

/permissive, allows code which doesn't conform to standard, but doesnt change what needs to be compiled, just how the compiler does so. This is major for some legacy projects which are not up-to-date to standard conformance. flag docs

Xuanwo · 2023-02-23T15:55:39Z

suggestion: Can you add a basic test for this feature?

temportalflux · 2023-02-23T15:59:34Z

suggestion: Can you add a basic test for this feature?

What do you mean by "basic test"? There are a number of tests in compiler/msvc.rs, including test_responsefile_absolute_path, which is the minimal viable successful test (the response file exists and contains arguments). Is there a more basic/minimal test you'd expect to see?

Xuanwo · 2023-02-23T16:04:13Z

I mean add a basic integration test like what we do in tests

temportalflux · 2023-02-23T16:17:07Z

Hm yeah I can look into that. I honestly didn't even notice the integration tests, and I haven't yet found any documentation on how those tests are added to or maintained. I'll look through the commit history to see if I can find a decent template/example.

drahnr

First superficial pass, this is the quality we strive for ❤️ thank you for the time and effort put in.

Two questions: Is there a recursion limit on eval? Is that spec'd? And could you write an integration test as @Xuanwo asked?

The only thing after that is appeasing CI

sylvestre · 2023-02-23T16:30:51Z

I'll look through the commit history to see if I can find a decent template/example.

I love docs but here, copying the current model is easy enough:
https://github.com/mozilla/sccache/blob/main/.github/workflows/integration-tests.yml#L30-L76

temportalflux · 2023-02-23T16:31:32Z

Is there a recursion limit on eval? Is that spec'd?

Yes there is a limit! I noted it in the docs/ResponseFiles.md doc; A response file cannot contain additional @file options, they are not recursive.. That comes straight out of the MSVC docs: It is not possible to specify the @ option from within a response file. That is, a response file cannot embed another response file.

Integration testing wise, I'm looking into that now. Seems like there is little to no obvious docs on adding one, but it looks like the pattern is adding a new function call to run_sccache_command_tests under the correct compiler (cl.exe for msvc) which runs the integration test (like test_msvc_deps).

Looks like CI is failing because clippy says let...else statements are unstable, even though they were stabilized recently. I can change the syntax on those two statements if folks prefer to not use let...else.

codecov-commenter · 2023-02-23T18:30:52Z

Codecov Report

Patch coverage: 48.29% and project coverage change: +0.01 🎉

Comparison is base (e3eefb3) 29.52% compared to head (c165b3b) 29.54%.

❗ Current head c165b3b differs from pull request most recent head 1cb4928. Consider uploading reports for the commit 1cb4928 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1618      +/-   ##
==========================================
+ Coverage   29.52%   29.54%   +0.01%     
==========================================
  Files          49       49              
  Lines       16662    17012     +350     
  Branches     8042     8195     +153     
==========================================
+ Hits         4920     5026     +106     
- Misses       6836     6996     +160     
- Partials     4906     4990      +84

Impacted Files	Coverage Δ
tests/system.rs	`28.57% <0.00%> (-1.63%)`	⬇️
src/compiler/msvc.rs	`44.87% <52.07%> (+2.07%)`	⬆️

... and 12 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

temportalflux · 2023-02-23T18:32:24Z

Added a fairly minimal integration test. Please nitpick it because how the integration tests work in this repo is still somewhat evading me.

src/compiler/msvc.rs

drahnr

A few small nits, generally not a a fan of manually parsing file formats, but in this case it's ok.

Thank you for you contribution! I really like the documentation you created in this PR!

temportalflux · 2023-02-28T14:17:33Z

A few small nits, generally not a a fan of manually parsing file formats, but in this case it's ok.

Thank you for you contribution! I really like the documentation you created in this PR!

Thanks for the review, and I'm glad the documentation is helpful! I've addressed the nits and squashed all subsequent changes into the original 2 commits after rebasing onto latest main.

temportalflux · 2023-03-02T14:14:54Z

@drahnr @sylvestre what does the timeline on getting this merged look like? Any chance it can make it in for the next pre-release or full release (0.4.0)?

sylvestre · 2023-03-02T14:41:47Z

I still would like to see an end to end integration test in
https://github.com/mozilla/sccache/blob/main/.github/workflows/integration-tests.yml if it makes sense

temportalflux · 2023-03-02T15:25:46Z

I still would like to see an end to end integration test in https://github.com/mozilla/sccache/blob/main/.github/workflows/integration-tests.yml if it makes sense

I'm not sure I fully follow. Are you saying that there should be a new job in the integration tests workflow which runs cargo test on the windows-2019 os against the x86_64-pc-windows-msvc to test msvc?

Edit: something like this?

  msvc:
    runs-on: windows-2019
    needs: build
    
    steps:
      - name: Clone repository
        uses: actions/checkout@v3

      - name: Install rust
        uses: ./.github/actions/rust-toolchain
        with:
          toolchain: "stable"

      - name: Build
        run: cargo clean && cargo test --no-run
      
      - name: Run tests
        run: cargo test --locked --target x86_64-pc-windows-msvc

sylvestre · 2023-03-02T18:17:14Z

Nope, building a fake project that uses MSVC / response files

temportalflux · 2023-03-03T20:10:18Z

The above force push is just rebasing on latest so its easy to merge when that time comes

sylvestre · 2023-03-05T12:45:19Z

ilammy/msvc-dev-cmd@v1.4.1 is not allowed to be used in mozilla/sccache.

is there an alternative ?
(i can ask to the Mozilla admin to enable it but it might take a few days/weeks)

temportalflux · 2023-03-05T17:54:18Z

Unfortunately I've yet to find a good alternative. All of the resources point towards using that action, which basically calls the vsvars.bat in the Visual Studio installation (bundled in the windows-2019 image) and copies the outputted results into the job's environment variables https://github.com/ilammy/msvc-dev-cmd/blob/master/index.js#L147. If we can't use that action, we could either copy the repo into .github/actions and maintain it until the action is approved, or find some way to manually do all the stuff that script is doing within the job. I'm totally open to any alternatives if I've missed another action that tackles the same thing, but all my research points to that one resource.

EDIT: A coworker informed me that https://github.com/microsoft/setup-msbuild exists, but after attempting to use it I found that it only sets up msbuild, not the msvc compiler cl.exe. So no change to the above comment from this discovery, just more context.

temportalflux · 2023-03-06T15:07:06Z

Given that there are no other integration tests comparable new msvc one (they are all testing integrations with outside tools, not the underlying compilers), and I'd prefer to be efficient about work spent while minimizing delay time, I think the best route forward is:

Separate the integration test job into its own PR, whiches uses ilammy/msvc-dev-cmd since it seems to be the standard
Merge this PR w/o the integration test
Mark the integration test PR as blocked until ilammy/msvc-dev-cmd is approved

temportalflux · 2023-03-06T21:32:48Z

@sylvestre any thoughts on the msvc gh-action dependency & split-the-PR solution?

drahnr · 2023-03-06T21:34:23Z

I fully agree, we don't need to mention it in the release notes or mark it as experimental, worst case it's just as broken as before.

@sylvestre I am inclined to merge it now, regardless of the CI test case

sylvestre · 2023-03-08T15:14:29Z

Please give me a week to see if we can get it enabled
AFAIK, there isn't any rush.

sylvestre · 2023-03-09T16:39:02Z

I think we enabled the github action. Could you please rebase it to retrigger the CI? for some reasons, I can't do it on github on this PR ?!

Adds an iteration layer between the command-line argument iterator and the `ArgIter` used to compare arguments against the supported flags/options. This new layer determines if an option is a response-file directive (`@file`), and if it is, reads the options from the file before continuing to iterate over the command-line args. This requires an additional file-parsing iterating (`SplitArgs`) to split the file contents into arguments in a way which is consistent with the file format. The `encoding` crate is used to read utf-8 (default encoding in rust) & utf-16 (big and little endian) encodings. The latter is used by `MSBuild` when generating response files. Resources: - [MSDN](https://docs.microsoft.com/en-us/cpp/build/reference/at-specify-a-compiler-response-file) - [MSBuild](https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-response-files?view=vs-2019) Contributes to #107 Based off of #192 Closes #1082 Closes #1183

Updated Flags: - `/FC` ([previously marked as TooHardFlag](https://github.com/proletariatgames/sccache/commit/bf6fb5de6f566d20321bf4a47714398250975d03)) changed to `PassThrough` because it only affects how the compiler prints debug information. [flag docs](https://learn.microsoft.com/en-us/cpp/build/reference/fc-full-path-of-source-code-file-in-diagnostics?view=msvc-170) Added Flags, to allow users to pass "disable" feature flags which do not affect cachability: - `/Gm-`, disable minimal rebuilds. [flag docs](https://learn.microsoft.com/en-us/cpp/build/reference/gm-enable-minimal-rebuild?view=msvc-170) - `/WX-`, disable "treat all compiler warnings as errors". [flag docs](https://learn.microsoft.com/en-us/cpp/build/reference/compiler-option-warning-level?view=msvc-170) - `/openmp-`, disable the openmp directive. [flag docs](https://learn.microsoft.com/en-us/cpp/build/reference/openmp-enable-openmp-2-0-support?view=msvc-170) Added Flags which enable features: - `/permissive`, allows code which doesn't conform to standard, but doesnt change what needs to be compiled, just how the compiler does so. This is major for some legacy projects which are not up-to-date to standard conformance. [flag docs](https://learn.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance?view=msvc-170) Also added a comment to `/Fp` to specify why it is not supported. `/Fp` is used to adjust PCH behaviors when used in conjunction with `/Yu` or `/Yc`. [flag docs](https://learn.microsoft.com/en-us/cpp/build/reference/fp-name-dot-pch-file?view=msvc-170) Contributes to #978

temportalflux · 2023-03-09T16:52:33Z

Rebased and pushed!

temportalflux · 2023-03-09T20:22:10Z

Whats going on with the freebsd check?

sylvestre · 2023-03-09T20:29:27Z

Don't bother. It is quite fragile. Le jeu. 9 mars 2023, 21:22, Dustin Yost ***@***.***> a écrit :

…

Whats going on with the freebsd check? — Reply to this email directly, view it on GitHub <#1618 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFTBDT27BFD2U6OKNO2FT3W3I3X5ANCNFSM6AAAAAAVFZ3SXI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Builds against windows and performs a series of compilation checks to ensure that the mock project builds and response files are properly expanded

temportalflux · 2023-03-09T21:53:58Z

Looks like it was only failing the integration test because the version allowed was @v1 and the PR was using the more specific @v1.4.1 for the msvc-dev-cmd. Squashed a change into the last commit which changed the version from 1.4.1 to 1 and hopefully it should be allowed now.

sylvestre · 2023-03-09T22:15:18Z

yeah, i asked if we could fix that on the admin side

sylvestre · 2023-03-09T23:26:03Z

Looks like it worked this time:

Compile requests                      2
Compile requests executed             2
Cache hits                            1
Cache hits (C/C++)                    1
Cache misses                          1
Cache misses (C/C++)                  1

well done :)

assarbad · 2023-09-11T20:32:43Z

EDIT: A coworker informed me that https://github.com/microsoft/setup-msbuild exists, but after attempting to use it I found that it only sets up msbuild, not the msvc compiler cl.exe. So no change to the above comment from this discovery, just more context.

Shameless plug: https://github.com/assarbad/scripts/blob/master/setvcvars.cmd

drahnr reviewed Feb 23, 2023

View reviewed changes

drahnr reviewed Feb 28, 2023

View reviewed changes

src/compiler/msvc.rs Outdated Show resolved Hide resolved

drahnr reviewed Feb 28, 2023

View reviewed changes

src/compiler/msvc.rs Show resolved Hide resolved

drahnr approved these changes Feb 28, 2023

View reviewed changes

Xuanwo approved these changes Mar 2, 2023

View reviewed changes

drahnr approved these changes Mar 4, 2023

View reviewed changes

temportalflux added 2 commits March 9, 2023 11:52

Add msvc response file integration test

1cb4928

Builds against windows and performs a series of compilation checks to ensure that the mock project builds and response files are properly expanded

sylvestre merged commit 00bd503 into mozilla:main Mar 9, 2023

UsernameLinde mentioned this pull request Mar 27, 2023

sccache places object file at different location compared to cl.exe and causes link failure when @rsp file is used #1687

Open

Alexei-Barnes mentioned this pull request May 31, 2023

Response file parsing for GCC and Clang does not handle quoted arguments #1780

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MSVC support for Response Files #1618

Add MSVC support for Response Files #1618

temportalflux commented Feb 23, 2023

Xuanwo commented Feb 23, 2023 •

edited

temportalflux commented Feb 23, 2023

Xuanwo commented Feb 23, 2023

temportalflux commented Feb 23, 2023

drahnr left a comment

sylvestre commented Feb 23, 2023

temportalflux commented Feb 23, 2023

codecov-commenter commented Feb 23, 2023 •

edited

temportalflux commented Feb 23, 2023

drahnr left a comment

temportalflux commented Feb 28, 2023

temportalflux commented Mar 2, 2023

sylvestre commented Mar 2, 2023

temportalflux commented Mar 2, 2023 •

edited

sylvestre commented Mar 2, 2023

temportalflux commented Mar 3, 2023

sylvestre commented Mar 5, 2023

temportalflux commented Mar 5, 2023 •

edited

temportalflux commented Mar 6, 2023

temportalflux commented Mar 6, 2023

drahnr commented Mar 6, 2023 •

edited

sylvestre commented Mar 8, 2023

sylvestre commented Mar 9, 2023

temportalflux commented Mar 9, 2023

temportalflux commented Mar 9, 2023

sylvestre commented Mar 9, 2023 via email

temportalflux commented Mar 9, 2023

sylvestre commented Mar 9, 2023

sylvestre commented Mar 9, 2023

assarbad commented Sep 11, 2023

Add MSVC support for Response Files #1618

Add MSVC support for Response Files #1618

Conversation

temportalflux commented Feb 23, 2023

Goal

Related Issues

Implementation Details

Flags

Updated Flags

Added Flags

Xuanwo commented Feb 23, 2023 • edited

temportalflux commented Feb 23, 2023

Xuanwo commented Feb 23, 2023

temportalflux commented Feb 23, 2023

drahnr left a comment

Choose a reason for hiding this comment

sylvestre commented Feb 23, 2023

temportalflux commented Feb 23, 2023

codecov-commenter commented Feb 23, 2023 • edited

Codecov Report

temportalflux commented Feb 23, 2023

drahnr left a comment

Choose a reason for hiding this comment

temportalflux commented Feb 28, 2023

temportalflux commented Mar 2, 2023

sylvestre commented Mar 2, 2023

temportalflux commented Mar 2, 2023 • edited

sylvestre commented Mar 2, 2023

temportalflux commented Mar 3, 2023

sylvestre commented Mar 5, 2023

temportalflux commented Mar 5, 2023 • edited

temportalflux commented Mar 6, 2023

temportalflux commented Mar 6, 2023

drahnr commented Mar 6, 2023 • edited

sylvestre commented Mar 8, 2023

sylvestre commented Mar 9, 2023

temportalflux commented Mar 9, 2023

temportalflux commented Mar 9, 2023

sylvestre commented Mar 9, 2023 via email

temportalflux commented Mar 9, 2023

sylvestre commented Mar 9, 2023

sylvestre commented Mar 9, 2023

assarbad commented Sep 11, 2023

Xuanwo commented Feb 23, 2023 •

edited

codecov-commenter commented Feb 23, 2023 •

edited

temportalflux commented Mar 2, 2023 •

edited

temportalflux commented Mar 5, 2023 •

edited

drahnr commented Mar 6, 2023 •

edited