Skip to content

Disuse unversioned triples on non-Darwin platforms. #3576

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

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

3405691582
Copy link
Member

@3405691582 3405691582 commented Jun 28, 2021

Disuse unversioned triples on non-Darwin platforms.

Motivation:

The target flag should take the versioned triple, otherwise on platforms
with versioned triples, the standard library won't be found.

On Darwin, the unversioned triple should still be used throughout. This
necessitates special-casing in the bootstrap script and when making
subdirectories during the build.

See TSC pr #229.

Modifications:

  • Utilities/bootstrap: get the versioned triple from swiftc, not the unversionedTriple.
  • Sources/Basics/Triple+Extensions.swift and Sources/Basics/CMakeLists.txt: introduce a new extension to and method on Triple from TSC to get the correct .build subdirectory for Darwin and non-Darwin platforms, platformBuildPathComponent
  • Sources/Commands/SwiftTool.swift: ensure platformBuildPathComponent is used.
  • Amended filter in BinaryTarget+Extensions.swift to deal with comparing tripleStrings instead of triple structs, so we can properly do the unversioning on Darwin.
  • Relevant tests updated to match.

Result:

When bootstrapping swiftpm, the target will be filled with the versioned triple on non-Darwin systems.

This must be applied in concert with the TSC pr.

@finagolfin
Copy link
Member

Btw, once TSC can handle versioned triples with your pull, swiftlang/swift-tools-support-core#229, does SPM properly pass those along to the Swift compiler, particularly when cross-compiling, or is further patching needed in SPM?

I need to modify TSC to handle Android versions too, which are appended to the environment, not the OS.

@3405691582
Copy link
Member Author

I can only really speak affirmatively to the platforms I have access to, but the -target flag appears to be appropriately set by SPM post the TSC change on OpenBSD and Linux. I don't believe further SPM patching should be required for the versioned triple changes but as you said Android may need to do something special.

@3405691582 3405691582 force-pushed the UnversionTripleInBootstrap branch 2 times, most recently from 4b7acf2 to 7e5db61 Compare June 29, 2021 04:13
@3405691582 3405691582 changed the title Use the versioned triple in the bootstrap script. Disuse unversioned triples on non-Darwin platforms. Jun 29, 2021
@finagolfin
Copy link
Member

I think this should work, but why not switch macOS to versioned triples too instead?

@aciidb0mb3r, any reason not to do that now that TSC would support it with his TSC pull?

@neonichu
Copy link
Contributor

@swift-ci please smoke test

@3405691582 3405691582 force-pushed the UnversionTripleInBootstrap branch 3 times, most recently from f57e544 to 2ec5b89 Compare June 30, 2021 03:40
@3405691582
Copy link
Member Author

Tests have been updated and some code cleaned up a little to factor this recurring pattern.

@neonichu
Copy link
Contributor

@swift-ci please smoke test

@3405691582 3405691582 force-pushed the UnversionTripleInBootstrap branch from 2ec5b89 to 795a205 Compare June 30, 2021 22:02
@neonichu
Copy link
Contributor

@swift-ci please smoke test

@3405691582 3405691582 force-pushed the UnversionTripleInBootstrap branch from 795a205 to 4ac062a Compare July 1, 2021 00:16
@3405691582
Copy link
Member Author

The extension is moved to Basics now to make sure that it is accessible everywhere, including in all relevant tests. All uses of tripleString in the tests were converted to using the new platformBuildPathComponent method where appropriate.

@abertelrud
Copy link
Contributor

@swift-ci please smoke test

1 similar comment
@abertelrud
Copy link
Contributor

@swift-ci please smoke test

@3405691582 3405691582 force-pushed the UnversionTripleInBootstrap branch from 4ac062a to d52eb0f Compare July 21, 2021 19:23
@3405691582
Copy link
Member Author

As edited, amended filter BinaryTarget+Extensions.swift to ensure triples are compared by tripleString and not triple themselves, so we can do the unversioning of triples on Darwin, to ensure the plugin tests that check for supportedTriples passes.

@abertelrud
Copy link
Contributor

@neonichu What is the next step here?

@finagolfin
Copy link
Member

Btw, I was just looking at the much more extensive Triple struct in SwiftDriver, which already supports API versioning for Android though perhaps not OpenBSD, as it was a port of the LLVM triple. Should we switch over to that instead, rather than using this more limited TSCUtility triple, which predates that Swift port?

@3405691582
Copy link
Member Author

Migrating the Triple module from the Driver is probably a bigger lift for another PR, I suspect, but the others on the pr may still want to comment on whether that works on a more tactical basis.

@neonichu
Copy link
Contributor

I think in principle that should work, but IIRC we did not want to depend on the driver outside of the targets that already depend on it to avoid the data model library product taking on that dependency. It looks like Workspace depends on SPMBuildCore which needs the triple, so that would violate that constraint.

@abertelrud might have more context.

@3405691582 3405691582 force-pushed the UnversionTripleInBootstrap branch 3 times, most recently from 5c034dd to 858b97a Compare August 17, 2021 22:22
@3405691582 3405691582 force-pushed the UnversionTripleInBootstrap branch 2 times, most recently from b7e2e59 to eb54bad Compare September 8, 2021 14:05
@3405691582 3405691582 force-pushed the UnversionTripleInBootstrap branch from eb54bad to 7ea746d Compare September 29, 2021 03:09
@3405691582 3405691582 force-pushed the UnversionTripleInBootstrap branch from 7ea746d to 69d8f60 Compare September 29, 2021 13:42
The target flag should take the versioned triple, otherwise on platforms
with versioned triples, the standard library won't be found.

On Darwin, the unversioned triple should still be used throughout. This
necessitates special-casing in the bootstrap script and when making
subdirectories during the build.

This platform-specific branch is encapsulated in a small extension
on Triple in swiftpm. All tests that use the `tripleString` to construct
the `.build` subdirectory are updated accordingly.

See TSC pr swiftlang#229.
@3405691582 3405691582 force-pushed the UnversionTripleInBootstrap branch from 69d8f60 to 685885a Compare September 29, 2021 13:44
@finagolfin
Copy link
Member

Does this need to be run through the CI too or can the three pulls be merged together now?

@neonichu
Copy link
Contributor

@finagolfin
Copy link
Member

Ping, these three pulls appear ready, two just need to be approved before all are merged together.

@neonichu
Copy link
Contributor

neonichu commented Oct 6, 2021

Yep, I am planning to take a final look this week and approve.

@neonichu
Copy link
Contributor

neonichu commented Oct 6, 2021

All three PRs look good to me, we should wait for Ben to approve swiftlang/sourcekit-lsp#430 and then we can land this.

@neonichu neonichu merged commit ef59ec2 into swiftlang:main Oct 18, 2021
3405691582 added a commit to 3405691582/swift-driver that referenced this pull request Oct 29, 2022
Some platforms require the normal, versioned triple to be supplied for
the target flag, as Swift modules use this versioned triple as a
subdirectory. If the unversioned triple is used instead, the standard
library won't be found on these platforms.

Instead, use the unversioned triple only on Darwin. This also should be
a noop on Linux. This behavior matches swiftlang/swift-package-manager#3576
and swiftlang/swift-tools-support-core#229 which contain additional context.
3405691582 added a commit to 3405691582/swift-driver that referenced this pull request Oct 29, 2022
Some platforms require the normal, versioned triple to be supplied for
the target flag, as Swift modules use this versioned triple as a
subdirectory. If the unversioned triple is used instead, the standard
library won't be found on these platforms.

Instead, use the unversioned triple only on Darwin. This also should be
a noop on Linux. This behavior matches swiftlang/swift-package-manager#3576
and swiftlang/swift-tools-support-core#229 which contain additional context.
3405691582 added a commit to 3405691582/swift-driver that referenced this pull request Oct 29, 2022
Some platforms require the normal, versioned triple to be supplied for
the target flag, as Swift modules use this versioned triple as a
subdirectory. If the unversioned triple is used instead, the standard
library won't be found on these platforms.

Instead, use the unversioned triple only on Darwin. This also should be
a noop on Linux. This behavior matches swiftlang/swift-package-manager#3576
and swiftlang/swift-tools-support-core#229 which contain additional context.
3405691582 added a commit to 3405691582/swift-driver that referenced this pull request Oct 29, 2022
Some platforms require the normal, versioned triple to be supplied for
the target flag, as Swift modules use this versioned triple as a
subdirectory. If the unversioned triple is used instead, the standard
library won't be found on these platforms.

Instead, use the unversioned triple only on Darwin. This also should be
a noop on Linux. This behavior matches swiftlang/swift-package-manager#3576
and swiftlang/swift-tools-support-core#229 which contain additional context.
MaxDesiatov pushed a commit to swiftlang/swift-driver that referenced this pull request Jan 5, 2023
Some platforms require the normal, versioned triple to be supplied for
the target flag, as Swift modules use this versioned triple as a
subdirectory. If the unversioned triple is used instead, the standard
library won't be found on these platforms.

Instead, use the unversioned triple only on Darwin. This also should be
a noop on Linux. This behavior matches swiftlang/swift-package-manager#3576
and swiftlang/swift-tools-support-core#229 which contain additional context.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants