identity improvements #3323

tomerd · 2021-03-05T07:33:43Z

motivation: improve correctness by using identity more broadly throughout the codebase

changes:

expose identity on Package, ResolvedPackage and GraphLoadingNode
reduce calls to identityResolver.resolveIdentity since we now have identity more broadly available (also improves performance)
ManifestLoader now take PackageIdentity to reduce calls to identityResolver.resolveIdentity when the identity is already known
introduce PackageIdentity.root as a utility for root packages, since their identity is straight-forward
adjust call-sites
adjust tests

tomerd · 2021-03-06T04:46:58Z

@swift-ci please smoke test

tomerd · 2021-03-06T04:58:58Z

@swift-ci please smoke test

abertelrud

This looks good to me! I think we'll have a lot of cases where we'll need to present the identity to the user, which means we'll probably want to get rid of the name in the manifest to reduce ambiguity. Will it be readable enough to use in schemes etc though?

abertelrud · 2021-03-07T05:38:17Z

Sources/Commands/Describe.swift

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


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?

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

#3323 (comment)

abertelrud · 2021-03-07T06:52:34Z

Sources/Commands/SwiftPackageTool.swift

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


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?

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.

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

Sounds great! Thanks for taking this on!

abertelrud · 2021-03-07T06:53:44Z

Sources/Commands/SwiftTestTool.swift

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


This would then presumably also be the identity?

#3323 (comment)

abertelrud · 2021-03-07T07:15:09Z

Sources/PackageGraph/PackageGraph+Loading.swift

+        let rootManifestsMap = root.packages.mapValues { $0.manifest }
+        let externalManifestsMap = externalManifests.map{ (identityResolver.resolveIdentity(for: $0.packageLocation), $0) }
+        let manifestMap = rootManifestsMap.merging(externalManifestsMap, uniquingKeysWith: { lhs, rhs in
+            return rhs // 👀 this was not possible before (the dictionary creation would crash), is preferring external correct?


It seems to me that preferring roots over externals would be preferable, since that would preserve the current overriding semantics.

changed to lhs

abertelrud · 2021-03-07T07:20:09Z

Sources/PackageGraph/PackageGraphRoot.swift

-        self.manifests = manifests
+    public init(input: PackageGraphRootInput, manifests: [Manifest], explicitProduct: String? = nil) throws {
+        self.packages = try input.packages.reduce(into: .init(), { partial, inputPath in
+            let manifestPath = inputPath.basename == Manifest.filename ? inputPath : inputPath.appending(component: Manifest.filename)


Does this need to take into account versioned package manifests?

this is mostly in place for testing -- some test pass the full manifest path and some just the directory. I would like to remove this crutch and clean up the tests

abertelrud · 2021-03-07T07:20:52Z

Sources/PackageGraph/ResolvedPackage.swift

@@ -17,19 +17,31 @@ public final class ResolvedPackage: ObjectIdentifierProtocol {
    /// The underlying package reference.
    public let underlyingPackage: Package

+    // The identity of the package.
+    public var identity: PackageIdentity {


Yay! This is going to help higher-level code a lot!

abertelrud · 2021-03-07T07:21:31Z

Sources/PackageGraph/ResolvedPackage.swift

+
+    /// The name of the package as entered in the manifest.
+    /// This should rarely be used beyond presentation purposes
+    public var manifestName: String {


Is this something we'll be able to move toward getting rid of once we have a strong notion of identity?

yes! #3323 (comment)

abertelrud · 2021-03-07T07:23:26Z

Sources/PackageLoading/PackageBuilder.swift

-    private func diagnosticLocation() -> DiagnosticLocation {
-        return PackageLocation.Local(name: manifest.name, packagePath: packagePath)
+    private var diagnosticLocation: DiagnosticLocation {
+        return PackageLocation.Local(name: self.manifest.name, packagePath: self.packagePath)


Eventually we probably want the identity in the diagnostic, but that can be a future change.

abertelrud · 2021-03-07T07:25:15Z

Sources/SPMBuildCore/PluginInvocation.swift

@@ -61,7 +61,7 @@ extension PackageGraph {
            var evalResults: [PluginInvocationResult] = []
            for pluginTarget in pluginTargets {
                // Give each invocation of an extension a separate output directory.
-                let extOutputDir = outputDir.appending(components: package.name, target.name, pluginTarget.name)
+                let extOutputDir = outputDir.appending(components: package.manifestName, target.name, pluginTarget.name) // TODO: use identity instead?


Yes, I think this should be identity.

abertelrud · 2021-03-07T07:28:15Z

Sources/Xcodeproj/SchemesGenerator.swift

@@ -86,7 +86,7 @@ public final class SchemesGenerator {
            }
        })
        schemes.append(Scheme(
-            name: rootPackage.name + "-Package",
+            name: rootPackage.manifestName + "-Package", // TODO: use identity instead?


Yes, probably everywhere name is used we should start using identity.

#3323 (comment)

tomerd · 2021-03-07T18:38:07Z

I think we'll have a lot of cases where we'll need to present the identity to the user, which means we'll probably want to get rid of the name in the manifest to reduce ambiguity. Will it be readable enough to use in schemes etc though?

This is a good point @abertelrud, in this PR tried to keep things mostly the same on that regards with a few exceptions to diagnostics about dependencies.

in general, i think there are two main cases to consider in this context given the current state of things:

the display name of the dependencies, where imo using the dependency's identity makes more sense than using it's manifest name, given that the user is exposed to the former and not to the latter.
the display name of the "root" package (ie the one the user is working on) in which the manifest name makes more sense (if/while we keep that manifest attribute) given that the package identity is fairly arbitrary (directory name).

i do think we should seriously consider removing the name attribute from the manifest, but we will need to come up with something better for root package identity than the directory name. there is also the concept of multiple root packages we need to consider in this context which we may want to address differently than how its done today

tomerd · 2021-03-08T23:57:07Z

@swift-ci please smoke test

motivation: improve correctness by using identity more broadly throughout the codebase changes: * expose identity on Package, ResolvedPackage and GraphLoadingNode * reduce calls to identityResolver.resolveIdentity since we now have identity more broadly available (also improves performance) * ManifestLoader now take PackageIdentity to reduce calls to identityResolver.resolveIdentity when the identity is already known * introduce PackageIdentity.root as a utility for root packages, since their identity is straight-forward * adjust call-sites * adjust tests

tomerd · 2021-03-09T01:40:42Z

@swift-ci please smoke test

tomerd · 2021-03-09T02:27:54Z

@swift-ci please smoke test

tomerd · 2021-03-09T20:18:53Z

@swift-ci please smoke test

tomerd · 2021-03-09T23:08:21Z

@swift-ci please smoke test linux

This reverts commit 953f368.

This reverts commit ec24ca3.

tomerd requested review from abertelrud, friedbunny and neonichu as code owners March 5, 2021 07:33

tomerd marked this pull request as draft March 5, 2021 07:33

tomerd force-pushed the refactor/identity-04 branch 3 times, most recently from 6f8717c to fe396d1 Compare March 6, 2021 04:46

tomerd self-assigned this Mar 6, 2021

tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Mar 6, 2021

tomerd marked this pull request as ready for review March 6, 2021 04:47

abertelrud reviewed Mar 7, 2021

View reviewed changes

tomerd force-pushed the refactor/identity-04 branch from ebdd69a to 89e801c Compare March 8, 2021 23:56

tomerd added 4 commits March 8, 2021 17:40

moar

c7eef31

cleanup

07abcf6

fixup

a253072

tomerd force-pushed the refactor/identity-04 branch from 89e801c to a253072 Compare March 9, 2021 01:40

tomerd merged commit 953f368 into swiftlang:main Mar 10, 2021

tomerd added a commit that referenced this pull request Mar 10, 2021

Revert "identity improvements (#3323)"

11f045c

This reverts commit 953f368.

tomerd mentioned this pull request Mar 10, 2021

Revert "identity improvements" #3342

Merged

abertelrud pushed a commit that referenced this pull request Mar 10, 2021

Revert "identity improvements (#3323)" (#3342)

ec24ca3

This reverts commit 953f368.

tomerd added a commit that referenced this pull request Mar 10, 2021

Revert "Revert "identity improvements (#3323)" (#3342)"

73d3fe9

This reverts commit ec24ca3.

tomerd mentioned this pull request Mar 10, 2021

bring back identity improvements #3344

Merged

tomerd added a commit that referenced this pull request Mar 11, 2021

Revert "Revert "identity improvements (#3323)" (#3342)"

61fd2f6

This reverts commit ec24ca3.

identity improvements #3323

identity improvements #3323

Uh oh!

Conversation

tomerd commented Mar 5, 2021

Uh oh!

tomerd commented Mar 6, 2021

Uh oh!

tomerd commented Mar 6, 2021

Uh oh!

abertelrud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd commented Mar 7, 2021

Uh oh!

tomerd commented Mar 8, 2021

Uh oh!

tomerd commented Mar 9, 2021

Uh oh!

tomerd commented Mar 9, 2021

Uh oh!

tomerd commented Mar 9, 2021

Uh oh!

tomerd commented Mar 9, 2021

Uh oh!

Uh oh!

tomerd Mar 8, 2021 •

edited

Loading