Manifest source generation should encode platform versions using their symbolic forms when possible #3707
Conversation
…ing their symbolic forms when possible, instead of using the freeform strings. This fixes that and adds a unit test. rdar://82467001
abertelrud
requested review from
friedbunny,
neonichu and
tomerd
as code owners
|
@swift-ci please smoke test |
| default: | ||
| self.init(enum: platform.platformName, string: platform.version) | ||
| // Provides knowledge about what the current version of PackageDescription provides. | ||
| struct PlatformManifestRepresentation { |
There was a problem hiding this comment.
is this information not also found anywhere else? does it make sense to make it more generally available?
There was a problem hiding this comment.
Unfortunately it isn't. There is no sharing of state today between what is defined in PackageDescription and what libSwiftPM knows to be defined in PackageDescription.
There are a lot of cases where this would be useful, in particular to share the Codable struct definitions for the stuff that is gets as JSON from the PackageDescription library to SwiftPM proper. Perhaps the best way would be to be able to share source files between PackageDescription and PackageModel, and have the same types be available in both. In that case there might be some kind of a runtime CaseIterable thing that we could do here.
There's no doubt that this is a really unfortunate thing to have to have here. I hope that if we have good mechanical editing we might perhaps be able to sunset the source generation in the future, though that's not imminent.
There was a problem hiding this comment.
But to your point, I think perhaps we can figure out something involving a custom source path that reaches into PackageDescription and includes one of its source files where the types are defined. That would be generally useful and maybe this is a good time to spend that effort rather than add more data like this.
There was a problem hiding this comment.
side note: today we don't allow overlapping source files between targets (but IIRC it's a restriction we have previously talked about lifting), so we would likely need to use symlinks for now
There was a problem hiding this comment.
I see — I thought sources allowed it and it was just targets directories that couldn't intersect (the path parameter). There seem to be some rather arbitrary rules for what can and cannot be in a target, and I think SwiftPM would do well to relax them. That's obvious a separate discussion.
I'll see what I can do with a symlink here.
| var knownVersions: Set<String> | ||
| } | ||
| let platformManifestReps = [ | ||
| "macos": PlatformManifestRepresentation(manifestName: "macOS", knownVersions: ["10.10", "10.11", "10.12", "10.13", "10.14", "10.15", "11", "12"]), |
There was a problem hiding this comment.
nit: can use .init( instead of long form PlatformManifestRepresentation( if the array type is defined
There was a problem hiding this comment.
I actually had it that way to start with but I dislike having to add a type to the collection more than I do spelling out the type. But I can change it, I don't feel superstrongly.
|
Still need to make time to see if we can avoid repeating the set of enums that PackageDescription declares, by sharing a source file between the two targets using a symlink. Lower priority than some other PRs. |
Manifest source generation had a FIXME to encode their symbolic forms when possible, instead of using the freeform strings. This fixes that and adds a unit test.
Motivation:
This was always something left over to fix. A client of libSwiftPM wants to make use of this to avoid spurious changes from input manifest.
rdar://82467001