Add back -no-toolchain-stdlib-rpath when installing sourcekit-lsp on linux

[finagolfin]

Pull #597 incorrectly removed this, as it's still needed on ELF platforms.

That pull noted some SPM changes invalidate the need for this flag, but that's irrelevant since this flag is passed directly to the Swift compiler.

As a result of this flag's removal last year, the 5.8 snapshots for linux have this extraneous /home/build-user/ rpath:

> readelf -d swift-5.8-DEVELOPMENT-SNAPSHOT-2022-12-20-a-ubuntu20.04/usr/bin/* | ag "File:|runpath" | ag sourcekit -A1
File: swift-5.8-DEVELOPMENT-SNAPSHOT-2022-12-20-a-ubuntu20.04/usr/bin/sourcekit-lsp
 0x000000000000001d (RUNPATH)            Library runpath: [/home/build-user/swift-nightly-install/usr/lib/swift/linux:$ORIGIN:$ORIGIN/../lib/swift/linux]

@ahoppen, please review.

[ahoppen]

Hey @buttaface,

We can’t simply add this flag back because in CI we are building all SwiftPM packages in a unified build, meaning that we don’t need to e.g. rebuild SwiftSyntax multiple times because the SourceKit-LSP, swift-format and the SourceKit stress tester import it. If one of the build invocation uses different compiler arguments than the others, that invalidates the entire unified build.

That being said, I think what we should be able to do is set this compiler flag inside SourceKit-LSP’s Package.swift using unsafeFlags. Do you think you could try that? I don’t have a Linux container at hand right now.

Also: Just so I understand the problem correctly: This is just about removing the rpath that references a directory on the Swift CI build machines. IIUC (and I might be completely wrong) that rpath shouldn’t be causing any harm because it most likely doesn’t exist on your machine, it’s just not clean. Is this correct?

[finagolfin]

I think what we should be able to do is set this compiler flag inside SourceKit-LSP’s Package.swift using unsafeFlags. Do you think you could try that?

I don't think that's the right place, because it's okay for local builds of sourcekit-lsp to have this rpath. The issue is that this build script determines the rpath for the executable shipping in the official toolchain, where this is wrong.

that rpath shouldn’t be causing any harm because it most likely doesn’t exist on your machine, it’s just not clean. Is this correct?

I believe it's a security hole on multi-user machines, as an attacker could create that account and then inject their own libraries' into others' use of sourcekit-lsp. Perhaps uncommon these days, but still worth closing up.

[finagolfin]

If one of the build invocation uses different compiler arguments than the others, that invalidates the entire unified build.

I did notice that. If there's some way to avoid that, let me know: maybe we can only apply it to executable targets at installation somehow?

[ahoppen]

Thanks for the explanation. That makes sense. I see two options here:

Option 1: Add the flag in Package.swift behind an environment variable like SWIFT_BUILD_SCRIPT_ENVIRONMENT similar to what we do in swift-syntax (https://github.com/apple/swift-syntax/blob/324166c2b2d6c2a06b078ff769b2e876ac933da2/Package.swift#L9)

Option 2: Pass -no-toolchain-stdlib-rpath in every build in the SwiftPM unified build. I believe this is the better option. The project that need updating should be: indexstore-db, SourceKitStressTester, SwiftEvolve, docc, swift-format and swift-syntax.

[finagolfin]

While the first option may be a way out, having to work around this seems wrong in the first place. Correct me if I'm wrong but I presume the reason SPM kicks off a rebuild is to make sure the same compiler flags are applied to all Swift source. However, this is a linker flag, so it should only apply to shared library and executable targets and shouldn't trigger a rebuild of all source files.

Is there currently no way to tell SPM or the compiler that?

[ahoppen]

I don’t think SwiftPM really knows anything about that compiler flag so it just assumes the worst and rebuilds all source files, which is a reasonable behavior IMO. Teaching SwiftPM about every Swift compiler flag doesn’t seem reasonable to me.

CC @neonichu

[neonichu]

I think if the flag gets passed as an unsafe flag in linkerSettings, that may only result in the link steps being redone, but not 100% confident in that behavior.

[finagolfin]

it just assumes the worst and rebuilds all source files, which is a reasonable behavior IMO. Teaching SwiftPM about every Swift compiler flag doesn’t seem reasonable to me.

Not every flag, but couldn't we give it a list of Swift compiler flags that shouldn't trigger a full rebuild?

I think if the flag gets passed as an unsafe flag in linkerSettings, that may only result in the link steps being redone, but not 100% confident in that behavior.

To be clear, this is a Swift compiler flag that only affects a linker flag set by the Swift compiler, so it wouldn't be passed to linkerSettings, which I believe are passed directly to the linker.

[neonichu]

To be clear, this is a Swift compiler flag that only affects a linker flag set by the Swift compiler

Ah I see. I don't think we have a great way to give special treatment to something like that at the moment. Rebuilding is happening at the llbuild layer and it works by basically comparing hashes of the full commandlines.

[finagolfin]

Huh, that is unfortunate that SPM doesn't control that. I will try Alex's workaround and submit that if it works.

[finagolfin]

Circled back around to this and updated this pull with a combination of Alex's Option 1 and Boris's suggestion of using linkerSettings, which works to not trigger a full unified rebuild in my local testing.

@neonichu, I was wrong to assume linkerSettings are passed directly to the linker like -Xlinker on the SPM command line, so I'm able to pass this through to the Swift compiler for the final executable product build, since only targets' linkerSettings are passed on to their associated SPM products and not their swiftSettings, which is what I originally tried as in Alex's swift-syntax link above.

@ahoppen, if you would kick off the CI for this, I'd like to get this in and into 5.8 next.

[ahoppen]

Note to self: Check that SourceKit-LSP build times aren’t increasing with this change.

@swift-ci Please test

[finagolfin]

Ready to go in? I will submit for 5.8 next.

[ahoppen]

Yes, looks good to me.

[ahoppen]

Ah, actually, I’d like to measure toolchain build times as well. I triggered a toolchain build in swiftlang/swift#37710. If that doesn’t show us rebuilding SourceKit-LSP during the install phase, I’ll merge.

[finagolfin]

Good thing you tested building the toolchain too: it appears there's a bug with macOS SPM alone where linkerSettings are wrongly passed to clang instead? I can work around that for now by only enabling this flag for ELF platforms, which are the only ones where it does anything anyway, as this rpath flag does nothing on macOS and Windows.

@neonichu, is this a known issue with mac SPM? Should I file a bug?

The good news is that this did not trigger a full unified rebuild on the linux toolchain, according to the build log, as the first iteration of this pull was doing.

[finagolfin]

Updated this to only add back the flag on linux, should be good to go now.

[ahoppen]

Oh, good thing we watched that. Let’s run CI on this PR + build a toolchain to be sure this doesn’t break anything.

@swift-ci Please test

[ahoppen]

Tests passed, toolchain build passed and installing SourceKit-LSP took <20s so it doesn’t seem to trigger a full rebuild. Let’s merge it.