[SE-0301] Package Editor Commands Implementation #3034

owenv · 2020-11-08T02:34:52Z

This PR:

Updates the SPMPackageEditor library so it compiles against ToT SwiftSyntax
Removes spm-manifest-tool, instead conditionally compiling package editor functionality into the Commands module
Refactors the library a bit to fix some bugs and make it more extensible
adds support for adding products

Possible future directions (not in this PR):

deleting products/targets/dependencies
a swift package upgrade command that could update requirement specs
Use the PackageSyntax infra to emit manifest-related diagnostics with source locations

owenv · 2020-11-08T02:35:03Z

@swift-ci please smoke test

owenv · 2020-11-08T03:23:26Z

@swift-ci please smoke test

owenv · 2020-11-08T04:03:32Z

Hmm, I'm seeing this failure when the library tries to load a manifest:
invalidManifestFormat("/var/folders/_8/79jmzf2142z2xydc_01btlx00000gn/T/TemporaryFile.Br55su.swift:2:8: error: no such module \'PackageDescription\'\nimport PackageDescription\n ^", diagnosticFile: nil)

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 · 2020-11-08T05:43:07Z

@swift-ci please smoke test

owenv · 2020-11-08T07:07:16Z

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)

abertelrud · 2020-11-09T18:24:53Z

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.

abertelrud

Looks good to me but I am not familiar with code, so I'd want @aciidb0mb3r to weigh in also.

owenv · 2020-11-15T19:29:21Z

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 · 2020-11-15T19:35:53Z

@swift-ci please smoke test

owenv · 2020-11-18T06:16:03Z

@swift-ci please smoke test

owenv · 2020-11-19T05:47:13Z

@swift-ci please smoke test

abertelrud · 2020-11-19T08:10:33Z

Package.swift

+// 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 {


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.

👍 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).

owenv · 2020-11-20T05:05:56Z

@swift-ci please smoke test

owenv · 2020-11-24T06:11:28Z

@swift-ci please smoke test

owenv · 2020-11-27T01:57:46Z

@swift-ci please test

owenv · 2020-11-27T01:57:58Z

@swift-ci please build toolchain macOS

owenv · 2020-11-27T02:15:59Z

@swift-ci please build toolchain

owenv · 2020-11-27T22:14:02Z

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.

owenv · 2021-06-12T01:35:17Z

@swift-ci please smoke test

owenv · 2021-06-12T03:34:44Z

@swift-ci please smoke test macos

owenv · 2021-06-12T04:38:24Z

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:

When building locally with a 5.4 or earlier compiler, PackageSyntax is skipped
When building locally with a 5.5 or later compiler (this requires Xcode 13 beta since Xcode ignores custom toolchains when compiling a manifest):
- If the parser library in the toolchain is compatible, PackageSyntax will compile and pass tests
- If the parser library in the toolchain is incompatible, PackageSyntax will still compile and link, but its tests will fail
When bootstrapping in CI, PackageSyntax will compile as part of the second-stage build and pass tests
When building a toolchain, SwiftSyntax will be statically linked, and will dynamically link against the lib_InternalSwiftSyntaxParser installed in the toolchain. This increases binary size a bit, but it should work with Xcode's layout.

owenv · 2021-06-14T04:28:22Z

@swift-ci please smoke test macOS

abertelrud · 2021-06-14T20:21:37Z

Package.swift

+// 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))]


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.

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

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?

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

abertelrud · 2021-06-14T20:24:23Z

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:

When building locally with a 5.4 or earlier compiler, PackageSyntax is skipped

When building locally with a 5.5 or later compiler (this requires Xcode 13 beta since Xcode ignores custom toolchains when compiling a manifest):

If the parser library in the toolchain is compatible, PackageSyntax will compile and pass tests

If the parser library in the toolchain is incompatible, PackageSyntax will still compile and link, but its tests will fail

When bootstrapping in CI, PackageSyntax will compile as part of the second-stage build and pass tests

When building a toolchain, SwiftSyntax will be statically linked, and will dynamically link against the lib_InternalSwiftSyntaxParser installed in the toolchain. This increases binary size a bit, but it should work with Xcode's layout.

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.

owenv · 2021-06-15T03:11:11Z

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?).

@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.

abertelrud · 2021-06-21T19:56:47Z

(Reengaging here, so we can try to get towards landing this — that you for your patience @owenv!)

If the parser library in the toolchain is incompatible, PackageSyntax will still compile and link, but its tests will fail

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?

owenv · 2021-06-22T05:09:56Z

If the parser library in the toolchain is incompatible, PackageSyntax will still compile and link, but its tests will fail
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 Library not loaded: @rpath/lib_InternalSwiftSyntaxParser.dylib. I haven't figured out what changed from Xcode 12 yet but I'll see if I can reduce it and report a bug.

abertelrud · 2021-06-22T17:54:39Z

If the parser library in the toolchain is incompatible, PackageSyntax will still compile and link, but its tests will fail
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 Library not loaded: @rpath/lib_InternalSwiftSyntaxParser.dylib. I haven't figured out what changed from Xcode 12 yet but I'll see if I can reduce it and report a bug.

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.

owenv · 2021-06-26T05:04:40Z

It turns out that the problem I'm running into is that the SwiftPM 5.5 CLI includes /Applications/Xcode-beta.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/macosx as an rpath but Xcode 13 doesn't when building packages, so lib_InternalSwiftSyntaxParser.dylib can't be loaded at runtime. I think it might have been an intentional change but I'm not really sure how to work around it.

abertelrud · 2021-06-28T19:19:49Z

It turns out that the problem I'm running into is that the SwiftPM 5.5 CLI includes /Applications/Xcode-beta.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/macosx as an rpath but Xcode 13 doesn't when building packages, so lib_InternalSwiftSyntaxParser.dylib can't be loaded at runtime. I think it might have been an intentional change but I'm not really sure how to work around it.

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).

abertelrud · 2021-07-12T20:53:47Z

I've been looking into this and I'm not sure that the change to no longer include that rpath was intentional. I'm looking to see what the options are for either a workaround or a fix.

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 · 2021-07-25T17:45:13Z

@swift-ci smoke test

owenv · 2021-07-31T21:07:53Z

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 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.

… 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.

MaxDesiatov · 2023-04-13T10:32:17Z

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.

neonichu · 2023-04-13T15:40:14Z

We have #5802

owenv requested review from abertelrud, aciidgh, friedbunny and neonichu as code owners November 8, 2020 02:34

owenv force-pushed the package-editor-modernization branch from 34877c5 to 60d418a Compare November 8, 2020 03:23

owenv force-pushed the package-editor-modernization branch from 60d418a to 5b26311 Compare November 8, 2020 05:42

owenv mentioned this pull request Nov 8, 2020

Rewrite spm-manifest-tool using swift-argument-parser and the updated SPMPackageEditor #3035

Closed

abertelrud approved these changes Nov 9, 2020

View reviewed changes

tomerd assigned DougGregor Nov 10, 2020

owenv force-pushed the package-editor-modernization branch from 81ff1f7 to 569ad0d Compare November 15, 2020 19:35

owenv force-pushed the package-editor-modernization branch from 7262b4c to 5182496 Compare November 19, 2020 05:10

abertelrud reviewed Nov 19, 2020

View reviewed changes

owenv changed the title ~~Update SPMPackageEditor library to work with current toolchains~~ [PackageEditor] Mechanical editing functionality for package manifests Nov 21, 2020

owenv mentioned this pull request Nov 27, 2020

[DNM] Toolchain testing swiftlang/swift#34878

Closed

owenv mentioned this pull request May 30, 2021

RFC: Remove lib_InternalSwiftSyntaxParser as a link-time dependency swiftlang/swift-syntax#290

Closed

owenv force-pushed the package-editor-modernization branch 2 times, most recently from dec198d to 0464ef6 Compare June 12, 2021 01:35

abertelrud reviewed Jun 14, 2021

View reviewed changes

owenv force-pushed the package-editor-modernization branch from 0464ef6 to 234303e Compare July 25, 2021 17:44

owenv closed this Jul 31, 2021

owenv mentioned this pull request Aug 7, 2021

Add swift-package-editor to update-checkout swiftlang/swift#38798

Closed

abertelrud mentioned this pull request Aug 16, 2021

WIP: Add linkage of SwiftSyntax, to set the stage for future PRs that need it. #3669

Closed

swiftlang deleted a comment Jun 26, 2024

[SE-0301] Package Editor Commands Implementation #3034

[SE-0301] Package Editor Commands Implementation #3034

Conversation

owenv commented Nov 8, 2020 • edited Loading

owenv commented Nov 8, 2020

owenv commented Nov 8, 2020

owenv commented Nov 8, 2020 • edited Loading

owenv commented Nov 8, 2020

owenv commented Nov 8, 2020

abertelrud commented Nov 9, 2020

abertelrud left a comment

Choose a reason for hiding this comment

owenv commented Nov 15, 2020

owenv commented Nov 15, 2020

owenv commented Nov 18, 2020

owenv commented Nov 19, 2020

abertelrud Nov 19, 2020

Choose a reason for hiding this comment

owenv Nov 20, 2020

Choose a reason for hiding this comment

owenv commented Nov 20, 2020

owenv commented Nov 24, 2020

owenv commented Nov 27, 2020

owenv commented Nov 27, 2020

owenv commented Nov 27, 2020

owenv commented Nov 27, 2020

owenv commented Jun 12, 2021

owenv commented Jun 12, 2021

owenv commented Jun 12, 2021

owenv commented Jun 14, 2021

abertelrud Jun 14, 2021

Choose a reason for hiding this comment

owenv Jun 15, 2021

Choose a reason for hiding this comment

abertelrud Jun 21, 2021

Choose a reason for hiding this comment

owenv Jun 22, 2021

Choose a reason for hiding this comment

abertelrud commented Jun 14, 2021

owenv commented Jun 15, 2021

abertelrud commented Jun 21, 2021

owenv commented Jun 22, 2021

abertelrud commented Jun 22, 2021

owenv commented Jun 26, 2021

abertelrud commented Jun 28, 2021

abertelrud commented Jul 12, 2021

owenv commented Jul 25, 2021

owenv commented Jul 31, 2021

MaxDesiatov commented Apr 13, 2023

neonichu commented Apr 13, 2023

owenv commented Nov 8, 2020 •

edited

Loading

owenv commented Nov 8, 2020 •

edited

Loading