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

Reapply "[libc++abi] Stop copying headers to the build directory" #115379

Conversation

arichardson
Copy link
Member

@arichardson arichardson commented Nov 7, 2024

This was needed before #115077
since the compiler-rt test build made assumptions about the build
layout of libc++ and libc++abi, but now they link against a local
installation of these libraries so we no longer need this workaround.

The last attempt at landing this was reverted due to buildbot failures
which should be fixed by llvm/llvm-zorg#299.

Created using spr 1.3.6-beta.1
@arichardson arichardson requested a review from a team as a code owner November 7, 2024 22:19
@llvmbot llvmbot added the libc++abi libc++abi C++ Runtime Library. Not libc++. label Nov 7, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 7, 2024

@llvm/pr-subscribers-libcxxabi

Author: Alexander Richardson (arichardson)

Changes

This was needed before #115077
since the compiler-rt test build made assumptions about the build
layout of libc++ and libc++abi, but now they link against a local
installation of these libraries so we no longer need this workaround.


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

2 Files Affected:

  • (modified) libcxxabi/CMakeLists.txt (-6)
  • (modified) libcxxabi/include/CMakeLists.txt (-13)
diff --git a/libcxxabi/CMakeLists.txt b/libcxxabi/CMakeLists.txt
index da0e8b286cddc14..50e9a296a4a13b3 100644
--- a/libcxxabi/CMakeLists.txt
+++ b/libcxxabi/CMakeLists.txt
@@ -86,12 +86,6 @@ set(LIBCXXABI_STATIC_OUTPUT_NAME "c++abi" CACHE STRING "Output name for the stat
 
 set(LIBCXXABI_INSTALL_INCLUDE_DIR "${CMAKE_INSTALL_INCLUDEDIR}/c++/v1" CACHE STRING "Path to install the libc++abi headers at.")
 
-if(LLVM_LIBRARY_OUTPUT_INTDIR)
-  set(LIBCXXABI_GENERATED_INCLUDE_DIR "${LLVM_BINARY_DIR}/include/c++/v1")
-else()
-  set(LIBCXXABI_GENERATED_INCLUDE_DIR "${CMAKE_BINARY_DIR}/include/c++/v1")
-endif()
-
 set(LIBCXXABI_LIBCXX_LIBRARY_PATH "" CACHE PATH "The path to libc++ library.")
 set(LIBCXXABI_LIBRARY_VERSION "1.0" CACHE STRING
 "Version of libc++abi. This will be reflected in the name of the shared \
diff --git a/libcxxabi/include/CMakeLists.txt b/libcxxabi/include/CMakeLists.txt
index 5b1cc2545016ec2..0deb7b1eb9e7151 100644
--- a/libcxxabi/include/CMakeLists.txt
+++ b/libcxxabi/include/CMakeLists.txt
@@ -3,20 +3,7 @@ set(files
   cxxabi.h
   )
 
-foreach(f ${files})
-  set(src "${CMAKE_CURRENT_SOURCE_DIR}/${f}")
-  set(dst "${LIBCXXABI_GENERATED_INCLUDE_DIR}/${f}")
-  add_custom_command(OUTPUT ${dst}
-    DEPENDS ${src}
-    COMMAND ${CMAKE_COMMAND} -E copy_if_different ${src} ${dst}
-    COMMENT "Copying CXXABI header ${f}")
-  list(APPEND _all_includes "${dst}")
-endforeach()
-
-add_custom_target(generate-cxxabi-headers ALL DEPENDS ${_all_includes})
-
 add_library(cxxabi-headers INTERFACE)
-add_dependencies(cxxabi-headers generate-cxxabi-headers)
 target_include_directories(cxxabi-headers INTERFACE "${CMAKE_CURRENT_SOURCE_DIR}")
 
 if (LIBCXXABI_INSTALL_HEADERS)

arichardson added a commit to llvm/llvm-zorg that referenced this pull request Nov 7, 2024
The build tree layout does not necessarily match a valid libc++
installation tree, so install libc++/libc++abi first.

This is needed for llvm/llvm-project#115379

Reviewed By: vitalybuka

Pull Request: #299
Created using spr 1.3.6-beta.1
@arichardson arichardson merged commit fd799ad into main Nov 7, 2024
4 of 6 checks passed
@arichardson arichardson deleted the users/arichardson/spr/reapply-libcabi-stop-copying-headers-to-the-build-directory branch November 7, 2024 22:50
@petrhosek
Copy link
Member

petrhosek commented Nov 11, 2024

This change broke an existing invariant that the build tree is usable as a toolchain without needing to run an install step because the include/c++/v1 directory no longer contains libc++abi headers after this change.

We rely on that assumptions in multi-stage builds where stage N+1 is using toolchain from stage N (without running the install step on stage N), but after this change the stage N output is no longer usable. On the Fuchsia side, this broke all our multi-stage builders.

Can we please revert this change? We'll need to find a different solution for #115077 that doesn't break the existing invariant.

arichardson added a commit that referenced this pull request Nov 12, 2024
…ory"" (#115793)

Reverts #115379

Reverting since this broke the Fuchsia builders.
@ldionne
Copy link
Member

ldionne commented Nov 12, 2024

FWIW I think the right solution here is for the multi-stage build to install libc++ / libc++abi headers to a temporary location and use those headers. We'd hit the same issue if we started e.g. generating a header only in the installation tree but not in the build tree.

Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024
This was needed before llvm#115077
since the compiler-rt test build made assumptions about the build
layout of libc++ and libc++abi, but now they link against a local
installation of these libraries so we no longer need this workaround.

The last attempt at landing this was reverted due to buildbot failures
which should be fixed by llvm/llvm-zorg#299.

Pull Request: llvm#115379
Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024
…ory"" (llvm#115793)

Reverts llvm#115379

Reverting since this broke the Fuchsia builders.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++abi libc++abi C++ Runtime Library. Not libc++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants