[Unix] Move all ELF SDK runtime libraries into their own architecture-specific directories #63782
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -168,8 +168,11 @@ static void updateRuntimeLibraryPaths(SearchPathOptions &SearchPathOpts, | |||||
| LibSubDir = "maccatalyst"; | ||||||
|
|
||||||
| llvm::sys::path::append(LibPath, LibSubDir); | ||||||
| llvm::SmallString<128> runtimeLibPath = LibPath; | ||||||
| if (!Triple.isOSDarwin()) | ||||||
| llvm::sys::path::append(runtimeLibPath, swift::getMajorArchitectureName(Triple)); | ||||||
| SearchPathOpts.RuntimeLibraryPaths.clear(); | ||||||
| SearchPathOpts.RuntimeLibraryPaths.push_back(std::string(runtimeLibPath.str())); | ||||||
|
There was a problem hiding this comment. Should this be in if (!Triple.isOSDarwin()) {
llvm::SmallString<128> runtimeLibPath = LibPath;
llvm::sys::path::append(runtimeLibPath, swift::getMajorArchitectureName(Triple));
SearchPathOpts.RuntimeLibraryPaths.push_back(std::string(runtimeLibPath.str()));
} else {
SearchPathOpts.RuntimeLibraryPaths.push_back(DARWIN_OS_LIBRARY_PATH);
}There was a problem hiding this comment. Yes, this last line applies to Darwin, as there it simply does not add the architecture, so it is the same as LibPath before. Your code leaves out the There was a problem hiding this comment.
Suggested change
There was a problem hiding this comment. @compnerd, just seeing this suggestion now, missed this when you wrote it a week back. Since this is changing unrelated behavior to my simple renaming, can we get your change in in a subsequent pull? I just don't want to start another hours-long CI run for this unrelated update. |
||||||
| if (Triple.isOSDarwin()) | ||||||
| SearchPathOpts.RuntimeLibraryPaths.push_back(DARWIN_OS_LIBRARY_PATH); | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -803,11 +803,11 @@ function(add_swift_target_library_single target name) | |
| "${SWIFT_SDK_MACCATALYST_LIB_SUBDIR}/${SWIFTLIB_SINGLE_ARCHITECTURE}") | ||
| endif() | ||
|
|
||
| if ("${SWIFTLIB_SINGLE_BOOTSTRAPPING}" STREQUAL "" OR NOT SWIFTLIB_SINGLE_SDK IN_LIST SWIFT_DARWIN_PLATFORMS) | ||
| set(output_sub_dir ${SWIFTLIB_SINGLE_SUBDIR}) | ||
| else() | ||
| # In the bootstrapping builds, we only have the single host architecture. | ||
| # So generate the Darwin library directly in the parent SDK specific directory | ||
|
There was a problem hiding this comment. It may be possible to continue placing the bootstrapping libraries in a non-architecture-specific directory like on Darwin, then only link the rpath for the final bootstrapped Swift compiler against the architecture-specific directory, but I'm not interested in digging into all the CMake config to make that happen. It is easier to simply place the bootstrapping libraries in architecture-specific directories too on ELF platforns. I don't check for Windows here because my understanding is that bootstrapping the compiler doesn't work on Windows: @compnerd, correct me if I'm wrong. There was a problem hiding this comment. Nope, bootstrapping isn't enabled yet on Windows. |
||
| # (avoiding to lipo/copy the library). | ||
| set(output_sub_dir ${SWIFT_SDK_${SWIFTLIB_SINGLE_SDK}_LIB_SUBDIR}) | ||
| endif() | ||
|
|
@@ -1257,11 +1257,12 @@ function(add_swift_target_library_single target name) | |
| ${SWIFTLIB_SINGLE_C_COMPILE_FLAGS} "-DSWIFT_TARGET_LIBRARY_NAME=${name}") | ||
| set(link_flags ${SWIFTLIB_SINGLE_LINK_FLAGS}) | ||
|
|
||
|
There was a problem hiding this comment. This is unused, so I removed it. |
||
| set(library_search_directories | ||
| "${lib_dir}/${output_sub_dir}" | ||
| "${SWIFT_NATIVE_SWIFT_TOOLS_PATH}/../lib/swift/${SWIFTLIB_SINGLE_SUBDIR}") | ||
| if("${SWIFTLIB_SINGLE_SDK}" IN_LIST SWIFT_DARWIN_PLATFORMS) | ||
| list(APPEND library_search_directories "${SWIFT_NATIVE_SWIFT_TOOLS_PATH}/../lib/swift/${SWIFT_SDK_${SWIFTLIB_SINGLE_SDK}_LIB_SUBDIR}") | ||
| endif() | ||
|
|
||
| # In certain cases when building, the environment variable SDKROOT is set to override | ||
| # where the sdk root is located in the system. If that environment variable has been | ||
|
|
@@ -1487,8 +1488,10 @@ function(add_swift_target_library_single target name) | |
| endif() | ||
| set(library_search_directories | ||
| "${search_base_dir}/${SWIFTLIB_SINGLE_SUBDIR}" | ||
| "${SWIFT_NATIVE_SWIFT_TOOLS_PATH}/../lib/swift/${SWIFTLIB_SINGLE_SUBDIR}") | ||
| if("${SWIFTLIB_SINGLE_SDK}" IN_LIST SWIFT_DARWIN_PLATFORMS) | ||
| list(APPEND library_search_directories "${SWIFT_NATIVE_SWIFT_TOOLS_PATH}/../lib/swift/${SWIFT_SDK_${SWIFTLIB_SINGLE_SDK}_LIB_SUBDIR}") | ||
| endif() | ||
| target_link_directories(${target_static} PRIVATE | ||
| ${library_search_directories}) | ||
| target_link_libraries("${target_static}" PRIVATE | ||
|
|
@@ -2377,8 +2380,10 @@ function(add_swift_target_library name) | |
| if (SWIFTLIB_BACK_DEPLOYMENT_LIBRARY) | ||
| # Back-deployment libraries get installed into a versioned directory. | ||
| set(install_dest "lib${LLVM_LIBDIR_SUFFIX}/${resource_dir}-${SWIFTLIB_BACK_DEPLOYMENT_LIBRARY}/${resource_dir_sdk_subdir}") | ||
| elseif(sdk STREQUAL WINDOWS OR sdk IN_LIST SWIFT_DARWIN_PLATFORMS) | ||
|
There was a problem hiding this comment. I generally kept things the same on Windows, even though it already moved to architecture-specific directories. @compnerd, if you want me to consolidate Windows more, as I did in the compiler, let me know. There was a problem hiding this comment. I am a fan of having fewer cases, as long as things continue to work, I don't mind seeing things becoming uniform :) There was a problem hiding this comment. I believe Windows is no different from Generic Unix here, so let’s make Darwin the only special case. There was a problem hiding this comment. I'm not sure what Windows needs, which is why I mentioned this. Before, all three of Windows, Darwin, and Unix were installing here to Now that I'm moving Unix to this architecture-specific install directory, I wasn't sure if the same would work when cross-compiling the Windows stdlib, so I left it the same. If you think that will work for cross-compiling the Windows stdlib too, I will change it, but it will be untested, unless one of you tries it. There was a problem hiding this comment. I think that we should assume that it will be the same and should be safe to make this There was a problem hiding this comment. OK, can you add that later for Windows? I don't want to kick off another whole CI run for this unimportant change, would rather just get these pulls in first. |
||
| set(install_dest "lib${LLVM_LIBDIR_SUFFIX}/${resource_dir}/${resource_dir_sdk_subdir}") | ||
| else() | ||
| set(install_dest "lib${LLVM_LIBDIR_SUFFIX}/${resource_dir}/${resource_dir_sdk_subdir}/${SWIFT_PRIMARY_VARIANT_ARCH}") | ||
| endif() | ||
|
|
||
| swift_install_in_component(FILES "${UNIVERSAL_LIBRARY_NAME}" | ||
|
|
@@ -2454,6 +2459,10 @@ function(add_swift_target_library name) | |
| "${name}-${library_subdir}-static") | ||
| set(UNIVERSAL_LIBRARY_NAME | ||
| "${universal_subdir}/${library_subdir}/${CMAKE_STATIC_LIBRARY_PREFIX}${name}${CMAKE_STATIC_LIBRARY_SUFFIX}") | ||
| set(install_dest "lib${LLVM_LIBDIR_SUFFIX}/${install_subdir}/${resource_dir_sdk_subdir}") | ||
| if(NOT sdk STREQUAL WINDOWS AND NOT sdk IN_LIST SWIFT_DARWIN_PLATFORMS) | ||
| set(install_dest "${install_dest}/${SWIFT_PRIMARY_VARIANT_ARCH}") | ||
| endif() | ||
| _add_swift_lipo_target(SDK | ||
| ${sdk} | ||
| TARGET | ||
|
|
@@ -2463,7 +2472,7 @@ function(add_swift_target_library name) | |
| ${THIN_INPUT_TARGETS_STATIC}) | ||
| add_dependencies(${SWIFTLIB_INSTALL_IN_COMPONENT} ${lipo_target_static}) | ||
| swift_install_in_component(FILES "${UNIVERSAL_LIBRARY_NAME}" | ||
| DESTINATION "${install_dest}" | ||
| PERMISSIONS | ||
| OWNER_READ OWNER_WRITE | ||
| GROUP_READ | ||
|
|
@@ -2542,6 +2551,10 @@ function(_add_swift_target_executable_single name) | |
| # Prepare linker search directories. | ||
| set(library_search_directories | ||
| "${SWIFTLIB_DIR}/${SWIFT_SDK_${SWIFTEXE_SINGLE_SDK}_LIB_SUBDIR}") | ||
| if(NOT ${SWIFTEXE_SINGLE_SDK} IN_LIST SWIFT_DARWIN_PLATFORMS AND NOT ${SWIFTEXE_SINGLE_SDK} STREQUAL WINDOWS) | ||
| set(library_search_directories | ||
| "${library_search_directories}/${SWIFTEXE_SINGLE_ARCHITECTURE}") | ||
| endif() | ||
|
|
||
| # Add variant-specific flags. | ||
| _add_target_variant_c_compile_flags( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ | |
| // RUN: %empty-directory(%t) | ||
|
|
||
| // RUN: %target-build-swift %S/Inputs/ConcreteTypes.swift %S/Inputs/GenericTypes.swift %S/Inputs/Protocols.swift %S/Inputs/Extensions.swift %S/Inputs/Closures.swift %S/Inputs/Conformances.swift -parse-as-library -emit-module -emit-library -module-name ConformanceCheck -o %t/Conformances | ||
| // RUN: %target-swift-reflection-dump %t/Conformances %platform-dylib-dir/%target-library-name(swiftCore) | %FileCheck %s | ||
|
There was a problem hiding this comment. Now that the @artemcm or @al45tair, let me know if I should change those two also. |
||
|
|
||
| // CHECK: CONFORMANCES: | ||
| // CHECK: ============= | ||
|
|
||
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.
Does this preserve compatibility with existing toolchains?
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.
Good point, no, it will not on these ELF platforms. I always build with the latest Swift compiler though, so if others do the same, it will work fine. There may be a way to keep compatibility, but as I noted above about continuing to keep these libraries in the module directory when bootstrapping, I am not about to dig through the CMake config to add that for a usecase that almost nobody is likely using (I currently have bootstrapping turned off on Android because of #60272).
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.
Out of curiosity, won’t this make Regex literal (which should be an official Swift 5.7 feature) unsupported? I suppose (at least the Swift piece of) bootstrapping is mandatory 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.
Yes, regex literals don't work for the upcoming Swift 5.8 that I have been testing out natively on Android, finagolfin/termux-packages@a1befe6, but they should work on the current 5.7.3 release with bootstrapping enabled, as 5.7 was branched before those LLVM issues discussed in that bug.