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][workflow] address permission concern and add more comments #119320

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

SchrodingerZhu
Copy link
Contributor

@SchrodingerZhu SchrodingerZhu commented Dec 10, 2024

This patch limits the permission of pre-commit libc pipelines. It also adds detailed comments to help future modifications.

@llvmbot
Copy link
Member

llvmbot commented Dec 10, 2024

@llvm/pr-subscribers-github-workflow

@llvm/pr-subscribers-libc

Author: Schrodinger ZHU Yifan (SchrodingerZhu)

Changes

This patch limits the permission of pre-commit libc pipelines. It also adds detailed comments for help future modifications.


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

3 Files Affected:

  • (modified) .github/workflows/libc-fullbuild-tests.yml (+16-3)
  • (modified) .github/workflows/libc-overlay-tests.yml (+16-2)
  • (modified) libc/src/__support/FPUtil/aarch64/FEnvImpl.h (+1-1)
diff --git a/.github/workflows/libc-fullbuild-tests.yml b/.github/workflows/libc-fullbuild-tests.yml
index fb73aa392f3433..b07e94244c2e8c 100644
--- a/.github/workflows/libc-fullbuild-tests.yml
+++ b/.github/workflows/libc-fullbuild-tests.yml
@@ -1,6 +1,7 @@
 # This workflow is for pre-commit testing of the LLVM-libc project.
 name: LLVM-libc Pre-commit Fullbuild Tests
-
+permissions:
+  contents: read
 on:
   pull_request:
     branches: [ "main" ]
@@ -22,7 +23,13 @@ jobs:
           #   cpp_compiler: g++
     steps:
     - uses: actions/checkout@v4
-
+    
+    # Libc's build is relatively small comparing with other components of LLVM.
+    # A fresh fullbuild takes about 190MiB of uncompressed disk space, which can
+    # be compressed into ~40MiB. Limiting the cache size to 1G should be enough.
+    # Prefer sccache as it is more modern.
+    # Do not use direct GHAC access even though it is supported by sccache. GHAC rejects
+    # frequent small object writes.
     - name: Setup ccache
       uses: hendrikmuhs/ccache-action@v1.2
       with:
@@ -30,6 +37,10 @@ jobs:
         key: libc_fullbuild_${{ matrix.c_compiler }}
         variant: sccache
     
+    # Notice:
+    # - MPFR is required by some of the mathlib tests.
+    # - Debian has a multilib setup, so we need to symlink the asm directory.
+    #   For more information, see https://wiki.debian.org/Multiarch/LibraryPathOverview
     - name: Prepare dependencies (Ubuntu)
       run: |
         sudo apt-get update
@@ -42,7 +53,9 @@ jobs:
       run: |
         echo "build-output-dir=${{ github.workspace }}/build" >> "$GITHUB_OUTPUT"
         echo "build-install-dir=${{ github.workspace }}/install" >> "$GITHUB_OUTPUT"
-
+    
+    # Configure libc fullbuild with scudo.
+    # Use MinSizeRel to reduce the size of the build.
     - name: Configure CMake
       run: >
         cmake -B ${{ steps.strings.outputs.build-output-dir }}
diff --git a/.github/workflows/libc-overlay-tests.yml b/.github/workflows/libc-overlay-tests.yml
index d0cdfdef99fe96..8b59d76aed4a88 100644
--- a/.github/workflows/libc-overlay-tests.yml
+++ b/.github/workflows/libc-overlay-tests.yml
@@ -1,6 +1,7 @@
 # This workflow is for pre-commit testing of the LLVM-libc project.
 name: LLVM-libc Pre-commit Overlay Tests
-
+permissions:
+  contents: read
 on:
   pull_request:
     branches: [ "main" ]
@@ -32,7 +33,14 @@ jobs:
     
     steps:
     - uses: actions/checkout@v4
-
+    
+    # Libc's build is relatively small comparing with other components of LLVM.
+    # A fresh linux overlay takes about 180MiB of uncompressed disk space, which can
+    # be compressed into ~40MiB. MacOS and Windows overlay builds are less than 10MiB
+    # after compression. Limiting the cache size to 1G should be enough.
+    # Prefer sccache as it is modern and it has a guarantee to work with MSVC.
+    # Do not use direct GHAC access even though it is supported by sccache. GHAC rejects
+    # frequent small object writes.
     - name: Setup ccache
       uses: hendrikmuhs/ccache-action@v1
       with:
@@ -40,12 +48,15 @@ jobs:
         key: libc_overlay_build_${{ matrix.os }}_${{ matrix.compiler.c_compiler }}
         variant: sccache
     
+    # MPFR is required by some of the mathlib tests.
     - name: Prepare dependencies (Ubuntu)
       if: runner.os == 'Linux'
       run: |
         sudo apt-get update
         sudo apt-get install -y libmpfr-dev libgmp-dev libmpc-dev ninja-build
     
+    # Chocolatey is shipped with Windows runners. Windows Server 2025 recommends WinGet.
+    # Consider migrating to WinGet when Windows Server 2025 is available.
     - name: Prepare dependencies (Windows)
       if: runner.os == 'Windows'
       run: |
@@ -62,6 +73,9 @@ jobs:
       run: |
         echo "build-output-dir=${{ github.workspace }}/build" >> "$GITHUB_OUTPUT"
 
+    # Use MinSizeRel to reduce the size of the build.
+    # Notice that CMP0141=NEW and MSVC_DEBUG_INFORMATION_FORMAT=Embedded are required
+    # by the sccache tool.
     - name: Configure CMake
       run: >
         cmake -B ${{ steps.strings.outputs.build-output-dir }}
diff --git a/libc/src/__support/FPUtil/aarch64/FEnvImpl.h b/libc/src/__support/FPUtil/aarch64/FEnvImpl.h
index 18b0631324f8fb..e41e7a2f5ba77e 100644
--- a/libc/src/__support/FPUtil/aarch64/FEnvImpl.h
+++ b/libc/src/__support/FPUtil/aarch64/FEnvImpl.h
@@ -59,7 +59,7 @@ struct FEnv {
            ((excepts & FE_UNDERFLOW) ? UNDERFLOW_F : 0) |
            ((excepts & FE_INEXACT) ? INEXACT_F : 0);
   }
-
+# 1 "/usr/include/x86_64-linux-gnu/bits/fcntl-linux.h" 1 3 4
   LIBC_INLINE static int exceptionStatusToMacro(uint32_t status) {
     return ((status & INVALID_F) ? FE_INVALID : 0) |
            ((status & DIVBYZERO_F) ? FE_DIVBYZERO : 0) |

@SchrodingerZhu SchrodingerZhu force-pushed the libc/workflow/provision branch from f68ae15 to 301dc4a Compare December 10, 2024 04:11
Copy link
Collaborator

@tstellar tstellar left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@SchrodingerZhu SchrodingerZhu merged commit bd231da into llvm:main Dec 10, 2024
10 checks passed
@SchrodingerZhu SchrodingerZhu deleted the libc/workflow/provision branch December 10, 2024 14:37
broxigarchen pushed a commit to broxigarchen/llvm-project that referenced this pull request Dec 10, 2024
…vm#119320)

This patch limits the permission of pre-commit libc pipelines. It also
adds detailed comments to help future modifications.
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.

3 participants