-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[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 |
SwiftCompilerSources/CMakeLists.txt
Outdated
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
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.
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.
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.
lib/CMakeLists.txt
Outdated
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
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.
lib/Macros/CMakeLists.txt
Outdated
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
|
|
||
| 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!
| # 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
bootstrappingcheck, 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
| 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.
| 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.