Disable traits for imported runtime#878
Conversation
|
If we wait until 6.3 for this we can avoid the split manifest right @czechboy0? If we really want to do it before I'd prefer we use compiler conditionals inside the package manifest. |
|
@simonjbeaumont Yeah, traits were introduced in 6.1 It would be great if we could get this in before... we cannot disable the "FullFoundation" trait in packages where we depend on the generator and the runtime atm. |
|
I'm sympathetic to the motivation to get this over the line. Let's see how we can get it done with minimal fuss. I triggered the CI. Looks like there's some issues. Might be worth experimenting locally with I think sticking to the single package manifest might make this simpler to play around with a get it working locally. |
|
@simonjbeaumont Thanks, will take a look at it locally. Hm, I am not sure if because that will still fail when parsing the package description: |
|
Hm. We've used compiler conditions in manifests before (https://github.com/swift-otel/swift-otel/blob/main/Package.swift) but I guess that was for the same compiler major version. Have you tried with |
|
@simonjbeaumont yeah, |
|
@madsodgaard Could you look at this a bit more. I just managed to get this to compile. I added this extension near the end of the Then using this in the package dependencies: With this patch, I have no problems compiling the package with |
|
@simonjbeaumont Hm, I cannot get that to work when compiling on MacOS with just I am guessing you are also keeping // swift-tools-version:6.0
// ...
.package(url: "https://github.com/apple/swift-openapi-runtime", from: "1.11.0", traitsIfAvailable: []),
// ...
extension Package.Dependency {
static func package(url: String, from version: Version, traitsIfAvailable traits: [String]) -> Package.Dependency {
#if swift(>=6.1)
.package(url: url, from: version, traits: Set(traits.map(Trait.init(stringLiteral:))))
#else
.package(url: url, from: version)
#endif
}
}results in: with |
|
I left it as it was ( Seems that bumping the minimum tools version to 6.0 is causing problems. Why don't we split that out so that you can land this patch? |
|
@simonjbeaumont Just to make sure I understood. You were suggesting to keep |
|
Yes. If you're in a rush to land this PR before 6.3 is released, we can do it that way. Once 6.3 is released this workaround is moot but I'd really rather avoid multiple manifests. |
|
@simonjbeaumont Unfortunately it does not seem that trick actually works when you build. shows that This does not happen when we use multiple manifests versions, so I guess we are forced to do that. |
| @@ -0,0 +1,167 @@ | |||
| // swift-tools-version:6.0 | |||
There was a problem hiding this comment.
main currently supports 5.10+, but I guess this PR drops support for 5.10 and moves to 6.0. #867 does this as well.
Do you want me to revert this to 5.10?
What version did you build this on? root@946be1992f3c:/pwd# swift --version
Swift version 6.2.4 (swift-6.2.4-RELEASE)
Target: aarch64-unknown-linux-gnu
root@946be1992f3c:/pwd# rm -rf .build && swift build -v --target PetstoreConsumerTestCore 2>&1 | grep "OpenAPIRuntime.build" | grep -o "\-DFullFoundation" | head -3
root@946be1992f3c:/pwd# echo $?
0root@223d66ebbf84:/pwd# swift --version
Swift version 6.0.3 (swift-6.0.3-RELEASE)
Target: aarch64-unknown-linux-gnu
root@223d66ebbf84:/pwd# rm -rf .build && swift build -v --target PetstoreConsumerTestCore 2>&1 | grep "OpenAPIRuntime.build" | grep -o "\-DFullFoundation" | head -3
root@223d66ebbf84:/pwd# echo $?
0% swift --version
swift-driver version: 1.127.15 Apple Swift version 6.2.4 (swiftlang-6.2.4.1.4 clang-1700.6.4.2)
Target: arm64-apple-macosx26.0
% rm -rf build && swift build -v --target PetstoreConsumerTestCore 2>&1 | grep "OpenAPIRuntime.build" | grep -o "\-DFullFoundation" | head -3
% echo $?
0 |
|
@simonjbeaumont Not sure if it was a typo, but in your Mac terminal you have I am seeing the issue on 6.2.4 as well. |
|
Heh, I had removed the build directory, but... what I hadn't done (facepalm) is updated the swift-openapi-runtime dependency. I'm very surprised this cannot be done in the package manifest since I have already linked a concrete example where we do use compiler conditionals to change behaviour. I played around some more and agree this isn't feasible. Even with some runtime checks, it seems that Well, thanks for at least entertaining the idea. Let me kick the CI on the current state of the PR and we can go from there. I guess this additional manifest will have a short shelf-life anyway. |
|
@simonjbeaumont I reverted the minimum version to 5.10 and set the language mode to v5 for 6.0+, this is to match what is currently in main. And I am assuming that #867 will be merged later to upgrade 6.0+ |
Is this missing from the commit? |
|
@simonjbeaumont 🤦 sorry, forgot to push |
Co-authored-by: Si Beaumont <simonjbeaumont@gmail.com>
simonjbeaumont
left a comment
simonjbeaumont
left a comment
There was a problem hiding this comment.
This LGTM now, if we can get all the CI to pass.
Thanks @madsodgaard for humouring me on this one—I am very aware I led you on a wild goose chase, so thank you for trying to get it done without the additional manifest, and sorry for the extra time that investigation caused.
|
@simonjbeaumont No problem! Thanks for the quick feedback and turnaround:) The remaining CI failures look Windows deamon related? |
### Motivation In #878 we split the manifest for this project so we could have the 6.1+ manifest declare a dependency on swift-openapi-runtime without default traits to support projects that want to avoid linking the full Foundation transitive dependency. The consequence of this is that the newer manifest lifted the `swift-tools-version`. There was an attempt to mitigate that by marking the Swift language version as v5 to decouple it from moving to 6. However, what this meant was we started getting warnings in the plugin code because plugin targets cannot have `swiftSettings` applied to them so they started complaining about the use of the deprecated `PackagePlugin.Path` APIs. We previously could not move to URL, as requested by the deprecation warning, because we were still supporting Swift 5.10. But 5.10 dropped out of our support window 6 months ago. We haven't been CI'ing it, but we never lifted the tools version in the package manifest. We can do that now, and then move to using `URL` in the plugin code, which should get rid of the warnings. ### Modifications Bump the minimum version of Swift to 6.0. However, we still retain a split manifest because traits are only supported in 6.1+. Then move to using URL instead of PackagePlugin.Path APIs. ### Result Plugin should stop generating warnings for adopters on 6.1+. ### Test Plan Integration tests still pass. Built locally and no longer see the warnings. ### Related Expect this to fix #881
Disables the default traits for OpenAPIRuntime, so that anyone that depends on urlsession transport, can choose disable the default. runtime traits Related: * swift-server/swift-openapi-async-http-client#59 * apple/swift-openapi-generator#878 --------- Co-authored-by: Si Beaumont <beaumont@apple.com>
2 participants
Disables the default traits for the
openapi-runtime, to make sure that other packages depending on the generator can actually disable the traits if they want to.