Skip to content

Manifest source generation should encode platform versions using their symbolic forms when possible #3707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Manifest source generation should encode platform versions using their symbolic forms when possible #3707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 26 additions & 16 deletions Sources/PackageModel/ManifestSourceGeneration.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,22 +100,32 @@ fileprivate extension SourceCodeFragment {

/// Instantiates a SourceCodeFragment to represent a single platform.
init(from platform: PlatformDescription) {
// NOTE: This could be cleaned up to use the nicer version accessors.
switch platform.platformName {
case "macos":
self.init(enum: "macOS", string: platform.version)
case "maccatalyst":
self.init(enum: "macCatalyst", string: platform.version)
case "ios":
self.init(enum: "iOS", string: platform.version)
case "tvos":
self.init(enum: "tvOS", string: platform.version)
case "watchos":
self.init(enum: "watchOS", string: platform.version)
case "driverkit":
self.init(enum: "DriverKit", string: platform.version)
default:
self.init(enum: platform.platformName, string: platform.version)
// Provides knowledge about what the current version of PackageDescription provides.
struct PlatformManifestRepresentation {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this information not also found anywhere else? does it make sense to make it more generally available?

Copy link
Contributor Author

@abertelrud abertelrud Sep 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 manifestName: String
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"]),
Copy link
Contributor

@tomerd tomerd Sep 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can use .init( instead of long form PlatformManifestRepresentation( if the array type is defined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"maccatalyst": PlatformManifestRepresentation(manifestName: "macCatalyst", knownVersions: ["13", "14", "15"]),
"ios": PlatformManifestRepresentation(manifestName: "iOS", knownVersions: ["8", "9", "10", "11", "12", "13", "14", "15"]),
"tvos": PlatformManifestRepresentation(manifestName: "tvOS", knownVersions: ["9", "10", "11", "12", "13", "14", "15"]),
"watchos": PlatformManifestRepresentation(manifestName: "watchOS", knownVersions: ["2", "3", "4", "5", "6", "7", "8"]),
"driverkit": PlatformManifestRepresentation(manifestName: "driverKit", knownVersions: ["19", "20", "21"]),
]

// See if we have a versioned platform manifest representation for this platform.
if let platformManifestRep = platformManifestReps[platform.platformName] {
let simplifiedVersion = platform.version.spm_dropSuffix(".0")
let versionSubnode = platformManifestRep.knownVersions.contains(simplifiedVersion)
? SourceCodeFragment(enum: "v" + simplifiedVersion.replacingOccurrences(of: ".", with: "_"))
: SourceCodeFragment(string: platform.version)
self.init(enum: platformManifestRep.manifestName, subnodes: [versionSubnode])
}
else {
// We don't have a versioned platform, so emit the platform without version numbers. But in debug builds we assert.
assert(false, "unhandled plaform \(platform.platformName) in manifest source generation")
self.init(enum: platform.platformName)
}
}

Expand Down
28 changes: 26 additions & 2 deletions Tests/WorkspaceTests/ManifestSourceGenerationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ import Workspace

class ManifestSourceGenerationTests: XCTestCase {

/// Private function that writes the contents of a package manifest to a temporary package directory and then loads it, then serializes the loaded manifest back out again and loads it once again, after which it compares that no information was lost.
private func testManifestWritingRoundTrip(manifestContents: String, toolsVersion: ToolsVersion, fs: FileSystem = localFileSystem) throws {
/// Private function that writes the contents of a package manifest to a temporary package directory and then loads it, then serializes the loaded manifest back out again and loads it once again, after which it compares that no information was lost. Returns the newly generated manifest.
@discardableResult
private func testManifestWritingRoundTrip(manifestContents: String, toolsVersion: ToolsVersion, fs: FileSystem = localFileSystem) throws -> String {
try withTemporaryDirectory { packageDir in
// Write the original manifest file contents, and load it.
try fs.writeFileContents(packageDir.appending(component: Manifest.filename), bytes: ByteString(encodingAsUTF8: manifestContents))
Expand Down Expand Up @@ -74,6 +75,9 @@ class ManifestSourceGenerationTests: XCTestCase {
XCTAssertEqual(newManifest.swiftLanguageVersions, manifest.swiftLanguageVersions, failureDetails)
XCTAssertEqual(newManifest.cLanguageStandard, manifest.cLanguageStandard, failureDetails)
XCTAssertEqual(newManifest.cxxLanguageStandard, manifest.cxxLanguageStandard, failureDetails)

// Return the new contents for additional checking.
return newContents
}
}

Expand Down Expand Up @@ -284,4 +288,24 @@ class ManifestSourceGenerationTests: XCTestCase {
"""
try testManifestWritingRoundTrip(manifestContents: manifestContents, toolsVersion: .v5_6)
}

func testPlatformVersions() throws {
let manifestContents = """
// swift-tools-version:5.5
import PackageDescription

let package = Package(
name: "Plugins",
platforms: [
.macOS(.v10_14),
.iOS(.v13),
.driverKit("42")
]
)
"""
let writtenContents = try testManifestWritingRoundTrip(manifestContents: manifestContents, toolsVersion: .v5_6)
XCTAssert(writtenContents.contains(".macOS(.v10_14)"), "didn't find expected contents in \(writtenContents)")
XCTAssert(writtenContents.contains(".iOS(.v13)"), "didn't find expected contents in \(writtenContents)")
XCTAssert(writtenContents.contains(".driverKit(\"42\")"), "didn't find expected contents in \(writtenContents)")
}
}