Bootstrap script needs to build PackageDescription and PackagePlugin universal #3431
Conversation
abertelrud
requested review from
friedbunny,
neonichu and
tomerd
as code owners
|
I'm having some trouble building swift-crypto with the bootstrap script even without my changes. Putting this up for discussion while I figure that out. |
|
@swift-ci please smoke test |
|
cc @compnerd |
+1 imo we should work to deprecate the 4 support and only support 4_2 |
I would agree, since this is a continuing source of work, but last I checked there were plenty of packages that still used tools version 4. We should perhaps do another pass to check on that. |
Quite separately from that though, I think this does illustrate the limitations of what package manifests can currently express. For example, there should definitely be a way to set the module name explicitly, independently of the target name, and ideally there should be a way to have two targets that can build different versions of the same underlying source base. Right now if you try to point two targets at the same sources (for example to build them two ways), there are errors about overlapping sources that seem somewhat unnecessary. We don't want to add a lot of complexity but could perhaps also use this to consider how to make the rules a little more flexible. |
|
The self-hosted Linux test seems to be running into the same thing I'm seeing locally, which is an invocation returning |
Ran a quick analysis on the data from swiftpackageindex.com: So roughly a quarter of the packages are using pre-4.2 tools versions. |
Thanks, Boris. That's roughly what I thought. The differences between 4 and 4_2 don't look awful — mostly it's enum changes, which seem bad but which I think can be handled now that different enum cases support individual availability annotations. One case that looks tricky is that a property changed types, but perhaps there's something that can be done there too. In any case, that would only resolve part of this, but would at least avoid the ugliness of a second build just to build the alternate version of PackageDescription. |
Right, I think the split into two libraries is mostly a legacy thing, because at the time we did not have the package description availability support in the compiler, so the way we controlled availability was through separate libraries. Once we got it, we never went back to consolidate the two libraries. |
There are some tricky aspects, though, like a single property needing to be both a string and an enum, which isn't achievable using availability AFAIK. |
this is interesting data, cc @kylemacomber @lorentey |
|
Very interesting. I wonder how many of these simply reflect the toolchain version at the time the package was first published. Anecdotally, of my old packages, BigInt upgraded the manifest to 5.0 with a major release bump, but Attabench, Btree, Deque and SipHash are stuck on 4.0, reflecting the major toolchain release at the time I joined Apple. Previously, I bumped the manifest version when a major new Swift release came out. (Sometimes even in a minor release, for which I was justly admonished.) I don't believe there are many people actively relying on 4.0 toolchains, but most these packages are on life support, so ideally they should keep working without a source-level update. |
|
It seems pretty clear that we can't drop support for 4.0. I've made some progress on a unified PackageDescription, but there are a few wrinkles — I don't think it breaks any conventional use of the API, but it might allow some things that would be otherwise disallowed, such as use of I'm sure it can be iterated on, so I will put up a WIP PR for that and then we can run it through some of the test packages to make sure nothing is broken. If we can get it to where there's just the one version of the runtime library, that would simplify all of the various bootstrap scripts we have. |
|
Put up some preparatory work to unify PackageDescription here: #3464 |
#3464 is now ready for review. It unifies 4.0 and 4.2 using availability annotations, along with a variety of other changes to make things work. I am going to rebase this PR on that one, since it now means that we can use |
abertelrud
force-pushed
the
eng/pkgdesc-and-pkgplug
branch
from
c3b1c8c to
60c7a44
Compare
abertelrud
changed the title
Utilities/bootstrap
Outdated
| @@ -651,6 +635,8 @@ def build_swiftpm_with_swiftpm(args, integrated_swift_driver): | |||
| if integrated_swift_driver: | |||
| swiftpm_args.append("--use-integrated-swift-driver") | |||
|
|
|||
| swiftpm_args.append("--enable-parseable-module-interfaces") | |||
There was a problem hiding this comment.
This currently results in warnings for any modules that are not built with library evolution enabled (although nothing harmful happens). I’ll take a look at the logic in SwiftPM to see whether this flag should be conditional on whether the module is built with that flag.
|
@swift-ci please smoke test |
|
@swift-ci please smoke test linux |
Not 100% sure what's going on here. It definitely looks related to this change, but not sure why libFoundation would no longer be found. |
|
@swift please smoke test |
|
Looks like the CI didn't run? |
|
@swift please smoke test |
Thanks for pointing that out — I still see the same thing with a fresh attempt. Will see what's going on. |
|
@swift-ci please test |
|
@swift-ci please smoke test |
I accidentally typed |
|
Looks like this works. I never looked into doing separate builds for particular build products, would be great if that works. This pull should fix SR-14521. |
|
Ready to go? I'm waiting on this to go in before getting #3403 in. |
|
@buttaface Sorry for my slow responses here. I will take a closer look at this today since, IIRC, I had some concerns about whether the |
abertelrud
force-pushed
the
eng/pkgdesc-and-pkgplug
branch
from
078a755 to
9e864c4
Compare
|
@swift-ci please smoke test |
|
I believe the latest changes here address the issue. It's not excellent to rebuild PackageDescription and PackagePlugin with swiftinterface generation enabled, but it's also not a very big deal, as they are small. Most likely SwiftPM will need to gain more control over how each target is built in order to do better, but that will require an evolution proposal or at least a pitch to add more settings to the manifest. |
|
I've been trying to get this landed, but have run into several issues that I'm working through:
It's a bit unfortunate that building for more than one architecture switches to the XCBuild build system, which can bring about a broader change in semantics. I'm looking to see how much work it would be to support multiple architectures in the old build system. Doing it the correct way, with separate compilation for each architecture and a separate CreateUniversalBinary command, is a bit of work — but SwiftPM doesn't currently support per-architecture settings, and so perhaps passing multiple architectures to the driver invocation will get us far enough in the short term. The idea would be to support x86_64 and arm64 without having to completely switch build systems. Meanwhile it might be possible to merge this even if it temporarily means that no .swiftinterface is generated (only swiftmodule). This is because in a toolchain, the compiler that generated the SwiftPM package libraries is expected to also be the one that uses it. It would only be a temporary solution, however, since we really should be generating the .swiftinterface. |
|
I don't know much about SPM on macOS, but since you already split off two more SPM commands for the PD and PP libraries, what about brute-forcing it and invoking SPM four times, once for each library and arch? Would that avoid the issues with "building for multiple architectures" and switching to XCBuild? Just a potential workaround, but it might allow you to move forward with this then come back and unify it later. |
Thats not a bad idea — I'll give that a try. Yes, I definitely want to land this ASAP even if there are loose ends to tie up, so as to not impede progress. Possibly we can get by with just the .swiftmodule files for a short while. |
…libraries universal when cross-compiling. Also unify and clean up some of the logic by making the helper function that installs libSwiftPM be more generic, and also apply to PackageDescription and PackagePlugin. rdar://75186958
abertelrud
force-pushed
the
eng/pkgdesc-and-pkgplug
branch
from
9e864c4 to
a9a42ca
Compare
|
Removed the |
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
The bootstrap script needs to build the PackageDescription and PackagePlugin runtime support libraries universal when cross-compiling. This PR builds on #3464, so it will be easier to review that PR first.
Motivation
Even when invoking the bootstrap script with flags to build universal for macOS x86_64 and arm64, the PackageDescription and PackagePlugin libraries ended up single-architecture. This is because the bootstrap script copied them from the CMake-built artifacts instead of the swiftpm-built artifacts.
PR #3464 resolves the complication of PackageDescription having both a 4.0 and a 4.2 variant.
Modifications:
rdar://75186958