Skip to content

[libc++] Remove Windows-specific configuration from libcxx/test/CMakeLists.txt #96330

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

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Jun 21, 2024

This is essentially a revert of 9853e9b which tried removing duplication in the Windows config files by moving it to the CMake. However, we want to decouple the CMake and the test suite as much as possible, so encoding additional (non-official) Lit parameters in the CMake only as a code reuse mechanism is not an approach we want to take.

…Lists.txt

This is essentially a revert of 9853e9b which tried removing duplication
in the Windows config files by moving it to the CMake. However, we want
to decouple the CMake and the test suite as much as possible, so encoding
additional (non-official) Lit parameters in the CMake only as a code reuse
mechanism is not an approach we want to take.
@ldionne ldionne requested a review from mstorsjo June 21, 2024 16:48
@ldionne ldionne requested a review from a team as a code owner June 21, 2024 16:48
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jun 21, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 21, 2024

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

This is essentially a revert of 9853e9b which tried removing duplication in the Windows config files by moving it to the CMake. However, we want to decouple the CMake and the test suite as much as possible, so encoding additional (non-official) Lit parameters in the CMake only as a code reuse mechanism is not an approach we want to take.


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

4 Files Affected:

  • (modified) libcxx/test/CMakeLists.txt (-25)
  • (modified) libcxx/test/configs/llvm-libc++-shared-clangcl.cfg.in (+16-2)
  • (modified) libcxx/test/configs/llvm-libc++-shared-no-vcruntime-clangcl.cfg.in (+16-2)
  • (modified) libcxx/test/configs/llvm-libc++-static-clangcl.cfg.in (+16-2)
diff --git a/libcxx/test/CMakeLists.txt b/libcxx/test/CMakeLists.txt
index 3c54a4edccff3..cdd1c2d90fbcf 100644
--- a/libcxx/test/CMakeLists.txt
+++ b/libcxx/test/CMakeLists.txt
@@ -34,31 +34,6 @@ endif()
 
 serialize_lit_params_list(SERIALIZED_LIT_PARAMS LIBCXX_TEST_PARAMS)
 
-if (MSVC)
-  # Shared code for initializing some parameters used by all
-  # llvm-libc++-*-clangcl.cfg.in test configs.
-  set(dbg_include "")
-
-  if (NOT CMAKE_MSVC_RUNTIME_LIBRARY OR CMAKE_MSVC_RUNTIME_LIBRARY MATCHES "DLL$")
-    set(fms_runtime_lib "dll")
-    set(cxx_lib "msvcprt")
-  else()
-    set(fms_runtime_lib "static")
-    set(cxx_lib "libcpmt")
-  endif()
-
-  if ((NOT CMAKE_MSVC_RUNTIME_LIBRARY AND uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")
-      OR (CMAKE_MSVC_RUNTIME_LIBRARY MATCHES "Debug"))
-    set(dbg_include " -include set_windows_crt_report_mode.h")
-    set(fms_runtime_lib "${fms_runtime_lib}_dbg")
-    set(cxx_lib "${cxx_lib}d")
-  endif()
-
-  serialize_lit_string_param(SERIALIZED_LIT_PARAMS dbg_include "${dbg_include}")
-  serialize_lit_string_param(SERIALIZED_LIT_PARAMS fms_runtime_lib "${fms_runtime_lib}")
-  serialize_lit_string_param(SERIALIZED_LIT_PARAMS cxx_lib "${cxx_lib}")
-endif()
-
 if (LIBCXX_INCLUDE_TESTS)
   include(AddLLVM) # for configure_lit_site_cfg and add_lit_testsuite
 
diff --git a/libcxx/test/configs/llvm-libc++-shared-clangcl.cfg.in b/libcxx/test/configs/llvm-libc++-shared-clangcl.cfg.in
index 163123fffb752..cffe895f653e3 100644
--- a/libcxx/test/configs/llvm-libc++-shared-clangcl.cfg.in
+++ b/libcxx/test/configs/llvm-libc++-shared-clangcl.cfg.in
@@ -3,12 +3,26 @@
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
+dbg_include = ''
+runtime_library = '@CMAKE_MSVC_RUNTIME_LIBRARY@'
+if runtime_library == '' or runtime_library.endswith('DLL'):
+    fms_runtime_lib = 'dll'
+    cxx_lib = 'msvcprt'
+else:
+    fms_runtime_lib = 'static'
+    cxx_lib = 'libcpmt'
+
+if '@CMAKE_BUILD_TYPE@'.upper() == 'DEBUG':
+    dbg_include = ' -D_DEBUG -include set_windows_crt_report_mode.h'
+    fms_runtime_lib += '_dbg'
+    cxx_lib += 'd'
+
 config.substitutions.append(('%{flags}', '--driver-mode=g++'))
 config.substitutions.append(('%{compile_flags}',
-    '-fms-runtime-lib=' + config.fms_runtime_lib + ' -nostdinc++ -I %{target-include-dir} -I %{include-dir} -I %{libcxx-dir}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX' + config.dbg_include
+    '-fms-runtime-lib=' + fms_runtime_lib + ' -nostdinc++ -I %{target-include-dir} -I %{include-dir} -I %{libcxx-dir}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX' + dbg_include
 ))
 config.substitutions.append(('%{link_flags}',
-    '-nostdlib -L %{lib-dir} -lc++ -l' + config.cxx_lib
+    '-nostdlib -L %{lib-dir} -lc++ -l' + cxx_lib
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T --prepend_env PATH=%{lib-dir} -- '
diff --git a/libcxx/test/configs/llvm-libc++-shared-no-vcruntime-clangcl.cfg.in b/libcxx/test/configs/llvm-libc++-shared-no-vcruntime-clangcl.cfg.in
index a8ad920897e61..80c7c0ee669ae 100644
--- a/libcxx/test/configs/llvm-libc++-shared-no-vcruntime-clangcl.cfg.in
+++ b/libcxx/test/configs/llvm-libc++-shared-no-vcruntime-clangcl.cfg.in
@@ -4,12 +4,26 @@
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
+dbg_include = ''
+runtime_library = '@CMAKE_MSVC_RUNTIME_LIBRARY@'
+if runtime_library == '' or runtime_library.endswith('DLL'):
+    fms_runtime_lib = 'dll'
+    cxx_lib = 'msvcprt'
+else:
+    fms_runtime_lib = 'static'
+    cxx_lib = 'libcpmt'
+
+if '@CMAKE_BUILD_TYPE@'.upper() == 'DEBUG':
+    dbg_include = ' -D_DEBUG -include set_windows_crt_report_mode.h'
+    fms_runtime_lib += '_dbg'
+    cxx_lib += 'd'
+
 config.substitutions.append(('%{flags}', '--driver-mode=g++'))
 config.substitutions.append(('%{compile_flags}',
-    '-fms-runtime-lib=' + config.fms_runtime_lib + ' -nostdinc++ -I %{include-dir} -I %{target-include-dir} -I %{libcxx-dir}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -D_HAS_EXCEPTIONS=0' + config.dbg_include
+    '-fms-runtime-lib=' + fms_runtime_lib + ' -nostdinc++ -I %{include-dir} -I %{target-include-dir} -I %{libcxx-dir}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -D_HAS_EXCEPTIONS=0' + dbg_include
 ))
 config.substitutions.append(('%{link_flags}',
-    '-nostdlib -L %{lib-dir} -lc++ -l' + config.cxx_lib
+    '-nostdlib -L %{lib-dir} -lc++ -l' + cxx_lib
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T --prepend_env PATH=%{lib-dir} -- '
diff --git a/libcxx/test/configs/llvm-libc++-static-clangcl.cfg.in b/libcxx/test/configs/llvm-libc++-static-clangcl.cfg.in
index 58ca3f27c713b..4b6b3fcf2d9c8 100644
--- a/libcxx/test/configs/llvm-libc++-static-clangcl.cfg.in
+++ b/libcxx/test/configs/llvm-libc++-static-clangcl.cfg.in
@@ -3,12 +3,26 @@
 
 lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
 
+dbg_include = ''
+runtime_library = '@CMAKE_MSVC_RUNTIME_LIBRARY@'
+if runtime_library == '' or runtime_library.endswith('DLL'):
+    fms_runtime_lib = 'dll'
+    cxx_lib = 'msvcprt'
+else:
+    fms_runtime_lib = 'static'
+    cxx_lib = 'libcpmt'
+
+if '@CMAKE_BUILD_TYPE@'.upper() == 'DEBUG':
+    dbg_include = ' -D_DEBUG -include set_windows_crt_report_mode.h'
+    fms_runtime_lib += '_dbg'
+    cxx_lib += 'd'
+
 config.substitutions.append(('%{flags}', '--driver-mode=g++'))
 config.substitutions.append(('%{compile_flags}',
-    '-fms-runtime-lib=' + config.fms_runtime_lib + ' -nostdinc++ -I %{target-include-dir} -I %{include-dir} -I %{libcxx-dir}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX' + config.dbg_include
+    '-fms-runtime-lib=' + fms_runtime_lib + ' -nostdinc++ -I %{target-include-dir} -I %{include-dir} -I %{libcxx-dir}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX' + dbg_include
 ))
 config.substitutions.append(('%{link_flags}',
-    '-nostdlib -L %{lib-dir} -llibc++ -l' + config.cxx_lib
+    '-nostdlib -L %{lib-dir} -llibc++ -l' + cxx_lib
 ))
 config.substitutions.append(('%{exec}',
     '%{executor} --execdir %T -- '

Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

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

Ok with me. This change was requested by others involved in the review at the time, but I don't mind going this way either.

@ldionne ldionne merged commit c393121 into llvm:main Jun 25, 2024
53 of 59 checks passed
@ldionne ldionne deleted the review/revert-windows-cmake-bridge branch June 25, 2024 04:33
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
…Lists.txt (llvm#96330)

This is essentially a revert of 9853e9b which tried removing duplication
in the Windows config files by moving it to the CMake. However, we want
to decouple the CMake and the test suite as much as possible, so
encoding additional (non-official) Lit parameters in the CMake only as a
code reuse mechanism is not an approach we want to take.
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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants