[5.9] Remove extra paths added to LD_LIBRARY_PATH on Linux when running tests #6691

bnbarham · 2023-07-10T21:23:06Z

Explanation: Removes an old workaround that breaks running tests when there's already a swift on PATH that isn't the swift currently being run.
Scope: swift test on Linux platforms
Risk: Low, our tests would fail if this was still needed.
Testing: Toolchain build and test on all Linux platforms.
Original PR: Remove extra paths added to LD_LIBRARY_PATH on Linux when running tests #6684

These break running tests when there's already a swift on PATH that isn't the swift currently being run. There shouldn't be a need for these as the run path of the compiled binary is already the correct path (ie. the library path of the swift that built it and `$ORIGIN`). (cherry picked from commit 5c7db58)

neonichu · 2023-07-10T21:37:53Z

Low, our tests would fail if this was still needed.

I'm not sure that is actually true. When I introduced the workaround, the shipped nightly toolchain was broken which I think implies that the tests were passing despite that being the case.

bnbarham · 2023-07-10T21:40:48Z

Low, our tests would fail if this was still needed.

I'm not sure that is actually true. When I introduced the workaround, the shipped nightly toolchain was broken which I think implies that the tests were passing despite that being the case.

That's... odd. That's what the integration tests are for 🤔. Plus indexstore-db/sourcekit-lsp/etc are use the just built toolchain (that's where the tests were failing with this workaround). Do you know what was failing exactly?

neonichu · 2023-07-10T21:42:11Z

Not exactly. IIRC, I was basically not able to use swift test without manually setting LD_LIBRARY_PATH.

DougGregor · 2023-07-24T20:57:10Z

@swift-ci please build toolchain

tomerd · 2023-07-25T17:39:44Z

@bnbarham does this need to be merged?

bnbarham · 2023-07-25T19:54:28Z

We still need to make sure it doesn't cause the issue @neonichu mentioned above.

finagolfin · 2023-08-05T03:37:33Z

Sources/Commands/Utilities/TestingSupport.swift

@@ -141,16 +141,6 @@ enum TestingSupport {
        if let location = toolchain.xctestPath {
            env.prependPath("Path", value: location.pathString)
        }
-        #elseif os(Linux)
-        var libraryPaths = ["/usr/lib/swift/linux"]


This line is clearly wrong and should be removed no matter if the others are.

finagolfin · 2023-08-05T03:49:03Z

There were clearly issues with this repo when Swift is pre-installed in the PATH, ie it would silently pick that prebuilt toolchain up and you wouldn't notice until some breaking change, as I've seen this problem on Android too.

If you want to remove this, you really need to make sure it's unneeded by adding some debug logging to some tests and checking that they're not building with the wrong toolchain or linking against the wrong runtime libraries.

bnbarham · 2023-08-10T04:56:00Z

There were clearly issues with this repo when Swift is pre-installed in the PATH, ie it would silently pick that prebuilt toolchain up and you wouldn't notice until some breaking change, as I've seen this problem on Android too.

If you want to remove this, you really need to make sure it's unneeded by adding some debug logging to some tests and checking that they're not building with the wrong toolchain or linking against the wrong runtime libraries.

I haven't gone as far as to add debug logging here, but I have a 5.8 on PATH and using the downloaded toolchain with this patch:

swift test for a local package passes
That xctest binary has Library runpath: [/home/build-user/ws/tmp/pmtest/usr/lib/swift/linux:$ORIGIN]

With the latest 5.9 snapshot swift test fails with:

/home/build-user/ws/tmp/test-package/.build/x86_64-unknown-linux-gnu/debug/test-packagePackageTests.xctest: symbol lookup error: /home/build-user/ws/tmp/test-package/.build/x86_64-unknown-linux-gnu/debug/test-packagePackageTests.xctest: undefined symbol: swift_getTypeByMangledNameInContext2

finagolfin · 2023-08-10T05:14:13Z

swift test for a local package passes

I assume you mean /home/build-user/ws/tmp/pmtest/usr/bin/swift test passes, ie you are running SPM 5.9 not 5.8 from your PATH. I suggest you run it again in verbose mode with -v and simply check all the absolute paths it spits out, to make sure it's only using paths to the 5.9 toolchain.

bnbarham · 2023-08-10T16:00:23Z

I assume you mean /home/build-user/ws/tmp/pmtest/usr/bin/swift test passes, ie you are running SPM 5.9 not 5.8 from your PATH.

Yes.

I suggest you run it again in verbose mode with -v and simply check all the absolute paths it spits out, to make sure it's only using paths to the 5.9 toolchain.

I already did, there's no mention of the 5.8 toolchain.

finagolfin

I just built and tested NIO with 5.8.1 in my PATH but using the latest trunk snapshot and didn't see anything wrong in the verbose log either. It's possible that subsequent pulls to swift-driver's Toolchain.lookup() have made this testing block no longer needed.

bnbarham · 2023-08-10T17:18:59Z

I just built and tested NIO with 5.8.1 in my PATH but using the latest trunk snapshot and didn't see anything wrong in the verbose log either. It's possible that subsequent pulls to swift-driver's Toolchain.lookup() have made this testing block no longer needed.

I'm more concerned with the 5.8 PR that I haven't had a chance to validate yet. I suspect there's a good chance it still has whatever bug had caused this to be needed in the first place. If that's the case it'll need to be changed to add the path of the currently running swift, rather than the first swift on PATH.

bnbarham · 2023-08-10T17:19:05Z

@swift-ci please test

neonichu · 2023-08-10T17:38:32Z

@swift-ci please smoke test

neonichu · 2023-08-10T17:39:58Z

I wonder if we should just have "test" be an alias of "smoke test" at this point? It looks like "test" starts the Windows build but nothing else, so there's no reason to actually have "test" be a separate thing at this point, right? It's just unnecessary confusing. @tomerd wdyt?

tomerd · 2023-08-10T19:51:19Z

+1

neonichu · 2023-08-10T19:58:45Z

Thanks for testing this @bnbarham, sounds like this should be good to merge, but we'll also need @tomerd and @airspeedswift at this point for 5.9

MaxDesiatov · 2023-08-16T21:20:37Z

Will this need to use release/5.9.0 as the base branch? Or maybe in that case that would need a separate PR as we'd want to see it in release/5.9 too.

DougGregor · 2023-08-16T21:36:31Z

Will this need to use release/5.9.0 as the base branch? Or maybe in that case that would need a separate PR as we'd want to see it in release/5.9 too.

We will need both

bnbarham · 2023-08-16T22:34:31Z

Heh, you must have commented on this just as I was putting up #6822 😅 (4 minutes apart apparently!)

bnbarham requested review from abertelrud, neonichu and tomerd as code owners July 10, 2023 21:23

DougGregor mentioned this pull request Jul 24, 2023

Clean up some whitespace in the changelog swiftlang/swift#66222

Closed

tomerd added the swift 5.9 This PR targets the 5.9 branch label Jul 25, 2023

bnbarham mentioned this pull request Aug 5, 2023

[DNM] PR test swiftlang/swift#62616

Closed

finagolfin reviewed Aug 5, 2023

View reviewed changes

finagolfin approved these changes Aug 10, 2023

View reviewed changes

neonichu approved these changes Aug 10, 2023

View reviewed changes

DougGregor approved these changes Aug 16, 2023

View reviewed changes

bnbarham merged commit df48749 into swiftlang:release/5.9 Aug 16, 2023

bnbarham deleted the cherry-remove-linux-test-ld-path branch August 16, 2023 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5.9] Remove extra paths added to LD_LIBRARY_PATH on Linux when running tests #6691

[5.9] Remove extra paths added to LD_LIBRARY_PATH on Linux when running tests #6691

bnbarham commented Jul 10, 2023 •

edited

Loading

neonichu commented Jul 10, 2023

bnbarham commented Jul 10, 2023 •

edited

Loading

neonichu commented Jul 10, 2023 •

edited

Loading

DougGregor commented Jul 24, 2023

tomerd commented Jul 25, 2023 •

edited

Loading

bnbarham commented Jul 25, 2023

finagolfin Aug 5, 2023

finagolfin commented Aug 5, 2023

bnbarham commented Aug 10, 2023

finagolfin commented Aug 10, 2023

bnbarham commented Aug 10, 2023

finagolfin left a comment

bnbarham commented Aug 10, 2023

bnbarham commented Aug 10, 2023

neonichu commented Aug 10, 2023

neonichu commented Aug 10, 2023

tomerd commented Aug 10, 2023

neonichu commented Aug 10, 2023

MaxDesiatov commented Aug 16, 2023 •

edited

Loading

DougGregor commented Aug 16, 2023

bnbarham commented Aug 16, 2023

[5.9] Remove extra paths added to LD_LIBRARY_PATH on Linux when running tests #6691

[5.9] Remove extra paths added to LD_LIBRARY_PATH on Linux when running tests #6691

Conversation

bnbarham commented Jul 10, 2023 • edited Loading

neonichu commented Jul 10, 2023

bnbarham commented Jul 10, 2023 • edited Loading

neonichu commented Jul 10, 2023 • edited Loading

DougGregor commented Jul 24, 2023

tomerd commented Jul 25, 2023 • edited Loading

bnbarham commented Jul 25, 2023

finagolfin Aug 5, 2023

Choose a reason for hiding this comment

finagolfin commented Aug 5, 2023

bnbarham commented Aug 10, 2023

finagolfin commented Aug 10, 2023

bnbarham commented Aug 10, 2023

finagolfin left a comment

Choose a reason for hiding this comment

bnbarham commented Aug 10, 2023

bnbarham commented Aug 10, 2023

neonichu commented Aug 10, 2023

neonichu commented Aug 10, 2023

tomerd commented Aug 10, 2023

neonichu commented Aug 10, 2023

MaxDesiatov commented Aug 16, 2023 • edited Loading

DougGregor commented Aug 16, 2023

bnbarham commented Aug 16, 2023

bnbarham commented Jul 10, 2023 •

edited

Loading

bnbarham commented Jul 10, 2023 •

edited

Loading

neonichu commented Jul 10, 2023 •

edited

Loading

tomerd commented Jul 25, 2023 •

edited

Loading

MaxDesiatov commented Aug 16, 2023 •

edited

Loading