[runtimes] Add missing test dependencies to check-all #72955
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The test-depends target contained all the dependencies needed to run the runtimes tests, but it was never added as a dependency of check-all. This led to some of the tsan tests to fail, since the custom libcxx build they were using was never built. Fixes llvm#58680
|
I don't think this is correct, |
petrhosek
approved these changes
Nov 21, 2023
tru
pushed a commit
that referenced
this pull request
Nov 27, 2023
The test-depends target contained all the dependencies needed to run the
runtimes tests, but it was never added as a dependency of check-all.
This caused some of the tsan tests to fail, since the custom libcxx
build the tests were looking for was never built. Besides the tsan
failures, this fixes all the other test failures I was seeing with:
cmake -G Ninja -B release-build -S llvm \
-DCMAKE_POSITION_INDEPENDENT_CODE=ON \
-DCMAKE_BUILD_TYPE=Release \
-DLLVM_ENABLE_ASSERTIONS=OFF \
-DLLVM_ENABLE_PROJECTS="clang;lld" \
-DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;libunwind;compiler-rt"
This is the same configuration the test-release.sh script uses, so I'm
hoping this will also fix all the test failures we've been seeing when
building the releases.
Fixes #58680
(cherry picked from commit 7f215b1)
|
Heads up that we're seeing builds fail with "unknown target 'runtimes-test-depends'" which is most likely due to this. (https://crbug.com/1505361). I'll try to verify that and come up with some kind of reproducer. |
|
A reproducer on Linux: |
|
I'll revert until this can be fixed. We should probably revert from the release branch too. |
zmodem
added a commit
that referenced
this pull request
Nov 27, 2023
This caused some runtimes builds to fail with: error: unknown target 'runtimes-test-depends' See comments on the PR. > The test-depends target contained all the dependencies needed to run the > runtimes tests, but it was never added as a dependency of check-all. > This caused some of the tsan tests to fail, since the custom libcxx > build the tests were looking for was never built. Besides the tsan > failures, this fixes all the other test failures I was seeing with: > cmake -G Ninja -B release-build -S llvm \ > -DCMAKE_POSITION_INDEPENDENT_CODE=ON \ > -DCMAKE_BUILD_TYPE=Release \ > -DLLVM_ENABLE_ASSERTIONS=OFF \ > -DLLVM_ENABLE_PROJECTS="clang;lld" \ > -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;libunwind;compiler-rt" > > This is the same configuration the test-release.sh script uses, so I'm > hoping this will also fix all the test failures we've been seeing when > building the releases. > > Fixes #58680 This reverts commit 7f215b1.
|
@zmodem I think there are some steps missing in the reproducer script. Is the sdk supposed to be unpacked into the llvm-project directory? Where do I run the cmake invocation from? |
Sorry, I suppose the missing first line would be: "run the commands from a build directory under llvm-project/, e.g. llvm-project/build/" (or adjust the |
|
@zmodem I got it working now, thanks. |
|
It looks like this change uncovered an existing problem where the code in llvm/runtimes/CMakeLists.txt assumes that if LLVM_INCLUDE_TESTS=ON is set that tests will be enabled for the runtime builds too. However, this configuration breaks this assumption by passing -DRUNTIMES_aarch64-linux-android21_LLVM_INCLUDE_TESTS=OFF, which disables the tests for the runtime build. Even without this change, trying to use targets like 'ninja test-depends I think I can come up with a fix to make this patch work, but it seems like there may be some other problems here that haven't been uncovered yet. |
|
I can also take a look but I don't want to duplicate effort, @tstellar please let me know if you need help. |
|
@zmodem @petrhosek I have a fix here: #73610 If the fix is good, then I'll push it first and let it sit in main awhile before re-committing the reverted patch. |
tru
added a commit
that referenced
this pull request
Nov 28, 2023
This reverts commit e957e6d. The commit was reverted on main because of issues. We will not carry this in the release branch for 17.x
Guzhu-AMD
pushed a commit
to GPUOpen-Drivers/llvm-project
that referenced
this pull request
Nov 30, 2023
Local branch amd-gfx 83aac61 Merged main:f21a70f9fe21 into amd-gfx:41205cbd2f82 Remote branch main a9e3d23 Revert "[runtimes] Add missing test dependencies to check-all (llvm#72955)"
MingcongBai
pushed a commit
to AOSC-Tracking/llvm-project
that referenced
this pull request
Mar 26, 2024
The test-depends target contained all the dependencies needed to run the
runtimes tests, but it was never added as a dependency of check-all.
This caused some of the tsan tests to fail, since the custom libcxx
build the tests were looking for was never built. Besides the tsan
failures, this fixes all the other test failures I was seeing with:
cmake -G Ninja -B release-build -S llvm \
-DCMAKE_POSITION_INDEPENDENT_CODE=ON \
-DCMAKE_BUILD_TYPE=Release \
-DLLVM_ENABLE_ASSERTIONS=OFF \
-DLLVM_ENABLE_PROJECTS="clang;lld" \
-DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;libunwind;compiler-rt"
This is the same configuration the test-release.sh script uses, so I'm
hoping this will also fix all the test failures we've been seeing when
building the releases.
Fixes llvm#58680
(cherry picked from commit 7f215b1)
MingcongBai
pushed a commit
to AOSC-Tracking/llvm-project
that referenced
this pull request
Mar 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The test-depends target contained all the dependencies needed to run the runtimes tests, but it was never added as a dependency of check-all. This caused some of the tsan tests to fail, since the custom libcxx build the tests were looking for was never built. Besides the tsan failures, this fixes all the other test failures I was seeing with:
cmake -G Ninja -B release-build -S llvm
-DCMAKE_POSITION_INDEPENDENT_CODE=ON
-DCMAKE_BUILD_TYPE=Release
-DLLVM_ENABLE_ASSERTIONS=OFF
-DLLVM_ENABLE_PROJECTS="clang;lld"
-DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;libunwind;compiler-rt"
This is the same configuration the test-release.sh script uses, so I'm hoping this will also fix all the test failures we've been seeing when building the releases.
Fixes #58680