-
Notifications
You must be signed in to change notification settings - Fork 461
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
Fix compiler family detection issue with clang-cl on macOS #1328
Conversation
I can't know for sure if the compiler is clang-cl, so can't apply the workaround reliably. Alternatively, we could combine if both are true then it's likely clang-cl on macOS, which needs workaroud. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear to me why we even support clang-cl
on macOS and Linux? cl.exe
is not something you'd run on those platforms, so why would you ever run clang-cl
? (Not a blocker for fixing this, just something I'd like to understand ;) )
cfg(macos)
That's a bad idea IMO, since this problem is also present on Linux and Windows hosts, just less prominently since their root paths usually start with a lowercase letter (on Linux), or prefixed with the drive letter (on Windows), but that's not a requirement!
Actually, the --
separator seems to be supported by normal Clang too, so why don't we just do this workaround everywhere?
This would also avoid issues if the user did e.g. .file("-l")
.
Based on the original issue, I believe they are using cargo-xwin to cross compile to msvc windows from a unix environment.
Does gcc and other compiler support it? If they mostly do, we can add
Good idea |
$ touch foo.c
$ clang -c -- foo.c
$ gcc -c -- foo.c
gcc: error: unrecognized command-line option '--' Doesn't seem like it? |
59b22d8
to
acfec3e
Compare
Fixed #1327 Signed-off-by: Jiahao XU <[email protected]>
to disambiguous path from arguments. Signed-off-by: Jiahao XU <[email protected]>
acfec3e
to
2e917e7
Compare
Signed-off-by: Jiahao XU <[email protected]>
17a6eb8
to
76f4f6c
Compare
Signed-off-by: Jiahao XU <[email protected]>
76f4f6c
to
5d5956d
Compare
"Expects macro `__clang__`, `__GNUC__` or `__EMSCRIPTEN__`, `__VXWORKS__` or accepts cl style flag `-?`, but found none", | ||
)) | ||
} | ||
if stdout.contains("-Wslash-u-filename") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This... Seems brittle :/.
Not that I have a better solution (yet).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like it either, but I really can't think of an alternative solution that is better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does CMake do? (if we're already following in their footsteps)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure , I didn't check about cmake.
I did remember the -?
solution is copied from it, so perhaps it uses that as a hint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel I know enough to really gauge whether this is the right approach or not, so I'll leave that up to you.
I don't think GitHub allows me to make a neutral review after having requested changes, so just so that won't block you, I've approved it.
Thanks, let me merge it and try shipping it, if it doesn't work we have to revert and find another solution |
Fixed #1327