Use new SwiftPM flag to remove $ORIGIN from installed sourcekit-lsp ELF executable runpath
#894
Conversation
| @@ -190,8 +195,6 @@ def build_single_product(product: str, swift_exec: str, args: argparse.Namespace | |||
| """ | |||
| swiftpm_args = get_swiftpm_options(swift_exec, args) | |||
| additional_env = get_swiftpm_environment_variables(swift_exec, args) | |||
| if args.action == 'install': | |||
| additional_env['SOURCEKIT_LSP_CI_INSTALL'] = "1" | |||
There was a problem hiding this comment.
I think I put this here and not in get_swiftpm_environment_variables() initially either because it broke something when invoking swiftpm_bin_path() also in the install action or I was concerned it would, I forget which. Let's try it on the CI and find out.
| if args.action == 'install': | ||
| swiftpm_args += ['--disable-local-rpath'] |
There was a problem hiding this comment.
Could you check if this causes sourcekit-lsp to get rebuilt if you do a build first and then an install? Because we want to avoid having to build sourcekit-lsp twice in CI, once for build and once for install.
There was a problem hiding this comment.
It would have to get rebuilt, as the normal build includes the local rpath but the installation build doesn't. I think you're worried about if all the source gets rebuilt, rather than simply the final link step getting redone?
Considering only the latter happens for the added linkerSettings that I previously added in #715 and this SwiftPM flag also only affects the link arguments for executable targets, I believe this flag won't trigger a full rebuild either, but I haven't tried it on this repo locally to make sure.
There was a problem hiding this comment.
Yes, re-linking would be totally fine because that really changes. I just want to avoid re-building all the Swift sources. That’s what I would expect as well, but you never know.
If you could check that the install only re-links, that would be great. You could also trigger a toolchain build from apple/swift using cross PR testing and see if the installation time of sourcekit-lsp increases.
There was a problem hiding this comment.
Done, applied this pull and built on my Android phone: it did not trigger a full rebuild, only 7 items were rebuilt. It was helpful though, as it showed a mistake I made in not renaming the additional_env variable when moving it, which I've now fixed.
… ELF executable runpath Also, move installation-only environment variable into get_swiftpm_environment_variables().
|
Ping, a CI run and this can go in? I'll submit for 5.10 next. |
|
@swift-ci please test |
|
Ready for merge, will submit for 5.10 once this is in. |
Also, move installation-only environment variable into
get_swiftpm_environment_variables().One of the latest trunk snapshot toolchains shows five executables built by SwiftPM that incorrectly have this runpath, including
sourcekit-lspfrom this repo:This will remove that unneeded runpath, as I just did for
swift-packagein swiftlang/swift-package-manager#6947. I limited this to non-Darwin installs because I have no idea if this would be useful on Darwin too.