[PackageSyntax] Pt 1 - Setup the PackageSyntax Target #3230
Conversation
owenv
requested review from
abertelrud,
friedbunny,
neonichu and
tomerd
as code owners
|
@swift-ci please smoke test |
| @@ -47,7 +49,7 @@ let package = Package( | |||
| "Build", | |||
| "Xcodeproj", | |||
| "Workspace" | |||
| ] | |||
| ] + (buildPackageSyntax ? ["PackageSyntax"] : []) | |||
There was a problem hiding this comment.
Would it make sense to add these conditionals to the bottom of the manifest, along the lines of the other conditional dependencies etc? That would keep the top of the manifest parseable. That sort of goes to the heart of the question of using SwiftSyntax in the first place — is it useful to recommend a certain manifest layout to make it as parseable as possible, even when that means that additional configuration "below the fold" of the manifest is unseen?
There was a problem hiding this comment.
I don't think this is possible since PackageDescription.Product doesn't have a public targets property that could be used to append new ones after it's been initialized. The good news is that even though the proposal doesn't guarantee it, the current editor implementation is able to recognize array concatenation like this and would still be able to add new targets to the list.
is it useful to recommend a certain manifest layout to make it as parseable as possible, even when that means that additional configuration "below the fold" of the manifest is unseen?
The current proposal test attempts to define what a "fully declarative" manifest looks like, one of the advantages being that any fully declarative manifest (+ some partially declarative ones) is guaranteed to be machine-editable. It might make sense to provide a linter to verify this at some point.
There was a problem hiding this comment.
I don't think this is possible since
PackageDescription.Productdoesn't have a publictargetsproperty that could be used to append new ones after it's been initialized.
Ah, right — each of the product types for which it's relevant has a targets property, but it's currently immutable. That might possibly be good to fix.
The good news is that even though the proposal doesn't guarantee it, the current editor implementation is able to recognize array concatenation like this and would still be able to add new targets to the list.
That's great to hear!
Given that, this seems good to me. If we can make the product type properties mutable, we can maybe make this a little cleaner in the future. Thanks for explaining!
Package.swift
Outdated
|
|
||
| // Because SwiftSyntax is closely tied to the compiler, only build it if 'SPM_BUILD_PACKAGE_SYNTAX' | ||
| // is set. Package syntax support can only be built alongside a matching SwiftSyntax | ||
| // and compiler toolchain, so only a path-based dependency is supported. |
There was a problem hiding this comment.
Since there are runtime requirements that involve embedding lib_InternalSwiftSyntaxParser.{dylib,so} into any client that uses this, would it make sense to include a reference to the instructions at https://github.com/apple/swift-syntax in this comment? (although I suppose it's easy enough to find, it might at least call out that there's a runtime requirement here in addition to the build requirement)
There was a problem hiding this comment.
Good idea. I expanded this comment a bit and added a link to the README
There was a problem hiding this comment.
Thanks for splitting this out @owenv! Just a couple of questions in the manifest.
owenv
force-pushed
the
package-editor-pt-1
branch
from
a6d1d44
to
7b319d5
Compare
|
@abertelrud thanks! I'm also cc'ing @shahmishal on this in case adding a SwiftSyntax dependency here will have any impact on the build infrastructure. Not every bot is testing SwiftSyntax today, but I think every one that builds SwiftPM using the bootstrap script is checking it out. |
We'll need to update the development docs since people developing on non-macOS will potentially use |
|
@neonichu good point. I've updated the section of Contributing.md that discusses the bootstrap script with directions on how to choose a matching version of SwiftSyntax when using a local build of swiftc or a nightly snapshot |
|
@swift-ci please smoke test |
| // independently of a full build-script invocation, only include the PackageSyntax | ||
| // target if SPM_BUILD_PACKAGE_SYNTAX is set. See the SwiftSyntax | ||
| // [README](https://github.com/apple/swift-syntax/blob/main/README.md) for more | ||
| // information about using it as a dependency. |
There was a problem hiding this comment.
So because of the conditionality here, clients that use libSwiftPM would continue building because SPM_BUILD_PACKAGE_SYNTAX isn't set by default, but the package editing functionality just wouldn't be available? Is the idea that none of the other code in libSwiftPM would import PackageSyntax, or would it use conditional imports to make sure it continues working when PackageSyntax isn't available? I suppose there's some risk that if the tests run with PackageSyntax checked out, unintentional imports might slip through that only show up when trying to use libSwiftPM without PackageSyntax (or I suppose the self-hosted tests would catch that?).
There was a problem hiding this comment.
So because of the conditionality here, clients that use libSwiftPM would continue building because SPM_BUILD_PACKAGE_SYNTAX isn't set by default, but the package editing functionality just wouldn't be available?
Yeah, it seemed like leaving it off by default (except when using the bootstrap script) was best for incremental adoption by library clients, and to ensure that somebody can still build a standalone checkout of the SwiftPM repo easily.
Is the idea that none of the other code in libSwiftPM would import PackageSyntax, or would it use conditional imports to make sure it continues working when PackageSyntax isn't available?
I think conditional imports are going to be required in the Commands and CommandsTests modules to support the CLI, but other than that I'm pretty sure we can avoid them. It's definitely a bit messy, but like you mentioned the self-hosted tests build without SPM_BUILD_PACKAGE_SYNTAX set so they should catch any accidental build breakage.
There was a problem hiding this comment.
Yes, I absolutely agree that leaving PackageSyntax out for now existing clients is the right thing. And it makes sense to use #if BUILD_PACKAGE_SYNTAX for this (as seen in #3180; I forgot about that part when leaving this comment earlier).
|
The changes look good to me but I would also want to hear from @neonichu and from @shahmishal about the earlier question. The benefit of this is clear but it's the kind of thing that can have a lot of downstream effects. |
|
Looks like the tests failed due to a swift-driver issue that's now fixed @swift-ci please smoke test |
SPM_BUILD_PACKAGE_SYNTAX must be set when executing the manifest to include the SwiftSyntax dependency and build the PackageSyntax target. This lets contributors to continue building SwiftPM standalone without needing to worry about whether they're using a matching toolchain/SwiftSyntax. SPM_BUILD_PACKAGE_SYNTAX is set by default during SwiftPM bootstrap. Right now, the PackageSyntax module is empty. The implementation will follow in separate commits.
owenv
force-pushed
the
package-editor-pt-1
branch
from
452f936
to
32b5f66
Compare
|
fixing a merge conflict in the docs @swift-ci please smoke test |
|
To follow up on this now that the proposal's been accepted, I've been thinking again about whether we should be bootstrapping the as a 'phase 2' component of SwiftPM or moving it to a second package/repo and having the commands in SwiftPM shell out to the new executable. IMO it comes down to a tradeoff between build complexity and the amount of extra infrastructure required, I'd be interested in hearing people's thoughts. |
owenv
changed the title
|
@owenv thanks for putting these together. we would need a bit of time before we can merge this, but will do so as soon as we can |
Motivation:
This PR breaks out the minimal initial set of changes to support manifest editing from #3034. The smaller set of changes here should be easier to review.
Modifications:
SPM_BUILD_PACKAGE_SYNTAX must be set when executing the manifest to include the SwiftSyntax dependency and build the PackageSyntax target. This lets contributors to continue building SwiftPM standalone without needing to worry about whether they're using a matching toolchain/SwiftSyntax.
SPM_BUILD_PACKAGE_SYNTAX is set by default during SwiftPM bootstrap.
Right now, the PackageSyntax module is empty. The implementation will follow in separate commits. I also removed the old prototype so that future PRs can replace it.