[libcxx] [ci] Make the CI find the right version of Clang-cl #161736
Conversation
llvmbot
added
libc++
|
@llvm/pr-subscribers-libcxx @llvm/pr-subscribers-github-workflow Author: Martin Storsjö (mstorsjo) ChangesWe install a (or in practice, update a preexisting) copy of Clang, in order to get a specific version that we want. At that point in the procedure, this is the only version of Clang in the PATH. However, the step of adding the MSVC build tools to the environment also ends up adding another copy of Clang, bundled with MSVC, into the PATH. Due to this, CMake ends up finding and preferring the older version of Clang bundled with MSVC, rather than the one we intend to be used. Manually add the directory of the version of Clang we want to use at the head of the search path, after initializing the MSVC build tool environment. The directory name we add is a hardcoded guess of where it is installed - this is not ideal, but seems like the most straightforward solution for now. Full diff: https://github.com/llvm/llvm-project/pull/161736.diff 1 Files Affected:
diff --git a/.github/workflows/libcxx-build-and-test.yaml b/.github/workflows/libcxx-build-and-test.yaml
index 1c07a0adc6e99..77f79a85a0a2f 100644
--- a/.github/workflows/libcxx-build-and-test.yaml
+++ b/.github/workflows/libcxx-build-and-test.yaml
@@ -281,6 +281,10 @@ jobs:
- name: Set up the MSVC dev environment
if: ${{ matrix.mingw != true }}
uses: ilammy/msvc-dev-cmd@0b201ec74fa43914dc39ae48a89fd1d8cb592756 # v1.13.0
+ - name: Add the installed Clang at the start of the path
+ if: ${{ matrix.mingw != true }}
+ run: |
+ echo "c:\Program Files\LLVM\bin" | Out-File -FilePath $Env:GITHUB_PATH -Encoding utf8 -Append
- name: Build and test
run: |
bash libcxx/utils/ci/run-buildbot ${{ matrix.config }}
|
|
Unfortunately, this change can't be landed as is; this breaks the The issue seems to be that Clang in 20.x has added more AVX10 intrinsics headers, and these use unreserved parameter names such as Action point to Clang: CC @erichkeane (feel free to delegate) and @phoebewang - This is at least the second case when the Intel intrinsics headers expose this kind of issue. Can Clang set up some kind of similar test as libcxx has, to make sure we don't add more similar issues in the Clang bundled resource headers (in particular, the intrinsics headers which are a pain to wade through). The libcxx CI is already on Clang 20.x for the mingw build configurations (and likewise for the Linux test configurations, I would presume), but those don't seem to pull in the Clang intrinsics headers. The fact that intrinsics headers are included here seems to be a specific quirk in the MS UCRT headers; since version 10.0.26100.0 of those headers, Aside from this, it seems like at least the @philnik777 As Clang 20.x (and early 21.x) intrinsics headers seem to be broken wrt this, what's the best way forward here do you think? Explicitly skip this test for e.g. |
|
I filed #161808 for the new issue in the Clang headers. |
… msvc Clang 20 (and early 21 versions; let's hope it can be fixed before the later versions before such versions become relevant for libcxx CI) have got an issue with its intrinsics headers, where they use unreserved names, that users are allowed to override. See llvm#161808 for the issue report. This only crops up in the MSVC build configurations, as recent versions of some MSVC/UCRT headers include `<intrin.h>`, which ends up pulling in most intrinsics headers, exposing this issue in the Clang headers. This should unblock llvm#161736 from being merged.
|
@mstorsjo sorry for the inconvenience and thanks for the fix!
I think fe42d34 may be another reason, we include all intrinsics after it. Does it mean we have covered it? |
I don't think that commit is related here. It's in 21.x only, and I was hitting this on 20.x. And that commit is a no-op if
I don't understand what you mean by this question here. |
I mean with fe42d34, these intrinsics should be tested on Windows environment since they are included through |
That commit does not make any difference - it is unrelated. It removed ifdefs like |
Oh, I see. I mixed up with removing _MSC_VER. That change was in 20.x already. |
… msvc (#161811) Clang 20 (and early 21 versions; let's hope it can be fixed before the later versions before such versions become relevant for libcxx CI) have got an issue with its intrinsics headers, where they use unreserved names, that users are allowed to override. See #161808 for the issue report. This only crops up in the MSVC build configurations, as recent versions of some MSVC/UCRT headers include `<intrin.h>`, which ends up pulling in most intrinsics headers, exposing this issue in the Clang headers. This should unblock #161736 from being merged.
We install a (or in practice, update a preexisting) copy of Clang, in order to get a specific version that we want. At that point in the procedure, this is the only version of Clang in the PATH. However, the step of adding the MSVC build tools to the environment also ends up adding another copy of Clang, bundled with MSVC, into the PATH. Due to this, CMake ends up finding and preferring the older version of Clang bundled with MSVC, rather than the one we intend to be used. Manually add the directory of the version of Clang we want to use at the head of the search path, after initializing the MSVC build tool environment. The directory name we add is a hardcoded guess of where it is installed - this is not ideal, but seems like the most straightforward solution for now.
mstorsjo
force-pushed
the
libcxx-ci-clang-path
branch
from
99d68f9 to
36b1451
Compare
…clang-20 && msvc (#161811) Clang 20 (and early 21 versions; let's hope it can be fixed before the later versions before such versions become relevant for libcxx CI) have got an issue with its intrinsics headers, where they use unreserved names, that users are allowed to override. See llvm/llvm-project#161808 for the issue report. This only crops up in the MSVC build configurations, as recent versions of some MSVC/UCRT headers include `<intrin.h>`, which ends up pulling in most intrinsics headers, exposing this issue in the Clang headers. This should unblock llvm/llvm-project#161736 from being merged.
That did indeed seem to be a fluke; now all the Windows builds succeeded again - so I'll merge this, to unblock #134330. |
We install a (or in practice, update a preexisting) copy of Clang, in order to get a specific version that we want. At that point in the procedure, this is the only version of Clang in the PATH.
However, the step of adding the MSVC build tools to the environment also ends up adding another copy of Clang, bundled with MSVC, into the PATH.
Due to this, CMake ends up finding and preferring the older version of Clang bundled with MSVC, rather than the one we intend to be used.
Manually add the directory of the version of Clang we want to use at the head of the search path, after initializing the MSVC build tool environment.
The directory name we add is a hardcoded guess of where it is installed - this is not ideal, but seems like the most straightforward solution for now.