[Macros] Add the correct RPATH flags to the in-process plugin server host library #75585
Conversation
cmake/modules/AddPureSwift.cmake
Outdated
| @@ -193,6 +193,9 @@ function(add_pure_swift_host_library name) | |||
| # Create the library. | |||
| add_library(${name} ${libkind} ${APSHL_SOURCES}) | |||
| _add_host_swift_compile_options(${name}) | |||
| if(APSHL_SHARED) | |||
| _set_pure_swift_link_flags(${name} "../../") | |||
There was a problem hiding this comment.
This assumes the library is installed to usr/lib/swift/host/: we'll have to make this a parameter of add_pure_swift_host_library() if it ever starts getting used by libraries installed elsewhere.
There was a problem hiding this comment.
This function is used for building stdlib macro plugins too. (libSwiftMacors.so and libObservationMacros.so) Currently they call _set_pure_swift_link_flags in the call site as the output and install directories are controlled in the call sites. For now could you follow that? I.e. call _set_pure_swift_link_flag in tools/swift-plugin-server/CMakeLists.txt
There was a problem hiding this comment.
This function is used for building stdlib macro plugins too. (libSwiftMacors.so and libObservationMacros.so) Currently they call _set_pure_swift_link_flags in the call site as the output and install directories are controlled in the call sites.
No, those macro plugins also call this _set_pure_swift_link_flags() function in a generalized add_swift_macro_library() function that uses a single hard-coded install path, just as I'm doing here in add_pure_swift_host_library().
Currently, there is only one shared host library built this way. I added _set_pure_swift_link_flags() to add_pure_swift_host_library() here to make sure the link flags are always added for any future shared host libraries, just as you did before with the two other generalized invocations of _set_pure_swift_link_flags() in add_swift_macro_library() and add_pure_swift_host_tool(), both of which also use hard-coded install paths.
Are you sure you want me to specialize this for the SwiftInProcPluginServer shared library alone?
There was a problem hiding this comment.
Sorry I think I wasn't clear. "this function" I meant was add_pure_swift_host_library()
add_swift_macro_library() is just calling add_pure_swift_host_library() and adding some macro plugin specific configuration to the target. I'm pretty sure your current change adds an incorrect RPATH to them.
Are you sure you want me to specialize this for the SwiftInProcPluginServer shared library alone?
Yes.
There was a problem hiding this comment.
"this function" I meant was add_pure_swift_host_library()
add_swift_macro_library() is just calling add_pure_swift_host_library() and adding some macro plugin specific configuration to the target. I'm pretty sure your current change adds an incorrect RPATH to them.
Yep, you're right, I missed that. I checked those Macro libraries built natively on Android and this RPATH was incorrectly added there too.
I've now made this change only for the one library instead, as you asked for, and rebased.
|
@kateinoigakukun, would you run the CI on this? |
|
@swift-ci please test |
Sure thing. I remember you running the CI for me on random repos where I never even pinged you or knew that you were involved with in anyway, so I thought you wanted to help. I won't ping you for CI runs on these other repos again. |
|
Normally the code owners should trigger the bot for you (not sure who owns this part, perhaps Rintaro), but you can also ask @swiftlang/contributor-experience, if, say, you are not getting a timely response. Though, really, you’ve contributed more than enough to go ahead and request CI/write access. |
|
Linux CI failure when running the SwiftPM tests appears spurious. |
…host library Also, make sure it builds for Android.
finagolfin
changed the title
|
@swift-ci Please smoke test |
|
Thanks for the review, @rintaro. |
like the in-process plugin server
Also, make sure the in-process plugin server builds for Android.
@rintaro, I noticed this library leaks the builder runpath in the latest trunk toolchain for linux:
This pull fixed that when I built it natively on Android, along with the
Bionicimport.I will submit an integration test for this new builder path once this is in.