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++] Cherry-pick the disabling of modules tests onto release/18.x #85247

Closed

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Mar 14, 2024

LIT was never really meant to generate tests during discovery, and we probably shouldn't be doing this.

This hack is even worse than the initial attempt because it buries the "UNSUPPORTED" at the bottom of the test.

Fixes #85242

LIT was never really meant to generate tests during discovery,
and we probably shouldn't be doing this.

This hack is even worse than the initial attempt because it buries the
"UNSUPPORTED" at the bottom of the test.
@ldionne ldionne added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Mar 14, 2024
@ldionne ldionne added this to the LLVM 18.X Release milestone Mar 14, 2024
@ldionne ldionne requested a review from a team as a code owner March 14, 2024 15:32
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 14, 2024

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

LIT was never really meant to generate tests during discovery, and we probably shouldn't be doing this.

This hack is even worse than the initial attempt because it buries the "UNSUPPORTED" at the bottom of the test.

Fixes #85242


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

2 Files Affected:

  • (modified) libcxx/test/libcxx/module_std.gen.py (+5)
  • (modified) libcxx/test/libcxx/module_std_compat.gen.py (+5)
diff --git a/libcxx/test/libcxx/module_std.gen.py b/libcxx/test/libcxx/module_std.gen.py
index a9a05a0cd74e61..3bc6ddbd882031 100644
--- a/libcxx/test/libcxx/module_std.gen.py
+++ b/libcxx/test/libcxx/module_std.gen.py
@@ -16,7 +16,11 @@
 # to be one monolitic test. Since the test doesn't take very long it's
 # not a huge issue.
 
+# WARNING: Disabled at the bottom. Fix this test and remove the UNSUPPORTED line
+# TODO: Re-enable this test once we understand why it keeps timing out.
+
 # RUN: %{python} %s %{libcxx}/utils
+# END.
 
 import sys
 
@@ -35,4 +39,5 @@
 
 
 print("//--- module_std.sh.cpp")
+print('// UNSUPPORTED: clang')
 generator.write_test("std")
diff --git a/libcxx/test/libcxx/module_std_compat.gen.py b/libcxx/test/libcxx/module_std_compat.gen.py
index 270d131779e5bf..c1e74eb379c8ab 100644
--- a/libcxx/test/libcxx/module_std_compat.gen.py
+++ b/libcxx/test/libcxx/module_std_compat.gen.py
@@ -16,7 +16,11 @@
 # to be one monolitic test. Since the test doesn't take very long it's
 # not a huge issue.
 
+# WARNING: Disabled at the bottom. Fix this test and remove the UNSUPPORTED line
+# TODO: Re-enable this test once we understand why it keeps timing out.
+
 # RUN: %{python} %s %{libcxx}/utils
+# END.
 
 import sys
 
@@ -36,6 +40,7 @@
 
 
 print("//--- module_std_compat.sh.cpp")
+print("// UNSUPPORTED: clang")
 generator.write_test(
     "std.compat",
     module_c_headers,

@mordante
Copy link
Member

I'd rather fix the clang-tidy integration. I'm quite sure we have ODR violations since we use clang-tidy 18 with clang 17 libraries. These violations break the modules, but other clang-tidy checks may also have issues. Changing https://github.com/llvm/llvm-project/blob/release/18.x/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt#L8 to 18.1 likely fixes the issue.

I forgot about the release branch when I fixed this in main.

@ldionne
Copy link
Member Author

ldionne commented Mar 14, 2024

@mordante So should I close this PR?

@ldionne ldionne closed this Mar 14, 2024
@ldionne ldionne deleted the review/disable-modules-tests-18 branch March 14, 2024 19:45
@mordante
Copy link
Member

@mordante So should I close this PR?

I see you already did. Do you want to make a fix for the release branch or do you want me to pick that up?

@ldionne
Copy link
Member Author

ldionne commented Mar 14, 2024

Is the fix only to switch to 18.1 in the cmake? I can do that if that's it, I just don't fully understand the situation w/ clang tidy ODR violations since you were the one to make these changes

@mordante
Copy link
Member

Is the fix only to switch to 18.1 in the cmake? I can do that if that's it, I just don't fully understand the situation w/ clang tidy ODR violations since you were the one to make these changes

That should be all. I'm also happy to do it, but that will be tomorrow.

@ldionne
Copy link
Member Author

ldionne commented Mar 14, 2024

No worries! I just created #85305, hopefully that should do the trick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants