Use the (versioned) triple from swiftc instead. #229
Conversation
3405691582
requested review from
abertelrud,
aciidgh,
friedbunny,
neonichu and
tomerd
as code owners
|
@swift-ci please test |
|
Thanks for the PR, this may have been discussed elsewhere but I'd love to understand what potential impact this has on downstream code, such as SwiftPM. Also, should we a dd a couple of tests for the new functionality? |
Here, swiftc is invoked to get the textual unversioned triple from the compiler. However, TSC wants to use this unversioned triple for the target flag, which causes breakage on the compiler side on platforms with versioned triples, because the compiler can't find the stdlib with the unversioned triple. Instead, default to getting versioned triples. When clients of TSC want to do funky things with unversioned triples, we lop the version numbers off versioned triples to get an unversioned triple. This is somewhat crude, but the alternative is to get both versioned _and_ unversioned triples from swiftc, which is similarly awkward.
3405691582
force-pushed
the
UnversionTriples
branch
from
bcfdacf to
f0e13a6
Compare
|
SPM should be getting platform information through We don't seem to be doing unit testing on Triple in TSC at the moment? I've added some brief tests instead. |
|
@swift-ci please test |
|
@abertelrud wdyt? |
|
@swift-ci please test |
|
@swift-ci please test |
|
Not 100% sure yet while the mac OS test is failing. Looking into it. I don't have a mac OS system to poke at this, so happy to receive some advice here... Also of note is swiftpm pr 3576 but I don't think that change would be of relevance to the test failure here... |
|
Yeah, you probably need to update the triples used in the SPM bootstrap script along with this pull, because it is failing because it cannot find the second build of See earlier discussion about macOS triple requirements, one of the Apple devs will need to guide you through this. |
|
This is why I'm confused, because the SPM bootstrap already uses the unversioned triple (and probably should always use the unversioned triple on mac OS). Maybe I'm missing something... |
|
So here's the way it works:
Note that that last step failed for both of us: I suspect you're having the inverse problem now. You could try sticking a debug line in the bootstrap script to dump the directory layout of the SPM build directory before the tests are run: that will tell you what's going wrong. Stick the debug line in your bootstrap pull if you don't see that build path info already in the CI log, and have these two pulls tested together to figure this out. |
|
Okay, I believe I've updated the right parts of swiftpm to ensure the subdirectory gets created with the correct name. This means swiftlang/swift-package-manager#3576 is required for a correct build on mac OS. Can someone please rerun the tests picking that above PR? |
|
swiftlang/swift-package-manager#3576 |
|
Looks like there's a test that needs an update? |
|
Heh, you just wanted to add the version to the triple for OpenBSD and now you're stuck making a bunch of macOS changes, since it uses versioned triples too. 😃 |
|
Associated SPM pr is updated to address the failing tests. I think that should take care of everything in the failed mac OS build log; sorry for the multiple cycles here. |
|
swiftlang/swift-package-manager#3576 |
|
More failing tests on macOS because of the versioned triple used in the SPM build directory. @neonichu, wouldn't it make more sense to just start using versioned triples in SPM on macOS, what is the downside there? |
|
Oof, I should have just changed all instances of The question as to mac OS is still worthwhile and I can't answer. I am specialcasing Darwin because I'm being cautious and from a cursory reading of the SPM code, I don't know for real whether that cautiousness is warranted or not. |
IIRC, platform versions were added around Swift 4.2 or 5.0, so they didn't always exist. I think we want to retain a shared build directory for all artifacts of a build operation, but since each package carries its own platform version with it, this would no longer be given if we added the version to the name of the build directory. |
I don't know anything about the intricacies of how this already worked on macOS, but I would think having multiple versioned build directories would enable versioning more. For example, on Android, I could build the same Swift package with the I assume the situation is similar on macOS, though I haven't looked at that code as I don't use macOS. |
|
@buttaface Sure, what I am saying is that if you have a package that targets e.g. macOS 11.0 which depends on another that targets macOS 10.0, today both end up in the same build directory (e.g. |
|
OK, I am not familiar with how that works on Darwin, hence my questions so far. I'm fine with whatever you decide regarding the build directory, my main concern is with passing these versioned triples through when compiling. |
|
Ping, just needs a CI run and we can get this in? |
|
@swift-ci please test |
|
@abertelrud, I think this needs to be tested with the SPM pull, as seen above. |
|
swiftlang/swift-package-manager#3576 |
|
Squashed another minor error on the spm side. Please retry, thanks. |
|
swiftlang/swift-package-manager#3576 |
|
Only a single sourcekit-lsp test failed on macOS on the last CI run, a flake? |
|
swiftlang/swift-package-manager#3576 |
|
The exact same test case failed again, @3405691582, what do you think? |
|
Yeah, I'll take a closer look and report back. |
|
I haven't done a full trace through SourceKit, but my guess is that https://github.com/apple/sourcekit-lsp/blob/main/Sources/SKSwiftPMWorkspace/SwiftPMWorkspace.swift#L120 needs to be updated accordingly. I've opened pr swiftlang/sourcekit-lsp#430 on SourceKit-LSP to try and fix this. I don't normally build SourceKit, so I can't immediately test this properly. I can do an integration test tomorrow on a Linux install, or it'd be nice to have a CI run that picks this PR as well to see if this fixes the broken test case. |
|
Squashed another failure on the SPM side (probably something changed in the interim on the CI environment that tickled this gap in the change). Please retry, thank you! |
|
Fixed the SourceKit tests, please retest, thank you! Apologies for the extra noise on this. |
|
Yay, one less flag I have to pass in when cross-compiling SPM or sourcekit-lsp for Android, specifically that linked line passes in Now I can just add it to the target triple passed to SPM and it won't be ignored. 🎆 |
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.
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.
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.
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.
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.
Here, swiftc is invoked to get the textual unversioned triple from the
compiler. However, TSC wants to use this unversioned triple for the
target flag, which causes breakage on the compiler side on platforms
with versioned triples, because the compiler can't find the stdlib with
the unversioned triple.
Instead, default to getting versioned triples. When clients of TSC want
to do funky things with unversioned triples, we lop the version numbers
off versioned triples to get an unversioned triple. This is somewhat
crude, but the alternative is to get both versioned and unversioned
triples from swiftc, which is similarly awkward.