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.

Sign up for GitHub

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

Jump to bottom

Fix ml path for Windows clang-cl cc toolchain #23337

Closed
wants to merge 2 commits into from
Closed

Fix ml path for Windows clang-cl cc toolchain #23337

wants to merge 2 commits into from

Conversation

michaelsiegrist
Copy link
Contributor

Assembly files are not valid inputs for clang-cl.exe; the MSVC ml64.exe must be used instead.

Fixes #23128.

@michaelsiegrist
Fix ml path for Windows clang-cl cc toolchain
102ef44 
Assembly files are not valid inputs for `clang-cl.exe`; the MSVC
`ml64.exe` must be used instead.
@github-actions github-actions bot added team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels Aug 19, 2024
@fmeum
Copy link
Collaborator

fmeum commented Aug 19, 2024

@meteorcloudy

@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally labels Aug 20, 2024
@meteorcloudy
Copy link
Member

Do you mind adding a simple test for this at https://cs.opensource.google/bazel/bazel/+/master:src/test/py/bazel/bazel_windows_cpp_test.py;l=766?

@meteorcloudy meteorcloudy added the awaiting-user-response Awaiting a response from the author label Aug 20, 2024
@michaelsiegrist
Copy link
Contributor Author

Sure, I added a .S and .asm to that test, which I copied from bazel_windows_test.py. I haven't managed to get the test to run locally myself (I think I'm doing something wrong with my MSYS2 setup), but I think it should work.

@meteorcloudy
Copy link
Member

I don't think it's covering what we want to test, the clang-cl test cases didn't use the default project, see https://cs.opensource.google/bazel/bazel/+/master:src/test/py/bazel/bazel_windows_cpp_test.py;l=790-794

I also manually ran this test on a Windows machine, it passed without the windows_cc_configure.bzl change.

@michaelsiegrist
Copy link
Contributor Author

Sorry, my mistake. I've moved the functionality to the proper test now. I'm still not able to get the tests to run locally (looks like the test harness isn't finding my MSVC installation for some reason). However, I was able to manually create a test project with the same file contents and confirmed that it failed with 7.3.1 but passed with my changes.

meteorcloudy
meteorcloudy approved these changes Aug 22, 2024
View reviewed changes
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-user-response Awaiting a response from the author labels Aug 22, 2024
@fmeum
Copy link
Collaborator

fmeum commented Aug 22, 2024

@bazel-io fork 7.4.0

@bazel-io bazel-io mentioned this pull request Aug 22, 2024
Closed
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 22, 2024
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Aug 22, 2024
@michaelsiegrist @iancha1992
Fix ml path for Windows clang-cl cc toolchain
bbc4e22 
Assembly files are not valid inputs for `clang-cl.exe`; the MSVC `ml64.exe` must be used instead.

Fixes bazelbuild#23128.

Closes bazelbuild#23337.

PiperOrigin-RevId: 666406544
Change-Id: Ia7a5fc4702f08a5754145ca286c079d1a4f0e204
@iancha1992 iancha1992 mentioned this pull request Aug 22, 2024
Merged
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Aug 23, 2024
@michaelsiegrist @iancha1992
Fix ml path for Windows clang-cl cc toolchain
ec77b9c 
Assembly files are not valid inputs for `clang-cl.exe`; the MSVC `ml64.exe` must be used instead.

Fixes bazelbuild#23128.

Closes bazelbuild#23337.

PiperOrigin-RevId: 666406544
Change-Id: Ia7a5fc4702f08a5754145ca286c079d1a4f0e204
@iancha1992 iancha1992 mentioned this pull request Aug 23, 2024
Closed
github-merge-queue bot pushed a commit that referenced this pull request Aug 28, 2024
@iancha1992 @michaelsiegrist
[7.4.0] Fix ml path for Windows clang-cl cc toolchain (#23406)
cb1bc28 
Assembly files are not valid inputs for `clang-cl.exe`; the MSVC
`ml64.exe` must be used instead.

Fixes #23128.

Closes #23337.

PiperOrigin-RevId: 666406544
Change-Id: Ia7a5fc4702f08a5754145ca286c079d1a4f0e204

Commit
e5a083d

---------

Co-authored-by: Michael Siegrist <[email protected]>
@meteorcloudy meteorcloudy mentioned this pull request Oct 25, 2024
Closed
fhanau added a commit to fhanau/rules_cc that referenced this pull request Nov 11, 2024
@fhanau
Use clang-cl assembler in clang-cl toolchain again
c5a0333 
This essentially reverts bazelbuild/bazel#23337 now that the toolchain
configuration has been moved here. clang-cl does support assembly just like
other clang variants and unlike MASM supports assembly using native AT&T syntax
as with regular clang. MSVC-style assembly requires using MASM, which continues
to be available in the MSVC toolchain.

This has previously been discussed in bazelbuild/bazel#24152 and reverted for
Bazel 7.4.1 in bazelbuild/bazel#24211. Ideally, we'd want to have some mechanism
to make assembler selection configurable and support several kinds of asm syntax
in the future.
@fhanau fhanau mentioned this pull request Nov 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meteorcloudy meteorcloudy meteorcloudy approved these changes

Assignees
No one assigned
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong assembler selected for @local_config_cc//:cc-toolchain-x64_windows-clang-cl
3 participants
@michaelsiegrist @fmeum @meteorcloudy