[SE-0301] Package Editor Commands Implementation #3034
Conversation
owenv
requested review from
abertelrud,
ankitspd,
friedbunny and
neonichu
as code owners
|
@swift-ci please smoke test |
owenv
force-pushed
the
package-editor-modernization
branch
from
34877c5
to
60d418a
Compare
|
@swift-ci please smoke test |
|
Hmm, I'm seeing this failure when the library tries to load a manifest: Other than that, everything seems to work as expected on CI Edit: I think I managed to fix it by passing a UserToolchain into PackageEditor instead of trying to create one manually |
owenv
force-pushed
the
package-editor-modernization
branch
from
60d418a
to
5b26311
Compare
|
@swift-ci please smoke test |
|
Looks like the newly reenabled tests ran and passed in the most recent run (but don't run in the self-hosted CI, as expected) |
|
Thanks for updating these! Right, passing in a toolchain seems like the right approach, as long as this is within SwiftPM. It would make sense to me for this to be in a separate package, except that (I believe) the intent was to make this functionality part of the core SwiftPM commands. |
There was a problem hiding this comment.
Looks good to me but I am not familiar with code, so I'd want @aciidb0mb3r to weigh in also.
|
I ended up finding a couple more bugs, so I merged the changes from #3035 into this PR to consolidate the package editing changes |
owenv
force-pushed
the
package-editor-modernization
branch
from
81ff1f7
to
569ad0d
Compare
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
owenv
force-pushed
the
package-editor-modernization
branch
from
7262b4c
to
5182496
Compare
|
@swift-ci please smoke test |
Package.swift
Outdated
| // Because SwiftSyntax is closely tied to the compiler, only attempt to build | ||
| // the package editor library if we're in a build-script environment and can | ||
| // assume we're using a just-built compiler and SwiftSyntax library. | ||
| if ProcessInfo.processInfo.environment["SWIFTCI_USE_LOCAL_DEPS"] != nil { |
There was a problem hiding this comment.
Perhaps we should have some additional control over whether to require swift-syntax and a matching compiler. SWIFTCI_USE_LOCAL_DEPS is the same environment variable that controls whether to fetch the other dependencies or expect to find them, and it would be good to have independent control over whether to try to build the package editor.
There was a problem hiding this comment.
👍 I updated this to use SPM_ENABLE_PACKAGE_EDITOR instead, and the bootstrap script passes that for the stage 2 build. We now also define ENABLE_PACKAGE_EDITOR when building the Commands module, and I'm conditionally compiling the new subcommands there instead of breaking them out into a separate executable swift-package would need to forward to. This setup should keep the build relatively simple as long as we're ok not supporting this functionality in stage 1 CMake builds, which IMO is fine (and then we don't need to setup CMake for SwiftSyntax either).
|
@swift-ci please smoke test |
owenv
changed the title
|
@swift-ci please smoke test |
|
@swift-ci please test |
|
@swift-ci please build toolchain macOS |
|
@swift-ci please build toolchain |
|
All of the functionality in this PR should be complete now, but the scope of the changes got a little out of hand 😭. I'm going to work on finishing the SE proposal and then I'll see if there's a good way to break this down into pieces that are easier to review. |
|
Thanks a lot for your additional investigations here!
I don't actually know at all how that works — @shahmishal would probably know, but I don't know how much information is. As far as including libSwiftSyntax.dylib in the Xcode toolchain, that might be something to explore, since there's a precedent for that with llbuild. But my impression was that it's lib_InternalSwiftSyntaxParser.dylib that needs to match the compiler, and that SwiftSyntax could generally be built from source, although I'm sure there are problems if they are too far apart. I suppose it's no different than relying on an llbuild that's in the Xcode toolchain, in that its API sometimes changes too. I guess my main concern with only building the mechanical editing support when SwiftSyntax is present is that it's very easy to break it. It's an interesting idea to use executables but I agree that it's probably not the way to go in the long run here. I will look into what can be done to have a libSwiftSyntax.dylib in the toolchain. |
owenv
force-pushed
the
package-editor-modernization
branch
2 times, most recently
from
dec198d
to
0464ef6
Compare
|
@swift-ci please smoke test |
|
@swift-ci please smoke test macos |
|
the macOS self-hosted bot has an unrelated failure, but everything else appears to be passing. @abertelrud The latest push reworks the build again, I'd be interested in hearing your thoughts on the approach - now PackageSyntax is always built if the compiler version is >=5.5. This works around the parser library ABI break between 5.4 and 5.5 that was previously causing issues when developing locally. Also, a local checkout of SwiftSyntax is no longer required unless SWIFTCI_USE_LOCAL_DEPS is set. Now:
|
|
@swift-ci please smoke test macOS |
| // 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(relatedDependenciesBranch))] |
There was a problem hiding this comment.
Should this be pinned to a particular release of SwiftSyntax rather than the branch? The toolchain build will use the branch, but it will presumably also set SWIFTCI_USE_LOCAL_DEPS.
There was a problem hiding this comment.
I think we should use the branch here - it ensures we only ever need to support one version of the SwiftSyntax API, which changes pretty often
There was a problem hiding this comment.
But if it changes often, doesn't that mean that changes to the branch could suddenly break SwiftPM? i.e. would it not make sense to specify a semantic version and then when necessary update SwiftPM to use the newer version at the same time as adopting any necessary changes, in a single PR?
There was a problem hiding this comment.
I believe SwiftSyntax CI builds SwiftPM so I think we'd at least catch breaking changes pre-merge. The main problem I'm running into is that SwiftPM always needs to build against top-of-tree SwiftSyntax for the feature to work in nightly toolchains. We could also support a specific tag for local development, but it wouldn't be guaranteed to compile 100% of the time
Thanks a lot, @owenv! Especially for taking into account the changes that will be needed to work with Xcode; that's what seems to me to be the trickiest aspect here. Skipping PackageSyntax with Swift 5.4 makes sense to me, though hopefully we won't have to do that moving forward. Do you think we'll be in that situation whenever there are changes in the parser library (in other words, are ABI breaks like the one you mentioned a regular thing?). Statically linking SwiftSyntax also makes sense — I think it would still be a good separate improvement for SwiftPM to allow binary targets to contain whatever libraries are needed, but that can be a future improvement. |
@abertelrud If there's another break, it'll probably require a similar workaround. However, the parser library rarely has breaking changes even if it doesn't guarantee stability. The one in 5.5 is the first one since SwiftSyntax was open-sourced a couple years back. |
|
(Reengaging here, so we can try to get towards landing this — that you for your patience @owenv!)
This is just referring to the tests for PackageSyntax itself, right, which don't normally run as part of building SwiftPM? Or would this also affect SwiftPM's own tests? |
This will affect the SwiftPM tests, but only when building with an incompatible swift >= 5.5. I may be able to add a public SwiftSyntax API so that we can skip these tests in that scenario. Unfortunately, I'm running into another problem with the local development now. Building with the SwiftPM CLI works fine, but Xcode 13 must be linking differently because swift-package crashes on launch with |
Thank you — if you add the JIRA here I can take a look as well. I am not aware of any changes, but also I haven't worked with SwiftSyntax before. I am keen to help get this landed — it's just the ergonomics and robustness of working with this new dependency that need to be worked out. |
|
It turns out that the problem I'm running into is that the SwiftPM 5.5 CLI includes |
Thanks @owenv that's news to me — I will look into what that's all about and see if I can come up with something (I know this PR has taken forever to land — apologies for that, there many things going on). |
|
I've been looking into this and I'm not sure that the change to no longer include that |
This commit adds a new module, PackageSyntax, which supports making mechanical edits to Package.swift manifests. It also exposes a CLI interface for this functionality [PackageSyntax] Adapt to changes in identity and package plugin APIs [PackageSyntax] Don;t include name argument in package dependency descriptions on 5.5+ [PackageSyntax] Improve tools version coverage of PackageEditor end to end tests [PackageSyntax] Adapt to more identity API changes [PackageSyntax] Use new branch and revision convenience methods on 5.5+ [PackageSyntax] Run the bootstrap script without PackageSyntax if SwiftSyntax isn't checked out or --skip-package-syntax is passed Revert "[PackageSyntax] Run the bootstrap script without PackageSyntax if SwiftSyntax isn't checked out or --skip-package-syntax is passed" This reverts commit 6f0a920.
owenv
force-pushed
the
package-editor-modernization
branch
from
0464ef6
to
234303e
Compare
|
@swift-ci smoke test |
|
I've decided to move this over to https://github.com/owenv/swift-package-editor as a standalone package since maintaining it out-of-tree for so long just ended up being too much work. I'm willing to transfer ownership of that repo if there's still interest in including this feature in the toolchain - otherwise I think I'll just ship a separate installer. Thanks everyone for your help with this! Sorry things didn't work out as expected |
… it. The way in which this is done is based on apple#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.
… need it. The way in which this is done is based on apple#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.
… need it. The way in which this is done is based on apple#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.
… need it. The way in which this is done is based on apple#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.
|
Since the proposal has been accepted, is that separate repository a preferred implementation of it? I wonder if proposal status should be updated if there are no immediate plans to bring that feature into the toolchain. |
|
We have #5802 |
This PR:
Possible future directions (not in this PR):
swift package upgradecommand that could update requirement specs