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

[libcxx] [ci] Add a test configuration with an incomplete sysroot #107089

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

mstorsjo
Copy link
Member

@mstorsjo mstorsjo commented Sep 3, 2024

When bringing up a new cross compiler from scratch, we build libunwind/libcxx in a setup where the toolchain is incomplete and unable to perform the normal linker checks; this requires a few special cases in the CMake files.

We simulate that scenario by removing the libc++ headers, libunwind and libc++ libraries from the installed toolchain.

We need to set CMAKE_CXX_COMPILER_WORKS since CMake fails to probe the compiler. We need to set CMAKE_CXX_COMPILER_TARGET, since LLVM's heuristics fail when CMake hasn't been able to probe the environment properly. (This is normal; one has to set those options when setting up such a toolchain from scratch.)

This adds CI coverage for these build scenarios, which otherwise seldom are tested by some build flow (but are essential when setting up a cross compiler from scratch).

When bringing up a new cross compiler from scratch, we build
libunwind/libcxx in a setup where the toolchain is incomplete and
unable to perform the normal linker checks; this requires a few
special cases in the CMake files.

We simulate that scenario by removing the libc++ headers, libunwind
and libc++ libraries from the installed toolchain.

We need to set CMAKE_CXX_COMPILER_WORKS since CMake fails to
probe the compiler. We need to set CMAKE_CXX_COMPILER_TARGET,
since LLVM's heuristics fail when CMake hasn't been able to
probe the environment properly. (This is normal; one has to
set those options when setting up such a toolchain from scratch.)

This adds CI coverage for these build scenarios, which otherwise
seldom are tested by some build flow (but are essential when setting
up a cross compiler from scratch).
@mstorsjo mstorsjo requested a review from a team as a code owner September 3, 2024 11:24
@llvmbot llvmbot added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. github:workflow labels Sep 3, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 3, 2024

@llvm/pr-subscribers-libcxx

@llvm/pr-subscribers-github-workflow

Author: Martin Storsjö (mstorsjo)

Changes

When bringing up a new cross compiler from scratch, we build libunwind/libcxx in a setup where the toolchain is incomplete and unable to perform the normal linker checks; this requires a few special cases in the CMake files.

We simulate that scenario by removing the libc++ headers, libunwind and libc++ libraries from the installed toolchain.

We need to set CMAKE_CXX_COMPILER_WORKS since CMake fails to probe the compiler. We need to set CMAKE_CXX_COMPILER_TARGET, since LLVM's heuristics fail when CMake hasn't been able to probe the environment properly. (This is normal; one has to set those options when setting up such a toolchain from scratch.)

This adds CI coverage for these build scenarios, which otherwise seldom are tested by some build flow (but are essential when setting up a cross compiler from scratch).


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

2 Files Affected:

  • (modified) .github/workflows/libcxx-build-and-test.yaml (+7)
  • (modified) libcxx/utils/ci/run-buildbot (+23)
diff --git a/.github/workflows/libcxx-build-and-test.yaml b/.github/workflows/libcxx-build-and-test.yaml
index 1a26a699db8e01..b5e60781e00064 100644
--- a/.github/workflows/libcxx-build-and-test.yaml
+++ b/.github/workflows/libcxx-build-and-test.yaml
@@ -242,6 +242,7 @@ jobs:
         - { config: mingw-dll, mingw: true }
         - { config: mingw-static, mingw: true }
         - { config: mingw-dll-i686, mingw: true }
+        - { config: mingw-incomplete-sysroot, mingw: true }
     steps:
       - uses: actions/checkout@v4
       - name: Install dependencies
@@ -260,6 +261,12 @@ jobs:
           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: Simulate a from-scratch build of llvm-mingw
+        if: ${{ matrix.config == 'mingw-incomplete-sysroot' }}
+        run: |
+          rm -r c:\llvm-mingw\include\c++
+          rm -r c:\llvm-mingw\*-w64-mingw32\lib\libc++*
+          rm -r c:\llvm-mingw\*-w64-mingw32\lib\libunwind*
       - name: Add Git Bash to the path
         run: |
           echo "c:\Program Files\Git\usr\bin" | Out-File -FilePath $Env:GITHUB_PATH -Encoding utf8 -Append
diff --git a/libcxx/utils/ci/run-buildbot b/libcxx/utils/ci/run-buildbot
index 102f1669e63b84..1387353d2a0f17 100755
--- a/libcxx/utils/ci/run-buildbot
+++ b/libcxx/utils/ci/run-buildbot
@@ -715,6 +715,29 @@ mingw-dll-i686)
           -C "${MONOREPO_ROOT}/libcxx/cmake/caches/MinGW.cmake"
     check-runtimes
 ;;
+mingw-incomplete-sysroot)
+    # When bringing up a new cross compiler from scratch, we build
+    # libunwind/libcxx in a setup where the toolchain is incomplete and
+    # unable to perform the normal linker checks; this requires a few
+    # special cases in the CMake files.
+    #
+    # Building in an incomplete setup requires setting CMAKE_*_COMPILER_WORKS,
+    # as CMake fails to probe the compiler. This case also requires
+    # setting CMAKE_CXX_COMPILER_TARGET, as LLVM's heuristics for setting
+    # the triple fails when CMake hasn't been able to probe the environment.
+    # (This is what one has to do when building the initial libunwind/libcxx
+    # for a new toolchain.)
+    clean
+    generate-cmake \
+          -DCMAKE_C_COMPILER_WORKS=TRUE \
+          -DCMAKE_CXX_COMPILER_WORKS=TRUE \
+          -DCMAKE_C_COMPILER_TARGET=x86_64-w64-windows-gnu \
+          -DCMAKE_CXX_COMPILER_TARGET=x86_64-w64-windows-gnu \
+          -C "${MONOREPO_ROOT}/libcxx/cmake/caches/MinGW.cmake"
+    # Only test that building succeeds; there's not much extra value in running
+    # the tests here, as it would be equivalent to the mingw-dll config above.
+    ${NINJA} -vC "${BUILD_DIR}"
+;;
 aix)
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/AIX.cmake" \

@mstorsjo
Copy link
Member Author

mstorsjo commented Sep 3, 2024

This was earlier posted for review in https://reviews.llvm.org/D150766.

Since that version, the setup of this test environment has gotten simpler, since we're running the Windows tests on GitHub Actions runners, where each run is in a separate clean container, where we're installing the toolchain ourselves. So it's easier to unconditionally remove things from the toolchain, to properly simulate the incomplete sysroot setup.

The previous review asked whether this really is Windows specific - it isn't. It should certainly be possible to have the same situation in a Linux environment too. But exactly which files to remove from a full working environment, in order to simulate this from-scratch bringup stage, is quite environment specific. It's much easier to do with a cross compiler that come with their own sysroot, rather than a native toolchain that uses whatever it can find in /usr etc. (The Windows based mingw toolchains have their sysroots bundled just like cross compilers, even if it's not technically cross compilation.)

So to test the same on Linux, you'd probably need to have a standalone cross toolchain setup (targeting e.g. musl or so?). And we don't have that readily available, as far as I know. While this testing setup uses the exact same tooling we're already using, and gives at least some sort of test coverage for this important scenario.

Right now, there are discussions in #90332 about things that do touch upon the from-scratch incomplete sysroot cases - having even some sort of test coverage of that in the CI setup would be valuable.

CC @petrhosek

Comment on lines +732 to +735
-DCMAKE_C_COMPILER_WORKS=TRUE \
-DCMAKE_CXX_COMPILER_WORKS=TRUE \
-DCMAKE_C_COMPILER_TARGET=x86_64-w64-windows-gnu \
-DCMAKE_CXX_COMPILER_TARGET=x86_64-w64-windows-gnu \
Copy link
Member

Choose a reason for hiding this comment

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

As part of (or as a prerequisite to) this patch, shouldn't we fix these "bugs" so that we don't have to work around them here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don’t consider this a bug that reasonably can be fixed.

At this stage, we can’t even link a trivial C application (the toolchain implicitly tries to link libunwind, which is yet to be built), so the cmake tests that normally deduce the target triple simply fall flat.

Copy link
Member Author

Choose a reason for hiding this comment

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

And as for needing to pass the *_COMPILER_WORKS flag, that’s also required to get cmake pass the very first sanity checks. It is possible to avoid that by setting CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC_LIBRARY, but that gives false positives on all further checks for whether libraries exist.

We’ve looked into reducing the number of extra flags one has to set when doing the initial build, but these couple flags are the ones that the caller do need to set in such situations. Trying to get rid of it isn’t worth it - we’ve tried.

@ldionne ldionne merged commit 5024dff into llvm:main Sep 5, 2024
63 checks passed
VitaNuo pushed a commit to VitaNuo/llvm-project that referenced this pull request Sep 12, 2024
…m#107089)

When bringing up a new cross compiler from scratch, we build
libunwind/libcxx in a setup where the toolchain is incomplete and unable
to perform the normal linker checks; this requires a few special cases
in the CMake files.

We simulate that scenario by removing the libc++ headers, libunwind and
libc++ libraries from the installed toolchain.

We need to set CMAKE_CXX_COMPILER_WORKS since CMake fails to probe the
compiler. We need to set CMAKE_CXX_COMPILER_TARGET, since LLVM's
heuristics fail when CMake hasn't been able to probe the environment
properly. (This is normal; one has to set those options when setting up
such a toolchain from scratch.)

This adds CI coverage for these build scenarios, which otherwise seldom
are tested by some build flow (but are essential when setting up a cross
compiler from scratch).
@mstorsjo mstorsjo deleted the libcxx-ci-mingw-incomplete-sysroot branch October 24, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants