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

[libc++] Use GitHub-provided runners for the windows CI #79326

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

philnik777
Copy link
Contributor

@philnik777 philnik777 commented Jan 24, 2024

Co-authored-by: Martin Storsjö martin@martin.st

@philnik777 philnik777 force-pushed the try_gh_runners branch 21 times, most recently from 3445df8 to 2f5a9d2 Compare January 25, 2024 21:32
@philnik777
Copy link
Contributor Author

@mstorsjo Could you take a look here? I've tried a bunch of stuff, but couldn't really get things to run properly.

@mstorsjo
Copy link
Member

@mstorsjo Could you take a look here? I've tried a bunch of stuff, but couldn't really get things to run properly.

Sure, I can try to set this up. I have tested running libcxx tests on Windows GitHub Actions runners before, and got it working before (for a subset of configs, not using the libcxx run-buildbot script) - I'll see what I can do.

@mstorsjo
Copy link
Member

@mstorsjo Could you take a look here? I've tried a bunch of stuff, but couldn't really get things to run properly.

Sure, I can try to set this up. I have tested running libcxx tests on Windows GitHub Actions runners before, and got it working before (for a subset of configs, not using the libcxx run-buildbot script) - I'll see what I can do.

I got it working for all of our Windows CI configs. See the two topmost commits at https://github.com/mstorsjo/llvm-project/commits/libcxx-gh-win-runners. The one setting up the actions stuff is quite straightforward. The other one fixes a seriously annoying issue in how CMake picks python, please read the commit message thoroughly.

Also, running the libcxx tests on Github runners used to be quite annoying as they used to be quite limited (2 cores, 8 GB memory only), so earlier, I've had to set -DLIBCXX_EXTRA_SITE_DEFINES="TEST_IS_EXECUTED_IN_A_SLOW_ENVIRONMENT" and -DLIBCXX_TEST_PARAMS="large_tests=False" to make it work reliably - see https://github.com/mstorsjo/llvm-mingw/blob/cfe2ac2783dfd86a7e9ae81aba2f5db3f1f64133/.github/workflows/test-libcxx.yml#L97-L98. But with the new runners it seems to run well without that (although I haven't run all that many rounds yet, so perhaps it fails sporadically still, who knows?).

@philnik777 philnik777 force-pushed the try_gh_runners branch 3 times, most recently from 3b4bf6e to 0439ebc Compare January 26, 2024 18:55
@philnik777 philnik777 marked this pull request as ready for review January 26, 2024 19:19
@philnik777 philnik777 requested a review from a team as a code owner January 26, 2024 19:19
@llvmbot llvmbot added the cmake Build system in general and CMake in particular label Jan 26, 2024
@llvmbot llvmbot added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. github:workflow labels Jan 26, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 26, 2024

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/79326.diff

4 Files Affected:

  • (modified) .github/workflows/libcxx-build-and-test.yaml (+41)
  • (added) libcxx/trigger ()
  • (modified) libcxx/utils/ci/buildkite-pipeline.yml (-50)
  • (modified) runtimes/CMakeLists.txt (+3)
diff --git a/.github/workflows/libcxx-build-and-test.yaml b/.github/workflows/libcxx-build-and-test.yaml
index 5727b956dc6dd2..3682ab6f61386a 100644
--- a/.github/workflows/libcxx-build-and-test.yaml
+++ b/.github/workflows/libcxx-build-and-test.yaml
@@ -199,3 +199,44 @@ jobs:
             **/CMakeError.log
             **/CMakeOutput.log
             **/crash_diagnostics/*
+  windows:
+    runs-on: windows-2022
+    strategy:
+      fail-fast: false
+      matrix:
+        include:
+        - { config: clang-cl-dll, mingw: false }
+        - { config: clang-cl-static, mingw: false }
+        - { config: clang-cl-no-vcruntime, mingw: false }
+        - { config: clang-cl-debug, mingw: false }
+        - { config: clang-cl-static-crt, mingw: false }
+        - { config: mingw-dll, mingw: true }
+        - { config: mingw-static, mingw: true }
+        - { config: mingw-dll-i686, mingw: true }
+    steps:
+      - uses: actions/checkout@v4
+      - name: Install dependencies
+        run: |
+          choco install -y ninja wget
+          pip install psutil
+      - name: Install a current LLVM
+        if: ${{ matrix.mingw != true }}
+        run: |
+          choco install -y llvm --version=17.0.6
+      - name: Install llvm-mingw
+        if: ${{ matrix.mingw == true }}
+        run: |
+          curl -LO https://github.com/mstorsjo/llvm-mingw/releases/download/20231128/llvm-mingw-20231128-ucrt-x86_64.zip
+          powershell Expand-Archive llvm-mingw*.zip -DestinationPath .
+          del llvm-mingw*.zip
+          mv llvm-mingw* c:\llvm-mingw
+          echo "c:\llvm-mingw\bin" | Out-File -FilePath $Env:GITHUB_PATH -Encoding utf8 -Append
+      - name: Add Git Bash to the path
+        run: |
+          echo "c:\Program Files\Git\usr\bin" | Out-File -FilePath $Env:GITHUB_PATH -Encoding utf8 -Append
+      - name: Set up the MSVC dev environment
+        if: ${{ matrix.mingw != true }}
+        uses: ilammy/msvc-dev-cmd@v1
+      - name: Build and test
+        run: |
+          bash libcxx/utils/ci/run-buildbot ${{ matrix.config }}
diff --git a/libcxx/trigger b/libcxx/trigger
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/libcxx/utils/ci/buildkite-pipeline.yml b/libcxx/utils/ci/buildkite-pipeline.yml
index a7c44dab709391..e42262620d5fb0 100644
--- a/libcxx/utils/ci/buildkite-pipeline.yml
+++ b/libcxx/utils/ci/buildkite-pipeline.yml
@@ -57,56 +57,6 @@ environment_definitions:
 
 
 steps:
-- group: ':windows: Windows'
-  steps:
-  - label: Clang-cl (DLL)
-    command: bash libcxx/utils/ci/run-buildbot clang-cl-dll
-    agents:
-      queue: windows
-    <<: *common
-
-  - label: Clang-cl (Static)
-    command: bash libcxx/utils/ci/run-buildbot clang-cl-static
-    agents:
-      queue: windows
-    <<: *common
-
-  - label: Clang-cl (no vcruntime exceptions)
-    command: bash libcxx/utils/ci/run-buildbot clang-cl-no-vcruntime
-    <<: *common
-    agents:
-      queue: windows
-
-  - label: Clang-cl (Debug mode)
-    command: bash libcxx/utils/ci/run-buildbot clang-cl-debug
-    agents:
-      queue: windows
-    <<: *common
-
-  - label: Clang-cl (Static CRT)
-    command: bash libcxx/utils/ci/run-buildbot clang-cl-static-crt
-    agents:
-      queue: windows
-    <<: *common
-
-  - label: MinGW (DLL, x86_64)
-    command: bash libcxx/utils/ci/run-buildbot mingw-dll
-    agents:
-      queue: windows
-    <<: *common
-
-  - label: MinGW (Static, x86_64)
-    command: bash libcxx/utils/ci/run-buildbot mingw-static
-    agents:
-      queue: windows
-    <<: *common
-
-  - label: MinGW (DLL, i686)
-    command: bash libcxx/utils/ci/run-buildbot mingw-dll-i686
-    agents:
-      queue: windows
-    <<: *common
-
 - group: ':mac: Apple'
   steps:
   - label: MacOS x86_64
diff --git a/runtimes/CMakeLists.txt b/runtimes/CMakeLists.txt
index 742334328fd32a..634ffe710b06b8 100644
--- a/runtimes/CMakeLists.txt
+++ b/runtimes/CMakeLists.txt
@@ -155,6 +155,9 @@ set(LLVM_COMPILER_CHECKED ON)
 include(AddLLVM)
 include(HandleLLVMOptions)
 
+# Loot at the PATH first to avoid a version mismatch between the command-line
+# python and the CMake-found version
+set(Python3_FIND_REGISTRY LAST)
 find_package(Python3 REQUIRED COMPONENTS Interpreter)
 
 # Host triple is used by tests to check if they are running natively.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 26, 2024

@llvm/pr-subscribers-github-workflow

Author: Nikolas Klauser (philnik777)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/79326.diff

4 Files Affected:

  • (modified) .github/workflows/libcxx-build-and-test.yaml (+41)
  • (added) libcxx/trigger ()
  • (modified) libcxx/utils/ci/buildkite-pipeline.yml (-50)
  • (modified) runtimes/CMakeLists.txt (+3)
diff --git a/.github/workflows/libcxx-build-and-test.yaml b/.github/workflows/libcxx-build-and-test.yaml
index 5727b956dc6dd23..3682ab6f61386a4 100644
--- a/.github/workflows/libcxx-build-and-test.yaml
+++ b/.github/workflows/libcxx-build-and-test.yaml
@@ -199,3 +199,44 @@ jobs:
             **/CMakeError.log
             **/CMakeOutput.log
             **/crash_diagnostics/*
+  windows:
+    runs-on: windows-2022
+    strategy:
+      fail-fast: false
+      matrix:
+        include:
+        - { config: clang-cl-dll, mingw: false }
+        - { config: clang-cl-static, mingw: false }
+        - { config: clang-cl-no-vcruntime, mingw: false }
+        - { config: clang-cl-debug, mingw: false }
+        - { config: clang-cl-static-crt, mingw: false }
+        - { config: mingw-dll, mingw: true }
+        - { config: mingw-static, mingw: true }
+        - { config: mingw-dll-i686, mingw: true }
+    steps:
+      - uses: actions/checkout@v4
+      - name: Install dependencies
+        run: |
+          choco install -y ninja wget
+          pip install psutil
+      - name: Install a current LLVM
+        if: ${{ matrix.mingw != true }}
+        run: |
+          choco install -y llvm --version=17.0.6
+      - name: Install llvm-mingw
+        if: ${{ matrix.mingw == true }}
+        run: |
+          curl -LO https://github.com/mstorsjo/llvm-mingw/releases/download/20231128/llvm-mingw-20231128-ucrt-x86_64.zip
+          powershell Expand-Archive llvm-mingw*.zip -DestinationPath .
+          del llvm-mingw*.zip
+          mv llvm-mingw* c:\llvm-mingw
+          echo "c:\llvm-mingw\bin" | Out-File -FilePath $Env:GITHUB_PATH -Encoding utf8 -Append
+      - name: Add Git Bash to the path
+        run: |
+          echo "c:\Program Files\Git\usr\bin" | Out-File -FilePath $Env:GITHUB_PATH -Encoding utf8 -Append
+      - name: Set up the MSVC dev environment
+        if: ${{ matrix.mingw != true }}
+        uses: ilammy/msvc-dev-cmd@v1
+      - name: Build and test
+        run: |
+          bash libcxx/utils/ci/run-buildbot ${{ matrix.config }}
diff --git a/libcxx/trigger b/libcxx/trigger
new file mode 100644
index 000000000000000..e69de29bb2d1d64
diff --git a/libcxx/utils/ci/buildkite-pipeline.yml b/libcxx/utils/ci/buildkite-pipeline.yml
index a7c44dab7093911..e42262620d5fb01 100644
--- a/libcxx/utils/ci/buildkite-pipeline.yml
+++ b/libcxx/utils/ci/buildkite-pipeline.yml
@@ -57,56 +57,6 @@ environment_definitions:
 
 
 steps:
-- group: ':windows: Windows'
-  steps:
-  - label: Clang-cl (DLL)
-    command: bash libcxx/utils/ci/run-buildbot clang-cl-dll
-    agents:
-      queue: windows
-    <<: *common
-
-  - label: Clang-cl (Static)
-    command: bash libcxx/utils/ci/run-buildbot clang-cl-static
-    agents:
-      queue: windows
-    <<: *common
-
-  - label: Clang-cl (no vcruntime exceptions)
-    command: bash libcxx/utils/ci/run-buildbot clang-cl-no-vcruntime
-    <<: *common
-    agents:
-      queue: windows
-
-  - label: Clang-cl (Debug mode)
-    command: bash libcxx/utils/ci/run-buildbot clang-cl-debug
-    agents:
-      queue: windows
-    <<: *common
-
-  - label: Clang-cl (Static CRT)
-    command: bash libcxx/utils/ci/run-buildbot clang-cl-static-crt
-    agents:
-      queue: windows
-    <<: *common
-
-  - label: MinGW (DLL, x86_64)
-    command: bash libcxx/utils/ci/run-buildbot mingw-dll
-    agents:
-      queue: windows
-    <<: *common
-
-  - label: MinGW (Static, x86_64)
-    command: bash libcxx/utils/ci/run-buildbot mingw-static
-    agents:
-      queue: windows
-    <<: *common
-
-  - label: MinGW (DLL, i686)
-    command: bash libcxx/utils/ci/run-buildbot mingw-dll-i686
-    agents:
-      queue: windows
-    <<: *common
-
 - group: ':mac: Apple'
   steps:
   - label: MacOS x86_64
diff --git a/runtimes/CMakeLists.txt b/runtimes/CMakeLists.txt
index 742334328fd32af..634ffe710b06b8e 100644
--- a/runtimes/CMakeLists.txt
+++ b/runtimes/CMakeLists.txt
@@ -155,6 +155,9 @@ set(LLVM_COMPILER_CHECKED ON)
 include(AddLLVM)
 include(HandleLLVMOptions)
 
+# Loot at the PATH first to avoid a version mismatch between the command-line
+# python and the CMake-found version
+set(Python3_FIND_REGISTRY LAST)
 find_package(Python3 REQUIRED COMPONENTS Interpreter)
 
 # Host triple is used by tests to check if they are running natively.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

TBH I think this LGTM if the CI passes.

I do expect that we'll hit resource limitations pretty quickly, but I guess we can cross that bridge when we get to it. This might entail setting up self-hosted runners for Windows.

@@ -199,3 +199,44 @@ jobs:
**/CMakeError.log
**/CMakeOutput.log
**/crash_diagnostics/*
windows:
Copy link
Member

Choose a reason for hiding this comment

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

When we're ready to merge this, we should add needs: [ stage1, stage2, stage3 ] to only trigger the Windows CI if the other stages are happy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather only wait for stage1, since the GitHub-provided agents are quite slow. I think it's quite unlikely that we'll actually hit resource limits, since we have up to 60 concurrent agents available, i.e. more than 7 libc++ patches uploaded in an hour that don't fail on stage1. We can still put that higher later if we do hit limitations at some point.

@tstellar
Copy link
Collaborator

Can you limit the total number of jobs to 40? We need to have some spare capacity for running some of the other jobs, like the PR labeler, code_formatter, etc. We may be able to bump it higher in the future, but I think 40 is a good start.

@tstellar tstellar self-requested a review January 26, 2024 19:43
@tstellar
Copy link
Collaborator

tstellar commented Jan 26, 2024

Can you limit the total number of jobs to 40? We need to have some spare capacity for running some of the other jobs, like the PR labeler, code_formatter, etc. We may be able to bump it higher in the future, but I think 40 is a good start.

I thought this was possible, but I haven't found a way to actually limit the number of jobs. I f you know how to implement this then that is great, but otherwise do you have an estimate of the number of concurrent machines you will need to handle the peak load?

@philnik777
Copy link
Contributor Author

Can you limit the total number of jobs to 40? We need to have some spare capacity for running some of the other jobs, like the PR labeler, code_formatter, etc. We may be able to bump it higher in the future, but I think 40 is a good start.

I thought this was possible, but I haven't found a way to actually limit the number of jobs. I f you know how to implement this then that is great, but otherwise do you have an estimate of the number of concurrent machines you will need to handle the peak load?

Hm, yeah. I thought it could be done with max-parallel:, but that only limits the parallelism per PR. I'd expect a maximum of ~10 runs/hour for libc++, which would be 80 runners. That would only be the case once in a blue moon though. A lot of the work on libc++ is done off-peak and we generally don't throw out a bunch of PRs at once. If this actually becomes a problem we could also consider moving only some of the jobs to GitHub or limiting the number of parallel jobs to 4/PR.

@tstellar
Copy link
Collaborator

@philnik777 OK, let's just keep it as-is and see how it goes.

@philnik777 philnik777 merged commit 82afd9b into llvm:main Jan 26, 2024
8 of 9 checks passed
@philnik777 philnik777 deleted the try_gh_runners branch January 26, 2024 20:53
@@ -155,6 +155,9 @@ set(LLVM_COMPILER_CHECKED ON)
include(AddLLVM)
include(HandleLLVMOptions)

# Loot at the PATH first to avoid a version mismatch between the command-line
# python and the CMake-found version
set(Python3_FIND_REGISTRY LAST)
Copy link
Member

Choose a reason for hiding this comment

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

FYI - ideally we wouldn't need to have things like this in the main CMakeLists.txt.

Ideally we'd set it in the CI environment, as we happen to need it there due to how the Github runners are set up. It is possible to pass this on the CMake command line, I think, like -DPython3_FIND_REGISTRY=LAST, but the run-buildbot script doesn't support passing extra command line parameters to cmake.

On the other hand - this doesn't really hurt either; the cmake default behaviour is really really surprising here, and this makes it less surprising. It makes things a little inconsistent to keep setting this in the runtimes/CMakeLists.txt only - I wonder if we could set it e.g. cmake/Modules/CMakePolicy.cmake as a sort of project-wide default policy?

Anyway, just venting my thoughts on the matter - no urgent action needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Build system in general and CMake in particular github:workflow libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants