Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For non-MSVC, separate flags/options from the input file. (#513) #514

Merged
merged 1 commit into from May 11, 2022

Conversation

ncalexan
Copy link
Contributor

@ncalexan ncalexan commented Jun 9, 2020

This avoids a clang-cl issue common when cross-compiling from macOS
to Windows: the -Wslash-u-filename where a /Users/... path is
interpreted as the cl.exe flag /U.

In general it's somewhere between harmless and good form to separate
flags/options from input files, so -- is used for all compilers save
MSVC.

There are no existing tests mocking out clang-cl and surface efforts
didn't succeed, so that will have to wait for follow-up.

@ncalexan
Copy link
Contributor Author

ncalexan commented Jun 9, 2020

@alexcrichton two relevant questions/things to check:

  1. I elected to separate with -- for all compilers save MSVC. That's not the most conservative choice, but I do think it's the right one. Happy to be proven wrong here. And the tests are back, failing. I'll guard this further, maybe down to clang and clang-cl?
  2. I tried to mock out clang-cl, but the test code is difficult to work with and can't really reach into the internals such as the FamilyType. Happy to be shown the way here.

@alexcrichton
Copy link
Member

Nah yeah I agree that conservatively passing -- is probably the best way to go here. Could you expand the in-code comment about how it's intended to prevent filesystem paths on Unix from accidentally being interpreted as flags?

It may be pretty difficult to add a test for this since I don't think any of the tests are in "mock clang-cl mode". For now I think it's ok to skip the test. Unfortunately most functionality in this crate doesn't have a test because it's typically quite difficult to set up all the native toolchains and various environments.

…h-u-filename`. (rust-lang#513)

This avoids a `clang-cl` issue common when cross-compiling from macOS
to Windows: the `-Wslash-u-filename` where a `/Users/...` path is
interpreted as the `cl.exe` flag `/U`.

Not all compilers recognize `--` so this is limited to `clang-cl`.

There are no existing tests mocking out `clang-cl` and surface efforts
didn't succeed, so that will have to wait for follow-up.
@ncalexan
Copy link
Contributor Author

I will have to patch mozilla-central with this to see if it actually addresses the problem for clang-cl, but I expect it will.

@alexcrichton
Copy link
Member

Ok sounds good to me! Wanna let me know if this works there and if so I can merge and publish?

@m-ou-se m-ou-se merged commit fcb8df5 into rust-lang:main May 11, 2022
@ChrisDenton
Copy link
Contributor

This PR appears to break in rustc's CI. See details. Perhaps @ncalexan or @roblabla would be willing to investigate this?

In the meantime this may need to be reverted until there's a fix.

@roblabla
Copy link
Contributor

roblabla commented Nov 8, 2022

So it seems like even if compiler.family == (ToolFamily::Msvc { clang_cl: true }), the assembler may be MSVC, and thus not accept the -- to separate the outputs.

A quick fix would be to add an && !is_asm to the if statement.

@roblabla
Copy link
Contributor

roblabla commented Nov 8, 2022

Got a fix over at #747

@thomcc
Copy link
Member

thomcc commented Nov 8, 2022

@glandium
Copy link
Contributor

FYI, this broke building with sccache, because sccache doesn't handle the "--" properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants