WIP: Add linkage of SwiftSyntax, to set the stage for future PRs that need it. #3669
Conversation
abertelrud
requested review from
friedbunny,
neonichu and
tomerd
as code owners
|
cc @owenv |
| @@ -33,6 +33,10 @@ let swiftPMDataModelProduct = ( | |||
| ] | |||
| ) | |||
|
|
|||
| #if compiler(>=5.5) | |||
There was a problem hiding this comment.
should we just require 5.5 for SwiftPM?
There was a problem hiding this comment.
If we did, it would allow dropping some of the conditional compilation in the manifest, but some would still be necessary elsewhere to account for SwiftSyntax being missing from the first stage cmake build when bootstrapping. It's probably still worth doing so long as it doesn't interfere with SourceKit-LSP (which currently has a minimum of 5.3).
There was a problem hiding this comment.
I think some of the toolchain CI is still using an older Swift version, so that might be problematic. But it would be great to start being able to rely on synthesized Codable of enums in particular, and that requires 5.5.
| @@ -33,6 +33,10 @@ let swiftPMDataModelProduct = ( | |||
| ] | |||
| ) | |||
|
|
|||
| #if compiler(>=5.5) | |||
There was a problem hiding this comment.
If we did, it would allow dropping some of the conditional compilation in the manifest, but some would still be necessary elsewhere to account for SwiftSyntax being missing from the first stage cmake build when bootstrapping. It's probably still worth doing so long as it doesn't interfere with SourceKit-LSP (which currently has a minimum of 5.3).
abertelrud
force-pushed
the
eng/link-against-swiftsyntax
branch
from
55459b8 to
4001857
Compare
|
@swift-ci please smoke test |
Package.swift
Outdated
| // should compile with any 5.5 compiler, it will only be functional when built | ||
| // with a toolchain that has a compatible parser library. | ||
| if ProcessInfo.processInfo.environment["SWIFTCI_USE_LOCAL_DEPS"] == nil { | ||
| package.dependencies += [.package(url: "https://github.com/apple/swift-syntax.git", .branch("release/5.5"))] |
There was a problem hiding this comment.
In the long run, I think it would be better to target main/relatedDependenciesBranch here. That should ensure that the smoke-testing bot will always use a compatible parser lib and SwiftSyntax, which IMO is the most important configuration to always run tests in. For a self-hosted build, I think it's ok to assume the libraries will sometimes be incompatible and skip the test if so, since that config isn't used for distribution.
There was a problem hiding this comment.
Agreed — I wasn't able to get that to work with being able to use any version of Xcode to work on SwiftPM at desktop unfortunately (as the newest Xcode (13.0) has Swift 5.5). So it's not just the self-hosted tests, it's also being able to run the tests locally, where it's less desirable to skip them.
|
I guess the: is coming from using the |
|
I've been thinking about this some more the past couple days and had a horrible hacky idea that might nonetheless unblock this in the short term. Right now, it looks like macOS is the only platform where we can't create a working executable/test bundle with certain toolchains. Maybe we could provide a binary target (to force dynamic linking) conditionalized to macOS which is ABI compatible with the parser library and has the same name. If it is able to dlopen the parser library, it would just forward all of the calls, and otherwise it would return an incompatible version hash, and tests would fail but we'd still produce a working executable. Running the tests locally would still require downloading a compatible toolchain, but I think it's fine to require that if somebody is going to modify a SwiftSyntax-based feature. |
@owenv that's pretty clever, and would require considerably less lead time than extending the binary target format to allow SwiftSyntax to deliver the real binary as a plain library on all platforms. I guess then it would come down to how often that shim would have to be revved, as the ABI evolves. I guess SwiftPM is in control of that since it would determine when to depend on a new version of SwiftSyntax. This new binary target would be in SwiftPM itself, and not SwiftSyntax, right? |
… need it. The way in which this is done is based on swiftlang#3034 but it's possible we'll need to tweak it. It's not great to only get the dependency in 5.5 or later but there might not be much to do about that if the compiler ABI changed.
abertelrud
force-pushed
the
eng/link-against-swiftsyntax
branch
from
4001857 to
416e69f
Compare
|
@abertelrud I think this can be closed with @neonichu work to bring in the new parser? |
The way in which this is done is based on #3034 but it's possible we'll need to tweak it. It's not great to only get the dependency in 5.5 or later but there might not be much to do about that if the compiler ABI changed.
The idea here is to shake out the issues around this separately, so that the PRs that need SwiftSyntax can focus on just their changes.