-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[lldb][test] Don't run libc++ API tests without a locally built libc++ #162657
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
Conversation
API tests in the `libc++` category will try their best to build against a locally built libc++. If none exists, the `Makefile.rules` currently fall back to using the system libc++. The issue with falling back to the system libc++ is that we are now potentially not testing what we intended to. But we also can't rely on certain libc++ features being available that the tests are trying to use. On Apple platforms this is a configuration error (because libc++ is the only stdlib supported), but we can't make it an error on Linux because a user might want to run the API tests with libstdc++. The Ubunutu 22.04 bots on the Apple fork are failing to run following tests are failing: * `TestLibcxxInternalsRecognizer.py` * `TestDataFormatterStdRangesRefView.py` because the system stdlib doesn't have `std::ranges` support yet. And the tests just fail to build. Building libc++ on those bots is also not possible because the system compiler is too old (and the Apple fork builds all the subprojects standalone, so it requires the system compiler). This patch marks tests in the `libc++` category as `UNSUPPORTED` if no local libc++ is available. The downside is that we will inevitably lose coverage on bots that were running these tests without a local libc++. Arguably those weren't really testing the right thing. But for vendors with LLDB forks it might have been useful to at least know that the tests on the fork don't fail against the system libc++.
✅ With the latest revision this PR passed the C/C++ code formatter. |
2ed5ef0
to
cc9b3e8
Compare
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesAPI tests in the The issue with falling back to the system libc++ is that we are now potentially not testing what we intended to. But we also can't rely on certain libc++ features being available that the tests are trying to use. On Apple platforms this is a configuration error (because libc++ is the only stdlib supported), but we can't make it an error on Linux because a user might want to run the API tests with libstdc++. The Ubunutu 22.04 bots on the Apple fork are failing to run following tests are failing:
This patch marks tests in the The downside is that we will inevitably lose coverage on bots that were running these tests without a local libc++. Arguably those weren't really testing the right thing. But for vendors with LLDB forks it might have been useful to at least know that the tests on the fork don't fail against the system libc++. rdar://136231390 Full diff: https://github.com/llvm/llvm-project/pull/162657.diff 1 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/dotest.py b/lldb/packages/Python/lldbsuite/test/dotest.py
index 2966ac04227cb..1d474a2180d22 100644
--- a/lldb/packages/Python/lldbsuite/test/dotest.py
+++ b/lldb/packages/Python/lldbsuite/test/dotest.py
@@ -789,6 +789,10 @@ def canRunLibcxxTests():
return True, "libc++ always present"
if platform == "linux":
+ if not configuration.libcxx_include_dir or not configuration.libcxx_library_dir:
+ return False, "API tests require a locally built libc++."
+
+ # Make sure -stdlib=libc++ works since that's how the tests will be built.
with temp_file.OnDiskTempFile() as f:
cmd = [configuration.compiler, "-xc++", "-stdlib=libc++", "-o", f.path, "-"]
p = subprocess.Popen(
|
Were there tests that are in that category that were successfully testing the Python libstdc++ data formatters that would now be no longer run? |
Nope those should be all under the libstdc++ test category |
Just to confirm, Linaro's bots were not running these tests. Not losing anything there. They do run the layout simulator tests though, these will continue to run I assume? I agree that having a way to opt-in with the category option is better than silently testing out of date libraries. Someone doing a standalone build might also use it. |
Thanks for confirming!
Yea they are not part of the
|
It shows 68 skipped, did you paste the wrong output? |
As in, I wanted to show that the simluators aren't part of the |
Also please add to the PR description a single line like:
In case the reader is a person who cared about system library compatibility. You kinda say what to do already but not in a Friday afternoon in a rush compatible way :) |
done! let me know if you want me to reword it |
Ok I see now. You enabled only that category, to show that no simulator test falls into it. Rather than disabling the category, to show that all the simulator tests ran (because how would I know exactly how many there are). 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
To confirm that means that, the |
Correct |
llvm#162657) API tests in the `libc++` category will try their best to build against a locally built libc++. If none exists, the `Makefile.rules` currently fall back to using the system libc++. The issue with falling back to the system libc++ is that we are now potentially not testing what we intended to. But we also can't rely on certain libc++ features being available that the tests are trying to use. On Apple platforms this is a configuration error (because libc++ is the only stdlib supported), but we can't make it an error on Linux because a user might want to run the API tests with libstdc++. The Ubunutu 22.04 bots on the Apple fork are failing to run following tests are failing: * `TestLibcxxInternalsRecognizer.py` * `TestDataFormatterStdRangesRefView.py` because the system stdlib doesn't have `std::ranges` support yet. And the tests just fail to build. Building libc++ on those bots is also not possible because the system compiler is too old (and the Apple fork builds all the subprojects standalone, so it requires the system compiler). This patch marks tests in the `libc++` category as `UNSUPPORTED` if no local libc++ is available. The downside is that we will inevitably lose coverage on bots that were running these tests without a local libc++. Arguably those weren't really testing the right thing. But for vendors with LLDB forks it might have been useful to at least know that the tests on the fork don't fail against the system libc++. Confirmed that the libc++ pre-merge CI still runs these tests (since it uses the explicit `--category libc++` dotest flag). Also confirmed that LLDB pre-merge CI runs the tests (because it builds `libcxx` locally). **Workaround** If you do need want to run libc++ tests against the system stdlib, you can invoke `lldb-dotest` with the `--category libc++` flag: ``` ./path/to/build/lldb-dotest --category libc++ OR ./path/to/build/bin/llvm-lit -sv --param dotest-args='--category libc++' "/path/to/monorepo/lldb/test/API ``` rdar://136231390 (cherry picked from commit 4188e18)
API tests in the
libc++
category will try their best to build against a locally built libc++. If none exists, theMakefile.rules
currently fall back to using the system libc++.The issue with falling back to the system libc++ is that we are now potentially not testing what we intended to. But we also can't rely on certain libc++ features being available that the tests are trying to use. On Apple platforms this is a configuration error (because libc++ is the only stdlib supported), but we can't make it an error on Linux because a user might want to run the API tests with libstdc++.
The Ubunutu 22.04 bots on the Apple fork are failing to run following tests are failing:
TestLibcxxInternalsRecognizer.py
TestDataFormatterStdRangesRefView.py
because the system stdlib doesn't havestd::ranges
support yet. And the tests just fail to build. Building libc++ on those bots is also not possible because the system compiler is too old (and the Apple fork builds all the subprojects standalone, so it requires the system compiler).This patch marks tests in the
libc++
category asUNSUPPORTED
if no local libc++ is available.The downside is that we will inevitably lose coverage on bots that were running these tests without a local libc++. Arguably those weren't really testing the right thing. But for vendors with LLDB forks it might have been useful to at least know that the tests on the fork don't fail against the system libc++.
Confirmed that the libc++ pre-merge CI still runs these tests (since it uses the explicit
--category libc++
dotest flag). Also confirmed that LLDB pre-merge CI runs the tests (because it buildslibcxx
locally).Workaround
If you do need want to run libc++ tests against the system stdlib, you can invoke
lldb-dotest
with the--category libc++
flag:rdar://136231390