-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[CMake] Support Macros in Linux #68082
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
swiftlang/swift-syntax#2090 |
add_custom_command( | ||
OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/HeaderDependencies.cpp" | ||
COMMAND ${CMAKE_COMMAND} -E copy_if_different | ||
"${CMAKE_CURRENT_BINARY_DIR}/HeaderDependencies.cpp.tmp" | ||
"${CMAKE_CURRENT_BINARY_DIR}/HeaderDependencies.cpp" | ||
) |
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.
This is to avoid mtime of HeaderDependencies.cpp
being updated every time cmake
configuration happens.
cmake/modules/AddSwift.cmake
Outdated
|
||
# At runtime, use swiftCore in the current toolchain. | ||
# FIXME: This assumes the ABI hasn't changed since the builder. | ||
set(swift_runtime_rpath "$ORIGIN/${relpath_to_lib_dir}/swift/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}") | ||
|
||
if(ASRLF_BOOTSTRAPPING_MODE STREQUAL "HOSTTOOLS") | ||
set(swift_runtime_rpath "${host_lib_dir}") | ||
else() | ||
set(swift_runtime_rpath "$ORIGIN/${relpath_to_lib_dir}/swift/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}") | ||
# But before building the stdlib with the tool, use the builder libs. This should be removed in install time. | ||
list(APPEND swift_runtime_rpath "${host_lib_dir}") |
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.
RUNPATH
is now like $ORIGIN/../lib/swift/linux:/opt/swift/5.8.1/usr/lib/swift/linux
So after the swiftCore
is built in the build directory, that's used.
@@ -89,7 +111,8 @@ function(add_swift_unittest test_dirname) | |||
endif() | |||
endif() | |||
|
|||
if (SWIFT_SWIFT_PARSER) | |||
if (SWIFT_SWIFT_PARSER AND NOT ASU_IS_TARGET_TEST) | |||
# Link to stdlib the compiler uses. |
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.
e.g. runtime
tests is a "target" test, not "host" test. "target" test should not link with host libraries.
@@ -6,7 +6,7 @@ function(swift_supports_implicit_module module_name out_var) | |||
"${CMAKE_Swift_COMPILER}" | |||
-Xfrontend -disable-implicit-${module_name}-module-import | |||
-Xfrontend -parse-stdlib | |||
-c - -o /dev/null | |||
-parse - |
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.
Linux swiftc
reads the output file after compiling (for e.g. autolink-extract). Since /dev/null
is not a valid object file, this failed.
All we want to check here is if the frontend accepts the command line option, so -parse
should be enough.
set(SWIFT_SYNTAX_LIBRARIES_SOURCE_DIR | ||
set(SWIFT_SYNTAX_LIBRARIES_BUILD_DIR | ||
"${SWIFT_PATH_TO_EARLYSWIFTSYNTAX_BUILD_DIR}/lib/swift/host") |
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.
This is really meant to be a "build" directory where earlyswiftsyntax's .so
files exist.
lib/CMakeLists.txt
Outdated
swift_install_in_component( | ||
FILES "${SWIFT_HOST_LIBRARIES_DEST_DIR}/${sharedlib}" | ||
DESTINATION "lib${LLVM_LIBDIR_SUFFIX}/swift/host" | ||
COMPONENT compiler | ||
) | ||
|
||
swift_install_file_set_rpath( | ||
"lib${LLVM_LIBDIR_SUFFIX}/swift/host/${sharedlib}" | ||
"$$ORIGIN:$$ORIGIN/../linux" | ||
compiler | ||
) |
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.
SwiftSyntax libraries (.so
and .swiftmodule
) are now being installed from swift
build directory, but not earlyswiftsyntax
build directory. So we can centralize the RUNPATH mutation logic in this repository.
# Add rpath to 'lib/host' | ||
if(SWIFT_HOST_VARIANT_SDK STREQUAL "LINUX") | ||
set_property(TARGET ${name} | ||
APPEND PROPERTY INSTALL_RPATH "$ORIGIN/..") |
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.
Linux's RUNPATH
doesn't inherits executable's RUNPATH
. So each .so
must have its own complete RUNPATH
.
800b056
to
4c10475
Compare
swiftlang/swift-syntax#2090 |
I notice you are stripping builder runpaths. I haven't look into why that's needed in detail, but maybe it won't be needed if That may be easier than removing builder runpaths yourself before installation, as you seem to be doing here? I could be way off since I didn't look deeply into why you're doing this. |
4c10475
to
37216ab
Compare
swiftlang/swift-syntax#2090 |
1 similar comment
swiftlang/swift-syntax#2090 |
For compiling codes required for macro support, we now need swiftc compiler in the build machine. Unlike Darwin OSes, where swiftCore runtime is guaranteed to be present in /usr/lib, Linux doesn't have ABI stability and the stdlib of the build machine is not at the specific location. So the built compiler cannot relies on the shared object in the toolchain.
37216ab
to
9c9010e
Compare
swiftlang/swift-syntax#2090 |
The TLDR is that we need to cherry-pick this change to 5.9.0 and thus would like as few changes as possible.
We also need to do this for the copied-in "early" swift-syntax libs, which currently aren't part of the swift build itself. Since they're built separately, we can't do the same BUILD/INSTALL_RPATH split. I have a PR up to build swift-syntax in the same tree (#66043), but will get back to that after this is in and 5.9 is released (I ran into some issues with the bootstrapping case, which I haven't had the time to look into). I also have a local patch that does most of the BUILD/INSTALL_RPATH splitting that I'll put up as well. Once that's all in, I definitely agree that using BUILD/INSTALL_RPATH here rather than the manual stripping is the way to go. |
swiftlang/swift-syntax#2090 |
Many shared libs and executables are only run after stdlib/runtime are built. They don't need to link with builders stdlib at all.
26cc7d5
to
40841b3
Compare
swiftlang/swift-syntax#2090 |
and Swift parser integration is enabled. If swift parser integration is enabled, SwiftSyntax libraries are always built with host tools. Other SwiftCompilerSources modules must use the same runtime with parser libraries.
40841b3
to
0f0c492
Compare
swiftlang/swift-syntax#2090 |
swiftlang/swift-syntax#2090 |
swiftlang/swift-syntax#2090 |
cmake/modules/AddPureSwift.cmake
Outdated
get_filename_component(swift_bin_dir ${CMAKE_Swift_COMPILER} DIRECTORY) | ||
get_filename_component(swift_dir ${swift_bin_dir} DIRECTORY) | ||
set(host_lib_dir "${swift_dir}/lib/swift/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}") |
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 these used? I guess this is currently working because ASTGen is statically linked and the pure tools would all use FALSE now anyway, so this function is only used by the plugins. But those aren't used except for in tests and thus host isn't actually needed anywhere that uses this right now?
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.
They are leftover. Just removed.
cmake/modules/AddSwift.cmake
Outdated
@@ -744,7 +745,7 @@ function(add_swift_host_library name) | |||
_set_target_prefix_and_suffix(${name} "${libkind}" "${SWIFT_HOST_VARIANT_SDK}") | |||
|
|||
if (ASHL_SHARED AND ASHL_HAS_SWIFT_MODULES) | |||
_add_swift_runtime_link_flags(${name} "." "") | |||
_add_swift_runtime_link_flags(${name} "." "" FALSE) |
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.
Hmm, maybe required_for_minimal_compiler
doesn't make sense as the name. Since these ... are required, it's just they don't have any Swift in them.
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 don't think any shared (ASHL_SHARED
) libraries are required for building stdlib.
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.
Ah this is only shared, true. Having said that, if we did move to shared libraries more, it would be the case that they are "required for minimal" but this would still be FALSE because they wouldn't have any Swift. And that was more my point.
if((NOT SWIFT_SWIFT_PARSER) AND NOT(BOOTSTRAPPING_MODE STREQUAL "HOSTTOOLS")) | ||
return() | ||
endif() |
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.
Should this be an OR
? ie. We just want !(linux && swift_swift_parser && bootstrapping == hosttools)
but this is !swift_swift_parser && bootstrapping != hosttools
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.
My intent was !(linux && (swift_swift_parser || boostrapping==hosttools))
because boostrapping==hosttools
alone also needs to strip builder's RUNPATH
.
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.
bootstrapping==hosttools
without early swift syntax shouldn't have the host in the RUNPATH
anyway though right?
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.
It should, otherwise which runtime does the SwiftCompilerSources
library link with (before it builds stdlib/runtime)?
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.
Ah true, forgot about SwiftCompilerSources here 😅 Thanks, sorry for the run around!
file(RELATIVE_PATH relative_rtlib_path "${path}" "${SWIFTLIB_DIR}/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}") | ||
list(APPEND RPATH_LIST "$ORIGIN/${relative_rtlib_path}") | ||
# NOTE: SourceKit components are NOT executed before stdlib is built. | ||
# So there's no need to add RUNPATH to builder's runtime libraries. | ||
|
||
elseif(ASKD_BOOTSTRAPPING_MODE STREQUAL "BOOTSTRAPPING") | ||
get_bootstrapping_swift_lib_dir(bs_lib_dir "") |
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.
Lines 169-191 are very odd, given that there is no bootstrapping for SK. I know we're trying to make minimal changes, but IMO we should at least:
- Delete 159 which is just plain wrong
- Delete the else branch, since we can't get into there for Linux anyway
- Remove the
bootstrapping
check, since it's always empty - Delete 161-165 since that's handled just above this comment
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.
Ah, (4) adds host. IMO I'd move that to inside the ifs here since the if there is wrong anyway (it should be specifically linux/android/openbsd). So ie.
if(SWIFT_SWIFT_PARSER)
file(RELATIVE_PATH relative_hostlib_path "${path}" "${SWIFTLIB_DIR}/host")
list(APPEND RPATH_LIST "$ORIGIN/${relative_hostlib_path}")
endif()
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.
👍 Done
tools/driver/CMakeLists.txt
Outdated
@@ -26,6 +26,7 @@ if(BOOTSTRAPPING_MODE MATCHES "BOOTSTRAPPING.*") | |||
HAS_SWIFT_MODULES | |||
BOOTSTRAPPING 0 | |||
THINLTO_LD64_ADD_FLTO_CODEGEN_ONLY | |||
REQUIRED_FOR_MINIMAL_COMPILER |
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.
Could skip adding this and the next, since they're only used for bootstrapping which we're not doing.
# Ensure that we can find the host shared libraries. | ||
set_property( | ||
TARGET libSwiftScan | ||
APPEND PROPERTY INSTALL_RPATH "@loader_path/swift/host") |
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.
Could just append them both at once with :
but doesn't really matter.
utils/build-presets.ini
Outdated
@@ -859,7 +859,6 @@ install-lldb | |||
install-llbuild | |||
install-swiftpm | |||
install-swift-driver | |||
install-swiftsyntax |
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.
Could just leave this in since the produce is changed. There's a lot of these that would need to be removed anyway.
utils/build-presets.ini
Outdated
# Linux only support 'hosttools' with early-swiftsyntax. | ||
bootstrapping=hosttools |
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.
We're going to need to add this in quite a few places. Maybe we should override and set to hosttools when early swiftsyntax is enabled, since that's the only thing that works? Bit weird but...
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.
Updated to force setting set(BOOTSTRAPPING_MODE "HOSTTOOLS")
when SWIFT_SWIFT_PARSER
is enabled.
""" | ||
self.install_with_cmake(["install"], self.host_install_destdir(host_target)) | ||
# No-op. | ||
None |
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.
pass
is the Python-ism you want here
Thanks Rintaro!
:O! |
SourceKit components are never built while bootstrapping stages. So it doen't need to have stage 0 or 1 handling.
swiftlang/swift-syntax#2090 |
swiftlang/swift-syntax#2090 |
`$ORIGIN` is only supported in Linux, BSD, etc. (i.e. not in Windows)
swiftlang/swift-syntax#2090 |
swiftlang/llvm-project#7331 |
libSwiftScan is built in 'lib' but installed in 'lib/swift/host' RUNPATH should have correct '$ORIGIN/../{platform}' to load 'swiftCore' runtime library.
swift-compatibility-symbols, swift-def-to-strings-converter, and swift-serialize-diagnostics don't use any Swift modules. But when SWIFT_SWIFT_PARSER was enabled, they are linked with swiftCore. But these binaries can be executed before the runtime is being built. We need to stop them linking with swiftCore.
ab0142b
to
dc68773
Compare
swiftlang/llvm-project#7331 |
swiftlang/llvm-project#7331 |
swiftlang/llvm-project#7331 |
RPATH_SET is not available until cmake 3.21.0. Use RPATH_CHANGE instead.
b4e7d09
to
f645069
Compare
swiftlang/swift-syntax#2090 |
swiftlang/swift-syntax#2090 |
2 similar comments
swiftlang/swift-syntax#2090 |
swiftlang/swift-syntax#2090 |
Some non-stdlib thing e.g. swift-backtrace still might be built before the runtime is built. For building Swift code in Linux "hosttools", always set 'LD_LIBRARY_PATH' to the runtime in the builder.
swiftlang/swift-syntax#2090 |
swiftlang/swift-syntax#2090 |
I see a handful of runpath issues with the last CI-built toolchain for linux after this pull, I'll look into the fixes:
That second relative path is wrong, hence the runtime error. |
Thanks for checking @finagolfin! Looks like https://github.com/apple/llvm-project/blob/63a082e326d053e778a05a899514ba101d7db5f6/lldb/cmake/modules/AddLLDB.cmake#L225 needs a |
elseif(SWIFT_HOST_VARIANT_SDK MATCHES "LINUX|ANDROID|OPENBSD|FREEBSD") | ||
set_property( | ||
TARGET libSwiftScan | ||
APPEND PROPERTY INSTALL_RPATH "$ORIGIN/swift/host") |
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.
@rintaro, is this needed for some intermediate build step or something? Because the final installed library also has this path, which is wrong:
> readelf -d ./usr/lib/swift/host/lib_InternalSwiftScan.so |ag runpath
0x000000000000001d (RUNPATH) Library runpath: [$ORIGIN:$ORIGIN/./swift/linux:$ORIGIN/../linux:$ORIGIN/swift/host:$ORIGIN/../host]
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.
Yeah, see the many FIXME above. This all needs to be split into BUILD/INSTALL_RPATH + built into host rather than into lib, but that's too much for 5.9. We'll fix this up after all the 5.9 PRs are in as well. We could remove it, but it doesn't really hurt either.
For compiling codes required for macro support, we now need swiftc compiler in the build machine.
Unlike Darwin OSes, where swiftCore runtime is guaranteed to be present in /usr/lib, Linux doesn't have ABI stability and the stdlib of the build machine is not at the specific location. So the built compiler cannot relies on the shared object in the system.