Skip to content
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.

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

[5.8] On ELF platforms, only add runpaths as needed (#6321) #6430

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

finagolfin
Copy link
Member

Cherrypick of #6321

Explanation: The bootstrap script currently adds two relative runpaths for ELF platforms, one for the PackageDescription and other libraries and another for the single swift-package executable, but since they're applied on the command line, both runpaths are applied to all binaries. This pull separates out the runpaths and only applies each where needed. It also shifts much of the bootstrap script over to applying build flags based on the build target, not on checking the build host platform in Python.

Scope: Should only affect the SPM build on ELF platforms like linux

Issue: None

Risk: low, as it only affects building SPM for ELF platforms

Testing: Passed all CI, including a trunk toolchain build that worked well

Reviewer: @neonichu

As long as I'm modifying the Python bootstrap script, clean up some incorrect
checks of the host `platform.system()` and replace some unnecessary regexes.
@neonichu
Copy link
Contributor

@swift-ci please smoke test

@tomerd tomerd added the swift 5.8 This PR targets the 5.8 branch label Apr 13, 2023
@finagolfin
Copy link
Member Author

MacOS SPM test failure appears unrelated, flake?

2023-04-13 21:14:48.302Test Case '-[SPMBuildCoreTests.PluginInvocationTests testCompilationDiagnostics]' started.
/Users/ec2-user/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-release/5.8/swiftpm/Tests/SPMBuildCoreTests/PluginInvocationTests.swift:420: error: -[SPMBuildCoreTests.PluginInvocationTests testCompilationDiagnostics] : XCTAssertEqual failed: ("0") is not equal to ("1") - unexpected diagnostics count in [] from /var/folders/7n/r31lzfjx6556jgc6vg55_cg80000gn/T/spm-tests-testCompilationDiagnostics.o65Cw0/plugin-cache/MyPlugin.dia
/Users/ec2-user/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-release/5.8/swiftpm/Tests/SPMBuildCoreTests/PluginInvocationTests.swift:421: error: -[SPMBuildCoreTests.PluginInvocationTests testCompilationDiagnostics] : XCTUnwrap failed: expected non-nil value of type "Diagnostic"Test Case '
[SPMBuildCoreTests.PluginInvocationTests testCompilationDiagnostics]' failed (7.416 seconds).
Test Suite 'PluginInvocationTests' failed at 2023-04-13 21:14:55.718.
	 Executed 1 test, with 2 failures (0 unexpected) in 7.416 (7.417) seconds

@neonichu
Copy link
Contributor

I think so, yah.

@tomerd are we planning on any more non-Darwin releases for 5.8?

@tomerd
Copy link
Contributor

tomerd commented Apr 14, 2023

there isn't one panned right now, but we normally try to get one going if there is enough / urgent fixes

@finagolfin
Copy link
Member Author

@neonichu, one last mac CI run and this can go in? I see the 5.8 branch still being updated for the compiler and stdlib.

@tomerd
Copy link
Contributor

tomerd commented Apr 18, 2023

@buttaface yes we can pull this in when we do a linux dot release (tbd exactly when)

@tomerd
Copy link
Contributor

tomerd commented Apr 18, 2023

@swift-ci smoke test macOS

@finagolfin
Copy link
Member Author

Huh, the same SPM test failed again on macOS, but this pull should change nothing on macOS, and passed fine on 5.9 and trunk.

I have little insight into macOS failures, anyone know what's going on?

@tomerd
Copy link
Contributor

tomerd commented Apr 18, 2023

@neonichu @abertelrud looks like testCompilationDiagnostics is still flaky / failing?

@swift-ci smoke test macOS

@neonichu
Copy link
Contributor

@neonichu @abertelrud looks like testCompilationDiagnostics is still flaky / failing?

That's possible, I think we realized that the way we're checking for compiler compatibility could be using the superior toolchain instead of the inferior one.

@tomerd
Copy link
Contributor

tomerd commented Apr 18, 2023

@swift-ci smoke test macOS

@finagolfin
Copy link
Member Author

Same test failed a third time, doesn't appear to be a flake. I compared the commit log for that file to trunk, and I see two pulls that recently modified that test in trunk and 5.9 that are not in the 5.8 branch, #6076 and #6080.

I have not looked into what triggers those, but perhaps the 5.8 CI is just broken right now because it needs some of those changes? This pull should change nothing on macOS and so shouldn't be at fault.

@tomerd
Copy link
Contributor

tomerd commented Apr 19, 2023

lets see what an empty PR does: #6455

@tomerd
Copy link
Contributor

tomerd commented Apr 20, 2023

@swift-ci smoke test macOS

@tomerd tomerd enabled auto-merge (squash) April 20, 2023 14:09
@tomerd tomerd merged commit f708879 into swiftlang:release/5.8 Apr 20, 2023
@finagolfin finagolfin deleted the rpath branch April 21, 2023 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
swift 5.8 This PR targets the 5.8 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants