-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[libc] Fix startup utilities failing to install in full build mode #82522
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
@llvm/pr-subscribers-libc Author: Joseph Huber (jhuber6) ChangesSummary: This patch solves this issue by making the This fixes an error I had where doing a runtimes failed to install its Full diff: https://github.com/llvm/llvm-project/pull/82522.diff 3 Files Affected:
diff --git a/libc/CMakeLists.txt b/libc/CMakeLists.txt
index 3d775736616745..616beae13d9aaa 100644
--- a/libc/CMakeLists.txt
+++ b/libc/CMakeLists.txt
@@ -225,6 +225,15 @@ else()
set(LIBC_INSTALL_INCLUDE_DIR ${CMAKE_INSTALL_INCLUDEDIR})
endif()
+if(LIBC_TARGET_TRIPLE)
+ set(LIBC_INSTALL_LIBRARY_DIR lib${LLVM_LIBDIR_SUFFIX}/${LIBC_TARGET_TRIPLE})
+elseif(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT LIBC_GPU_BUILD)
+ set(LIBC_INSTALL_LIBRARY_DIR
+ lib${LLVM_LIBDIR_SUFFIX}/${LLVM_DEFAULT_TARGET_TRIPLE})
+else()
+ set(LIBC_INSTALL_LIBRARY_DIR lib${LLVM_LIBDIR_SUFFIX})
+endif()
+
if(LIBC_TARGET_ARCHITECTURE_IS_GPU)
include(prepare_libc_gpu_build)
set(LIBC_ENABLE_UNITTESTS OFF)
diff --git a/libc/lib/CMakeLists.txt b/libc/lib/CMakeLists.txt
index af7ef2de93dd41..99f8c6a8be9da4 100644
--- a/libc/lib/CMakeLists.txt
+++ b/libc/lib/CMakeLists.txt
@@ -35,19 +35,11 @@ foreach(archive IN ZIP_LISTS
)
if(LLVM_LIBC_FULL_BUILD)
target_link_libraries(${archive_1} PUBLIC libc-headers)
+ add_dependencies(${archive_1} libc-startup)
endif()
list(APPEND added_archive_targets ${archive_1})
endforeach()
-if(LIBC_TARGET_TRIPLE)
- set(LIBC_INSTALL_LIBRARY_DIR lib${LLVM_LIBDIR_SUFFIX}/${LIBC_TARGET_TRIPLE})
-elseif(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT LIBC_GPU_BUILD)
- set(LIBC_INSTALL_LIBRARY_DIR
- lib${LLVM_LIBDIR_SUFFIX}/${LLVM_DEFAULT_TARGET_TRIPLE})
-else()
- set(LIBC_INSTALL_LIBRARY_DIR lib${LLVM_LIBDIR_SUFFIX})
-endif()
-
install(
TARGETS ${added_archive_targets}
ARCHIVE DESTINATION ${LIBC_INSTALL_LIBRARY_DIR}
diff --git a/libc/startup/linux/CMakeLists.txt b/libc/startup/linux/CMakeLists.txt
index 39bcca9cdba9fe..3837737d478a2a 100644
--- a/libc/startup/linux/CMakeLists.txt
+++ b/libc/startup/linux/CMakeLists.txt
@@ -131,7 +131,7 @@ foreach(target IN LISTS startup_components)
set(fq_target_name libc.startup.linux.${target})
add_dependencies(libc-startup ${fq_target_name})
install(FILES $<TARGET_OBJECTS:${fq_target_name}>
- DESTINATION ${CMAKE_INSTALL_LIBDIR}
+ DESTINATION ${LIBC_INSTALL_LIBRARY_DIR}
RENAME $<TARGET_PROPERTY:${fq_target_name},OUTPUT_NAME>
COMPONENT libc)
endforeach()
|
Summary: Currently, doing `ninja install` will fail in fullbuild mode due to the startup utilities not being built by default. This was hidden previously by the fact that if tests were run, it would build the startup utilities and thus they would be present. This patch solves this issue by making the `libc-startup` target a dependncy on the final library. Furthermore we simply factor out the library install directory into the base CMake directory next to the include directory handling. This change makes the `crt` files get installed in `lib/x86_64-unknown-linu-gnu` instead of just `lib`. This fixes an error I had where doing a runtimes failed to install its libraries because the install step always errored.
5b933fc
to
2dba88c
Compare
Also realized that actually installing the startup utilities breaks compiling with the compiler. I remember we previously hid these behind |
@@ -225,6 +225,15 @@ else() | |||
set(LIBC_INSTALL_INCLUDE_DIR ${CMAKE_INSTALL_INCLUDEDIR}) | |||
endif() | |||
|
|||
if(LIBC_TARGET_TRIPLE) |
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.
Can you comment a bit about the logic where (i.e. why checking TARGET_TRIPLE)
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.
That's the existing logic that's been moved, so it's not related to the patch. But LIBC_TARGET_TRIPLE
is set whenever building using https://libc.llvm.org/full_cross_build.html#runtimes-cross-build.
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.
The intention is to install these things in subdirectories, like libcxx
and libc
end up in lib/x86_64-unknown-linux-gnu
.
libc/lib/CMakeLists.txt
Outdated
@@ -35,19 +35,11 @@ foreach(archive IN ZIP_LISTS | |||
) | |||
if(LLVM_LIBC_FULL_BUILD) | |||
target_link_libraries(${archive_1} PUBLIC libc-headers) | |||
add_dependencies(${archive_1} libc-startup) |
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.
are there any fullbuild targets we support that don't have startup code yet? (E.g. risc-v linux?)
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.
I think linux
and gpu
are the only ones with startup code currently. But good catch, should be if TARGET
.
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, hopefully we can eventually remove this from EXCLUDE_FROM_ALL
but that will probably take a while
Summary:
Currently, doing
ninja install
will fail in fullbuild mode due to thestartup utilities not being built by default. This was hidden previously
by the fact that if tests were run, it would build the startup utilities
and thus they would be present.
This patch solves this issue by making the
libc-startup
target adependncy on the final library. Furthermore we simply factor out the
library install directory into the base CMake directory next to the
include directory handling. This change makes the
crt
files getinstalled in
lib/x86_64-unknown-linu-gnu
instead of justlib
.This fixes an error I had where doing a runtimes failed to install its
libraries because the install step always errored.