Skip to content

Use new SwiftPM flag to remove $ORIGIN from installed sourcekit-lsp ELF executable runpath #894

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 20, 2023
Merged

Use new SwiftPM flag to remove $ORIGIN from installed sourcekit-lsp ELF executable runpath #894

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions Utilities/build-script-helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ def get_swiftpm_options(swift_exec: str, args: argparse.Namespace) -> List[str]:
'-Xcxx', '-I', '-Xcxx',
os.path.join(args.toolchain, 'lib', 'swift', 'Block'),
]
if args.action == 'install':
swiftpm_args += ['--disable-local-rpath']
Comment on lines +127 to +128
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if '-android' in build_target:
swiftpm_args += [
Expand Down Expand Up @@ -181,6 +183,9 @@ def get_swiftpm_environment_variables(swift_exec: str, args: argparse.Namespace)
if args.action == 'test' and args.skip_long_tests:
env['SKIP_LONG_TESTS'] = '1'

if args.action == 'install':
env['SOURCEKIT_LSP_CI_INSTALL'] = "1"

return env


Expand All @@ -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"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cmd = [swift_exec, 'build', '--product', product] + swiftpm_args
check_call(cmd, additional_env=additional_env, verbose=args.verbose)

Expand Down