Skip to content
New issue

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

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

Already on GitHub? # 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
Member

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.

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

Successfully merging this pull request may close these issues.

7 participants