Skip to content

identity improvements #3323

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

Merged
merged 4 commits into from
Mar 10, 2021
Merged

identity improvements #3323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce91eea
identity improvements
tomerd Mar 5, 2021
c7eef31
moar
tomerd Mar 5, 2021
07abcf6
cleanup
tomerd Mar 6, 2021
a253072
fixup
tomerd Mar 8, 2021
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
6 changes: 3 additions & 3 deletions Examples/package-info/Sources/package-info/main.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ let packagePath = localFileSystem.currentWorkingDirectory!
// Each takes longer to load than the level above it, but provides more detail.
let diagnostics = DiagnosticsEngine()
let identityResolver = DefaultIdentityResolver()
let manifest = try tsc_await { ManifestLoader.loadManifest(at: packagePath, kind: .local, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], identityResolver: identityResolver, on: .global(), completion: $0) }
let loadedPackage = try tsc_await { PackageBuilder.loadPackage(at: packagePath, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], identityResolver: identityResolver, diagnostics: diagnostics, on: .global(), completion: $0) }
let graph = try Workspace.loadGraph(packagePath: packagePath, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], identityResolver: identityResolver, diagnostics: diagnostics)
let manifest = try tsc_await { ManifestLoader.loadRootManifest(at: packagePath, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], identityResolver: identityResolver, on: .global(), completion: $0) }
let loadedPackage = try tsc_await { PackageBuilder.loadRootPackage(at: packagePath, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], identityResolver: identityResolver, diagnostics: diagnostics, on: .global(), completion: $0) }
let graph = try Workspace.loadRootGraph(at: packagePath, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], identityResolver: identityResolver, diagnostics: diagnostics)

// EXAMPLES
// ========
Expand Down
7 changes: 1 addition & 6 deletions Sources/Build/BuildOperationBuildSystemDelegateHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,13 +215,8 @@ public struct BuildDescription: Codable {
self.testDiscoveryCommands = testDiscoveryCommands
self.copyCommands = copyCommands

self.builtTestProducts = try plan.buildProducts.filter{ $0.product.type == .test }.map { desc in
// FIXME(perf): Provide faster lookups.
guard let package = (plan.graph.packages.first{ $0.products.contains(desc.product) }) else {
throw InternalError("package with product \(desc.product) not found")
}
self.builtTestProducts = plan.buildProducts.filter{ $0.product.type == .test }.map { desc in
return BuiltTestProduct(
packageName: package.name,
productName: desc.product.name,
binaryPath: desc.binary
)
Expand Down
2 changes: 1 addition & 1 deletion Sources/Commands/Describe.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ fileprivate struct DescribedPackage: Encodable {
let swiftLanguagesVersions: [String]?

init(from package: Package) {
self.name = package.name
self.name = package.manifestName // TODO: rename property to manifestName?
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about that — it doesn't seem clear to end users if there is both a package name and a manifest name. Or am I misunderstanding the question?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, per the comment below, maybe this should be the identity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#3323 (comment)

self.path = package.path.pathString
self.toolsVersion = "\(package.manifest.toolsVersion.major).\(package.manifest.toolsVersion.minor)"
+ (package.manifest.toolsVersion.patch == 0 ? "" : ".\(package.manifest.toolsVersion.patch)")
Expand Down
33 changes: 21 additions & 12 deletions Sources/Commands/SwiftPackageTool.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,13 +170,16 @@ extension SwiftPackageTool {
let workspace = try swiftTool.getActiveWorkspace()
let root = try swiftTool.getWorkspaceRoot()

let manifests = try temp_await {
let rootManifests = try temp_await {
workspace.loadRootManifests(packages: root.packages, diagnostics: swiftTool.diagnostics, completion: $0)
}
guard let manifest = manifests.first else { return }
guard let rootManifest = rootManifests.first else {
throw StringError("invalid manifests at \(root.packages)")
}

let builder = PackageBuilder(
manifest: manifest,
identity: .root(name: rootManifest.name),
manifest: rootManifest,
productFilter: .everything,
path: try swiftTool.getPackageRoot(),
xcTestMinimumDeploymentTargets: MinimumDeploymentTarget.default.xcTestMinimumDeploymentTargets,
Expand Down Expand Up @@ -237,12 +240,16 @@ extension SwiftPackageTool {
// Get the root package.
let workspace = try swiftTool.getActiveWorkspace()
let root = try swiftTool.getWorkspaceRoot()
let manifest = try temp_await {
let rootManifests = try temp_await {
workspace.loadRootManifests(packages: root.packages, diagnostics: swiftTool.diagnostics, completion: $0)
}[0]
}
guard let rootManifest = rootManifests.first else {
throw StringError("invalid manifests at \(root.packages)")
}

let builder = PackageBuilder(
manifest: manifest,
identity: .root(name: rootManifest.name),
manifest: rootManifest,
productFilter: .everything,
path: try swiftTool.getPackageRoot(),
xcTestMinimumDeploymentTargets: [:], // Minimum deployment target does not matter for this operation.
Expand Down Expand Up @@ -366,15 +373,17 @@ extension SwiftPackageTool {
let workspace = try swiftTool.getActiveWorkspace()
let root = try swiftTool.getWorkspaceRoot()

let manifests = try temp_await {
let rootManifests = try temp_await {
workspace.loadRootManifests(packages: root.packages, diagnostics: swiftTool.diagnostics, completion: $0)
}
guard let manifest = manifests.first else { return }
guard let rootManifest = rootManifests.first else {
throw StringError("invalid manifests at \(root.packages)")
}

let encoder = JSONEncoder.makeWithDefaults()
encoder.userInfo[Manifest.dumpPackageKey] = true

let jsonData = try encoder.encode(manifest)
let jsonData = try encoder.encode(rootManifest)
let jsonString = String(data: jsonData, encoding: .utf8)!
print(jsonString)
}
Expand Down Expand Up @@ -582,7 +591,7 @@ extension SwiftPackageTool {
destination = output
} else {
let graph = try swiftTool.loadPackageGraph()
let packageName = graph.rootPackages[0].name
let packageName = graph.rootPackages[0].manifestName // TODO: use identity instead?
Copy link
Contributor

Choose a reason for hiding this comment

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

This depends on what we want to do with name moving forward. If identity is an opaque string, it might make sense to have a canonical display form of it. Or should this intentionally be exactly the same string as one would put in the manifest when specifying the package in the registry?

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the other changes here, it seems that we'll need to use the identity string instead of the name (which isn't necessarily wrong). That might be a good thing to move to across-the-board. I wonder if IDEs should also show that in their user interface — there has been this unfortunate ambiguity between the name of the package directory vs the name declared in the manifest.

I had been thinking of the identity as opaque but it probably is the right thing to use in diagnostics and descriptions after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried to keep the changes to behavior minimal in this PR -- a follow up PR to clean up the remain usage of manifestName and eventually remove name from the manifest are next on my list

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds great! Thanks for taking this on!

destination = packageRoot.appending(component: "\(packageName).zip")
}

Expand Down Expand Up @@ -657,10 +666,10 @@ extension SwiftPackageTool {
dstdir = outpath.parentDirectory
case let outpath?:
dstdir = outpath
projectName = graph.rootPackages[0].name
projectName = graph.rootPackages[0].manifestName // TODO: use identity instead?
case _:
dstdir = try swiftTool.getPackageRoot()
projectName = graph.rootPackages[0].name
projectName = graph.rootPackages[0].manifestName // TODO: use identity instead?
}
let xcodeprojPath = Xcodeproj.buildXcodeprojPath(outputDir: dstdir, projectName: projectName)

Expand Down
18 changes: 15 additions & 3 deletions Sources/Commands/SwiftTestTool.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,9 +224,12 @@ public struct SwiftTestTool: SwiftCommand {
case .codeCovPath:
let workspace = try swiftTool.getActiveWorkspace()
let root = try swiftTool.getWorkspaceRoot()
let rootManifest = try temp_await {
let rootManifests = try temp_await {
workspace.loadRootManifests(packages: root.packages, diagnostics: swiftTool.diagnostics, completion: $0)
}[0]
}
guard let rootManifest = rootManifests.first else {
throw StringError("invalid manifests at \(root.packages)")
}
let buildParameters = try swiftTool.buildParametersForTest()
print(codeCovAsJSONPath(buildParameters: buildParameters, packageName: rootManifest.name))

Expand Down Expand Up @@ -352,6 +355,15 @@ public struct SwiftTestTool: SwiftCommand {

/// Processes the code coverage data and emits a json.
private func processCodeCoverage(_ testProducts: [BuiltTestProduct], swiftTool: SwiftTool) throws {
let workspace = try swiftTool.getActiveWorkspace()
let root = try swiftTool.getWorkspaceRoot()
let rootManifests = try temp_await {
workspace.loadRootManifests(packages: root.packages, diagnostics: swiftTool.diagnostics, completion: $0)
}
guard let rootManifest = rootManifests.first else {
throw StringError("invalid manifests at \(root.packages)")
}

// Merge all the profraw files to produce a single profdata file.
try mergeCodeCovRawDataFiles(swiftTool: swiftTool)

Expand All @@ -360,7 +372,7 @@ public struct SwiftTestTool: SwiftCommand {
// Export the codecov data as JSON.
let jsonPath = codeCovAsJSONPath(
buildParameters: buildParameters,
packageName: product.packageName)
packageName: rootManifest.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

This would then presumably also be the identity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#3323 (comment)

try exportCodeCovAsJSON(to: jsonPath, testBinary: product.binaryPath, swiftTool: swiftTool)
}
}
Expand Down
8 changes: 4 additions & 4 deletions Sources/Commands/show-dependencies.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ private final class PlainTextDumper: DependenciesDumper {

let pkgVersion = package.manifest.version?.description ?? "unspecified"

stream <<< "\(hanger)\(package.name)<\(package.manifest.packageLocation)@\(pkgVersion)>\n"
stream <<< "\(hanger)\(package.identity.description)<\(package.manifest.packageLocation)@\(pkgVersion)>\n"

if !package.dependencies.isEmpty {
let replacement = (index == packages.count - 1) ? " " : "│ "
Expand All @@ -69,7 +69,7 @@ private final class FlatListDumper: DependenciesDumper {
func dump(dependenciesOf rootpkg: ResolvedPackage, on stream: OutputByteStream) {
func recursiveWalk(packages: [ResolvedPackage]) {
for package in packages {
stream <<< package.name <<< "\n"
stream <<< package.identity.description <<< "\n"
if !package.dependencies.isEmpty {
recursiveWalk(packages: package.dependencies)
}
Expand All @@ -88,7 +88,7 @@ private final class DotDumper: DependenciesDumper {
let url = package.manifest.packageLocation
if nodesAlreadyPrinted.contains(url) { return }
let pkgVersion = package.manifest.version?.description ?? "unspecified"
stream <<< #""\#(url)" [label="\#(package.name)\n\#(url)\n\#(pkgVersion)"]"# <<< "\n"
stream <<< #""\#(url)" [label="\#(package.identity.description)\n\#(url)\n\#(pkgVersion)"]"# <<< "\n"
nodesAlreadyPrinted.insert(url)
}

Expand Down Expand Up @@ -130,7 +130,7 @@ private final class JSONDumper: DependenciesDumper {
func dump(dependenciesOf rootpkg: ResolvedPackage, on stream: OutputByteStream) {
func convert(_ package: ResolvedPackage) -> JSON {
return .orderedDictionary([
"name": .string(package.name),
"name": .string(package.manifestName), // TODO: remove? add identity?
"url": .string(package.manifest.packageLocation),
"version": .string(package.manifest.version?.description ?? "unspecified"),
"path": .string(package.path.pathString),
Expand Down
10 changes: 7 additions & 3 deletions Sources/PackageGraph/GraphLoadingNode.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,17 @@ import TSCUtility
/// - SeeAlso: DependencyResolutionNode
public struct GraphLoadingNode: Equatable, Hashable {

/// The package identity.
public let identity: PackageIdentity

/// The package manifest.
public let manifest: Manifest

/// The product filter applied to the package.
public let productFilter: ProductFilter

public init(manifest: Manifest, productFilter: ProductFilter) {
public init(identity: PackageIdentity, manifest: Manifest, productFilter: ProductFilter) {
self.identity = identity
self.manifest = manifest
self.productFilter = productFilter
}
Expand All @@ -41,9 +45,9 @@ extension GraphLoadingNode: CustomStringConvertible {
public var description: String {
switch productFilter {
case .everything:
return manifest.name
return self.manifest.name
case .specific(let set):
return "\(manifest.name)[\(set.sorted().joined(separator: ", "))]"
return "\(self.manifest.name)[\(set.sorted().joined(separator: ", "))]"
}
}
}
15 changes: 8 additions & 7 deletions Sources/PackageGraph/LocalPackageContainer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,22 +39,23 @@ public final class LocalPackageContainer: PackageContainer {
private func loadManifest() throws -> Manifest {
try manifest.memoize() {
// Load the tools version.
let toolsVersion = try toolsVersionLoader.load(at: AbsolutePath(package.location), fileSystem: fileSystem)
let toolsVersion = try self.toolsVersionLoader.load(at: AbsolutePath(self.package.location), fileSystem: self.fileSystem)

// Validate the tools version.
try toolsVersion.validateToolsVersion(self.currentToolsVersion, packagePath: package.location)
try toolsVersion.validateToolsVersion(self.currentToolsVersion, packagePath: self.package.location)

// Load the manifest.
// FIXME: this should not block
return try temp_await {
manifestLoader.load(at: AbsolutePath(package.location),
packageKind: package.kind,
packageLocation: package.location,
manifestLoader.load(at: AbsolutePath(self.package.location),
packageIdentity: self.package.identity,
packageKind: self.package.kind,
packageLocation: self.package.location,
version: nil,
revision: nil,
toolsVersion: toolsVersion,
identityResolver: identityResolver,
fileSystem: fileSystem,
identityResolver: self.identityResolver,
fileSystem: self.fileSystem,
diagnostics: nil,
on: .global(),
completion: $0)
Expand Down
Loading