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] add multi-platform pre-commit github actions #119104

Merged
merged 30 commits into from
Dec 9, 2024

Conversation

SchrodingerZhu
Copy link
Contributor

@SchrodingerZhu SchrodingerZhu commented Dec 8, 2024

This is a proof-of-concept for now.

We do not have CI coverage for Windows/MacOS and we regularly run into problem where changes break post-commit fullbuild which is not tested in pre-commit builds. This PR utilizes the github action to address such issues.

@llvmbot
Copy link
Member

llvmbot commented Dec 8, 2024

@llvm/pr-subscribers-github-workflow

Author: Schrodinger ZHU Yifan (SchrodingerZhu)

Changes

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

3 Files Affected:

  • (added) .github/workflows/libc-fullbuild-tests.yml ()
  • (added) .github/workflows/libc-gpu-build.yml ()
  • (added) .github/workflows/libc-overlay-tests.yml (+66)
diff --git a/.github/workflows/libc-fullbuild-tests.yml b/.github/workflows/libc-fullbuild-tests.yml
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/.github/workflows/libc-gpu-build.yml b/.github/workflows/libc-gpu-build.yml
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/.github/workflows/libc-overlay-tests.yml b/.github/workflows/libc-overlay-tests.yml
new file mode 100644
index 00000000000000..63732a37277886
--- /dev/null
+++ b/.github/workflows/libc-overlay-tests.yml
@@ -0,0 +1,66 @@
+# This workflow is for pre-commit testing of the LLVM-libc project.
+name: LLVM-libc Pre-commit Overlay Tests
+
+on:
+  pull_request:
+    branches: [ "main" ]
+    paths:
+      - 'libc/**'
+      - '.github/workflows/libc-overlay-tests.yml'
+
+jobs:
+  build:
+    env:
+      SCCACHE_GHA_ENABLED: "true"
+    runs-on: ${{ matrix.os }}
+    strategy:
+      # Set fail-fast to false to ensure that feedback is delivered for all matrix combinations.
+      fail-fast: false
+      matrix:
+        os: [ubuntu-latest, windows-latest, macos-latest]
+        build_type: [Release, Debug]
+        compiler: [
+          { c_compiler: gcc, cpp_compiler: g++ },
+          { c_compiler: clang, cpp_compiler: clang++ },
+          { c_compiler: clang-cl, cpp_compiler: clang-cl }
+        ]
+        exclude:
+          - os: windows-latest
+            compiler: { c_compiler: gcc, cpp_compiler: g++ }
+          - os: ubuntu-latest
+            compiler: { c_compiler: clang-cl, cpp_compiler: clang-cl }
+          - os: macos-latest
+            compiler: { c_compiler: clang-cl, cpp_compiler: clang-cl }
+          - os: macos-latest
+            compiler: { c_compiler: gcc, cpp_compiler: g++ }
+
+    steps:
+    - uses: actions/checkout@v4
+
+    - name: Run sccache-cache
+      uses: mozilla-actions/sccache-action@v0.0.6
+
+    - name: Set reusable strings
+      id: strings
+      shell: bash
+      run: |
+        echo "build-output-dir=${{ github.workspace }}/build" >> "$GITHUB_OUTPUT"
+
+    - name: Configure CMake
+      run: >
+        cmake -B ${{ steps.strings.outputs.build-output-dir }}
+        -DCMAKE_CXX_COMPILER=${{ matrix.compiler.cpp_compiler }}
+        -DCMAKE_C_COMPILER=${{ matrix.compiler.c_compiler }}
+        -DCMAKE_BUILD_TYPE=${{ matrix.build_type }}
+        -DCMAKE_C_COMPILER_LAUNCHER=sccache
+        -DCMAKE_CXX_COMPILER_LAUNCHER=sccache
+        -DLLVM_ENABLE_RUNTIMES=libc
+        -G Ninja
+        -S ${{ github.workspace }}/runtimes
+
+    - name: Build
+      run: cmake --build ${{ steps.strings.outputs.build-output-dir }}
+
+    - name: Test
+      working-directory: ${{ steps.strings.outputs.build-output-dir }}
+      run: ninja check-libc

.github/workflows/libc-fullbuild-tests.yml Outdated Show resolved Hide resolved
.github/workflows/libc-overlay-tests.yml Outdated Show resolved Hide resolved
.github/workflows/libc-overlay-tests.yml Outdated Show resolved Hide resolved
@SchrodingerZhu
Copy link
Contributor Author

Update branch to check caching.

@SchrodingerZhu
Copy link
Contributor Author

I can see that the cache writing run into errors but I don't know why. Perhaps github forbids some writes when update the cache too frequently. For the time being, I guess this is already the best caching effort I can do with sccache.

@tstellar
Copy link
Collaborator

tstellar commented Dec 8, 2024

I can see that the cache writing run into errors but I don't know why. Perhaps github forbids some writes when update the cache too frequently. For the time being, I guess this is already the best caching effort I can do with sccache.

I'm not sure what kind of errors you are seeing but for the other jobs we've been using a different action for managing the sccache and it's working pretty well: https://github.com/llvm/llvm-project/blob/main/.github/workflows/llvm-project-tests.yml#L93

@SchrodingerZhu
Copy link
Contributor Author

SchrodingerZhu commented Dec 8, 2024

I'm not sure what kind of errors you are seeing

Post job cleanup.
Get human-readable stats
Get JSON stats
Notice: 0% - 0 hits, 5527 misses, 0 errors
Full human-readable stats:
Compile requests                   5527
Compile requests executed          5527
Cache hits                            0
Cache misses                       5527
Cache misses (C/C++)               5527
Cache hits rate                    0.00 %
Cache hits rate (C/C++)            0.00 %
Cache timeouts                        0
Cache read errors                     0
Forced recaches                       0
Cache write errors                 5527
Compilation failures                  0
Cache errors                          0
Non-cacheable compilations            0
Non-cacheable calls                   0
Non-compilation calls                 0
Unsupported compiler calls            0
Average cache write               0.000 s
Average compiler                  0.942 s
Average cache read hit            0.000 s
Failed distributed compilations       0
Cache location                  ghac, name: sccache-v0.8.2, prefix: /sccache/
Version (client)                0.8.2

There is a Cache write errors 5527 in statistics.

I will try the other action later on.

Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

The windows testing should probably go into the traditional premerge pipeline where we have decently powerful machines available.

@SchrodingerZhu
Copy link
Contributor Author

The windows/macos testing are all light weight, even without caching, it finishes within 10 mins. The Linux one is a bit more problematic, as there are 9000+ testing units. But all of these finishes in 20mins.

@SchrodingerZhu
Copy link
Contributor Author

GHAC does not like to handle too many requests. I think this makes the mozilla/sccache ghac storage approach actually not a good fit for its purpose. Switching to the ccache action with sccache variant.

Copy link
Member

@nickdesaulniers nickdesaulniers left a comment

Choose a reason for hiding this comment

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

Cool! Mostly just some simple questions about the PR.

Comment on lines +20 to +22
# TODO: add back gcc build when it is fixed
# - c_compiler: gcc
# cpp_compiler: g++
Copy link
Member

Choose a reason for hiding this comment

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

Do you have more context about what's going wrong with the gcc builds?

Copy link
Contributor Author

@SchrodingerZhu SchrodingerZhu Dec 9, 2024

Choose a reason for hiding this comment

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

run: |
sudo apt-get update
sudo apt-get install -y libmpfr-dev libgmp-dev libmpc-dev ninja-build linux-headers-generic linux-libc-dev
sudo ln -sf /usr/include/$(uname -p)-linux-gnu/asm /usr/include/asm
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the symlink?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debian and its variants has a multilib sysroot, so arch-dependent headers are not installed into /usr/include/.

Ideally, libc should also add /usr/include/$(uname -p)-linux-gnu/ in its include directories but we do not do that.

cmake -B ${{ steps.strings.outputs.build-output-dir }}
-DCMAKE_CXX_COMPILER=${{ matrix.cpp_compiler }}
-DCMAKE_C_COMPILER=${{ matrix.c_compiler }}
-DCMAKE_BUILD_TYPE=MinSizeRel
Copy link
Member

Choose a reason for hiding this comment

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

Why MinSizeRel (vs. Release)?

Copy link
Contributor Author

@SchrodingerZhu SchrodingerZhu Dec 9, 2024

Choose a reason for hiding this comment

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

GHA has limited cache capability (10G in total). And we need to share it with other projects such as libcxx. Therefore, I decide to optimize for sizes.

- name: Prepare dependencies (Windows)
if: runner.os == 'Windows'
run: |
choco install ninja
Copy link
Member

Choose a reason for hiding this comment

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

What is choco?

https://github.com/chocolatey/choco

does that itself have to be installed, or does it ship on the github buildbots? I guess same question for brew on OSX; usually those don't come pre-installed IIRC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-DCMAKE_C_COMPILER_LAUNCHER=sccache
-DCMAKE_CXX_COMPILER_LAUNCHER=sccache
-DCMAKE_POLICY_DEFAULT_CMP0141=NEW
-DCMAKE_MSVC_DEBUG_INFORMATION_FORMAT=Embedded
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary for MinSizeRel or Release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-DCMAKE_BUILD_TYPE=MinSizeRel
-DCMAKE_C_COMPILER_LAUNCHER=sccache
-DCMAKE_CXX_COMPILER_LAUNCHER=sccache
-DCMAKE_POLICY_DEFAULT_CMP0141=NEW
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to set 141? Surely we can omit that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above, following the instructions from https://github.com/mozilla/sccache/tree/main?tab=readme-ov-file#usage

@SchrodingerZhu SchrodingerZhu merged commit f15cc6f into llvm:main Dec 9, 2024
10 checks passed
@SchrodingerZhu SchrodingerZhu deleted the libc/github-action branch December 9, 2024 21:04
@@ -0,0 +1,76 @@
# This workflow is for pre-commit testing of the LLVM-libc project.
name: LLVM-libc Pre-commit Fullbuild Tests

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add:

permissions:
  contents: read

To each new file you've added. This is standard best practices for GitHub actions definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the same level of name? Sure. Adding now

broxigarchen pushed a commit to broxigarchen/llvm-project that referenced this pull request Dec 10, 2024
We do not have CI coverage for Windows/MacOS and we regularly run into
problem where changes break post-commit fullbuild which is not tested in
pre-commit builds. This PR utilizes the github action to address such
issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants