Fix Command Resolution #4038

SDGGiesbrecht · 2022-01-21T03:28:46Z

See #4028 for the impetus, particularly starting from this comment.

This implements the option labeled “3” in that thread, attempting to exactly match the API and usage described in SE‐0332. It does so by implicitly inserting every command plugin found in a first‐level dependency into the dependency graph.

SDGGiesbrecht

I left some inline comments.

SDGGiesbrecht · 2022-01-21T03:32:37Z

Sources/PackageGraph/PackageModel+Extensions.swift

                }
+                if includeCommands {
+                    nodes.append(.implicitCommands(package: self.package))


This is where the new resolution node is first inserted.

SDGGiesbrecht · 2022-01-21T03:36:22Z

Sources/PackageGraph/DependencyResolutionNode.swift

+    /// This type of node is equivalent to a product node in all respects,
+    /// except that the number and names of the products are unkown,
+    /// because the manifest contains no explicit reference to them.
+    case implicitCommands(package: PackageReference)


It is too bad we need a whole new kind of node, but since we cannot know the product names before the dependency is fetched, we have no other choice.

SDGGiesbrecht · 2022-01-21T03:40:07Z

Sources/PackageLoading/PackageBuilder.swift

+        case .specific(let set, let includeCommands):
+            filteredProducts = self.manifest.products.filter { product in
+                return set.contains(product.name)
+                || (includeCommands && self.manifest.productIsCommandPlugin(product))


This is where the products are loaded. We now load all command plugins no matter what, bypassing the filter that culls unreferenced products (when at the relevant level in the graph).

SDGGiesbrecht · 2022-01-21T03:41:56Z

Sources/PackageModel/Manifest.swift

+            case .specific(let productFilter, let includeCommands):
+                let products = self.products.filter { product in
+                    return productFilter.contains(product.name)
+                    || (includeCommands && productIsCommandPlugin(product))


Here we bypass the filter again, keeping around any dependencies of an implicit command plugin.

SDGGiesbrecht · 2022-01-21T03:43:06Z

Sources/PackageModel/Manifest.swift

-            let dependencies = self.dependenciesRequired(for: targets, keepUnused: productFilter == .everything)
+            let dependencies = self.dependenciesRequired(
+                for: targets,
+                keepImplicitCommandPlugins: productFilter == .everything,


The manifest is a root if productFilter == .everything. Therefore its direct dependencies are the ones that can have implicit commands. If the product filter is anything else, then the manifest is already partway into the graph, and its dependencies are transitive.

SDGGiesbrecht · 2022-01-21T03:59:03Z

Sources/PackageModel/Product.swift

-        case .specific(let set):
-            return "[\(set.sorted().joined(separator: ", "))]"
+        case .specific(let set, let includeCommands):
+            return "[\(set.sorted().joined(separator: ", "))\(includeCommands ? " (including commands)" : "")]"


This made test failures easier to read while debugging. It did not clash with any fixtures, so I left it.

SDGGiesbrecht · 2022-01-21T04:03:17Z

Sources/PackageModel/Product.swift

@@ -241,26 +246,3 @@ extension ProductType: Codable {
        }
    }
 }
-
-extension ProductFilter: Codable {


The old Codable implementation here could not be added to in a backwards compatible way. I just removed it to let compiler synthesize it.

I do not know where or if this is actually used. We stopped recording filters in the pins file a while ago. Changing it did not flag on any tests. Do you think there is anywhere we need to bump a format version?

I don't think this is stored anyway, though @neonichu or @tomerd would know better.

SDGGiesbrecht · 2022-01-21T04:07:17Z

Tests/PackageGraphPerformanceTests/DependencyResolverPerfTests.swift

@@ -174,7 +174,7 @@ extension ProductFilter: JSONSerializable, JSONMappable {
        switch self {
        case .everything:
            return "all".toJSON()
-        case .specific(let products):
+        case .specific(let products, _):


Oops. I forgot to come back to this. I will look at the performance tests tomorrow.

These performance tests are inoperable on main anyway, because the JSON files are out of date.

SDGGiesbrecht · 2022-01-21T04:08:10Z

Tests/WorkspaceTests/SourceControlPackageContainerTests.swift

@@ -510,6 +510,18 @@ class SourceControlPackageContainerTests: XCTestCase {
            )
        }

+        let v5_2ProductMappingWithoutCommands: [String: ProductFilter] = [


The differing context means the includeCommands need to be opposite what they were above, so they can no longer share the same variable.

SDGGiesbrecht · 2022-01-21T04:08:59Z

Tests/WorkspaceTests/WorkspaceTests.swift

-            // Ensure that the order of the manifests is stable.
+            // Ensure that the order of the manifests is stable from one run to the next (changes accross versions do not matter).
+            #if ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION
+            XCTAssertEqual(manifests.allDependencyManifests().map { $0.value.manifest.displayName }, ["Bam", "Baz", "Bar", "Foo"])


The new nodes altered the traversal order.

SDGGiesbrecht · 2022-01-21T04:50:32Z

Tests/PackageGraphPerformanceTests/DependencyResolverPerfTests.swift

@@ -174,14 +174,17 @@ extension ProductFilter: JSONSerializable, JSONMappable {
        switch self {
        case .everything:
            return "all".toJSON()
-        case .specific(let products):
+        case .specific(var products, let includeCommands):


I patched this up to do something sane. It does not match the actual files any more than main does, but will at least be operable if someone makes a new file.

abertelrud · 2022-01-21T04:51:48Z

Thank you very much @SDGGiesbrecht! Including for the detailed comments. I will start looking through it tonight, but might not wrap up until tomorrow. I really appreciate the fixes here.

abertelrud

This looks good to me (at least to the degree I'm familiar with the package resolution). Thanks for taking on these fixes — hopefully when target-based dependencies are enabled by default again, the unit tests will prevent any further regressions.

abertelrud · 2022-01-21T05:13:57Z

Sources/PackageGraph/DependencyResolutionNode.swift

@@ -58,23 +58,38 @@ public enum DependencyResolutionNode {
    ///   to the information needed in order to provide actionable suggestions to help the user stitch up the dependency declarations properly.
    case root(package: PackageReference)

+    // FIXME: If command plugins become explicit in the manifest, or are extricated from the main graph, `implicitCommands` should be removed.


abertelrud · 2022-01-21T05:17:42Z

Sources/PackageModel/Product.swift

@@ -131,23 +137,22 @@ public enum ProductFilter: Equatable, Hashable {
        self = self.union(other)
    }

-    public func contains(_ product: String) -> Bool {
+    public func contains(_ product: String, isCommandPlugin: (String) -> Bool) -> Bool {


That makes sense. Because SwiftPM doesn't yet have a versioned API I suspect that any clients could adapt, but passing in the closure makes sense too.

abertelrud · 2022-01-21T05:19:11Z

Sources/PackageModel/Product.swift

@@ -241,26 +246,3 @@ extension ProductType: Codable {
        }
    }
 }
-
-extension ProductFilter: Codable {


I don't think this is stored anyway, though @neonichu or @tomerd would know better.

abertelrud · 2022-01-21T05:20:25Z

Tests/PackageGraphPerformanceTests/DependencyResolverPerfTests.swift

@@ -174,14 +174,17 @@ extension ProductFilter: JSONSerializable, JSONMappable {
        switch self {
        case .everything:
            return "all".toJSON()
-        case .specific(let products):
+        case .specific(var products, let includeCommands):
+            if includeCommands { products.insert("_commands_") }


I know it's a nitpick, but perhaps call this "command plugins" since command is a somewhat overloaded term?

Which are you referring to? The label of the enumeration’s associated value, or the JSON placeholder string?

Both actually, but the name of the placeholder is one that might show up in debug output (I assume?), so that's the more important one, I'd think.

@swift-ci please smoke test

I did find‐and‐replace from “includeCommands” to “includeCommandPlugins” and from “_commands_” to “_command_plugins_”.

Thank you! Might save someone some confusion one day...

abertelrud · 2022-01-22T02:21:17Z

@swift-ci please smoke test

tomerd · 2022-01-25T21:08:53Z

thanks @SDGGiesbrecht, need to review this in more details given the added complexity - which may be necessary, but still somewhat unfortunate

SDGGiesbrecht · 2022-02-09T06:11:01Z

Something has changed in main since I submitted this, such that testTransitiveDependencySwitchWithSameIdentity() now crashes.

The following is able to “fix” it, but seems to indicate an underlying problem somewhere else:

// Tests/WorkspaceTests/WorkspaceTests.swift
     func testTransitiveDependencySwitchWithSameIdentity() throws {
     // ...
+        immortal = workspace._workspace?.repositoryManager.lookupSemaphore
     }
+    var immortal: Any?

// Sources/SPMTestSupport/MockWorkspace.swift
-    private var _workspace: Workspace?
+    public var _workspace: Workspace?

// Sources/Workspace/Workspace.swift
-    fileprivate var repositoryManager: RepositoryManager
+    public var repositoryManager: RepositoryManager

// Sources/SourceControl/RepositoryManager.swift
-    private let lookupSemaphore: DispatchSemaphore
+    public let lookupSemaphore: DispatchSemaphore

SDGGiesbrecht · 2022-02-11T22:39:22Z

I moved the workaround from my last comment into the umbrella PR (#3838) in order to unblock work on its branch. The issue does not seem to be directly related to anything here.

This pull request does not require that other fix. Under legacy resolution this PR makes no difference and all remains green, and under target‐based resolution, It moves the current state from several tests broken to one test broken, which is a net improvement.

SDGGiesbrecht · 2022-02-19T02:44:01Z

The above bug simply vanished. Some recent change to main must have fixed it.

abertelrud · 2022-03-07T19:22:43Z

@swift-ci please smoke test

SDGGiesbrecht · 2022-05-19T23:41:57Z

I was just pinged on the forums for an update on this. Are you able to clarify when “the next merge window” is?

tomerd · 2022-05-20T03:38:15Z

thank you for your patience with this @SDGGiesbrecht, our hope to take the changes marked as "next" in the few weeks

SDGGiesbrecht · 2022-06-21T21:22:54Z

@tomerd. A friendly ping one month later.

SDGGiesbrecht · 2022-07-06T22:50:33Z

Pinging because another few weeks have passed. @tomerd, @abertelrud, @neonichu

samdeane · 2022-07-22T10:40:26Z

Is there a reason why this is stalled? For what it's worth, the status of this change came up in this discussion on the forums (particular from here onwards).

SDGGiesbrecht · 2022-08-03T22:55:55Z

Pinging again. A total of six months have now elapsed. @tomerd, @abertelrud, @neonichu

tomerd · 2022-08-04T21:32:52Z

thanks for your patience @SDGGiesbrecht, aware this is taking very long. we are discussing scope for the next release and this is something we are looking into

SDGGiesbrecht · 2022-09-22T20:49:31Z

Work on SwiftPM seems to be picking up again after the 5.7 release. What came of your discussions? @tomerd

malcommac · 2022-10-25T06:26:09Z

Is there any schedule for this MR? Thanks

SDGGiesbrecht · 2023-03-06T21:33:55Z

Closing. This and related PRs are stale and I do not have time to maintain them anymore. I will leave the related branches around for now in case anyone else wants to pick them up.

Implemented command resolution.

749b907

SDGGiesbrecht requested review from abertelrud, elsh, neonichu and tomerd as code owners January 21, 2022 03:28

SDGGiesbrecht commented Jan 21, 2022

View reviewed changes

SDGGiesbrecht mentioned this pull request Jan 21, 2022

Update Target Based Resolution #3838

Closed

11 tasks

Enabled testing JSON.

b324d8e

SDGGiesbrecht commented Jan 21, 2022

View reviewed changes

abertelrud approved these changes Jan 21, 2022

View reviewed changes

Found and replaced “commands” with “command plugins”.

4ddc5ff

tomerd self-assigned this Jan 25, 2022

Resolved merge conflicts.

e52030c

neonichu added the next waiting for next merge window label Apr 11, 2022

heckj mentioned this pull request Jan 28, 2022

[SR-15783] With swift package plugins, the diagnostic about all dependencies needing to be matched to targets is accurate but potentially harmful #4354

Closed

SDGGiesbrecht mentioned this pull request May 18, 2022

Fix Command Resolution (CI) SDGGiesbrecht/swift-package-manager#2

Closed

SDGGiesbrecht closed this Mar 6, 2023

Fix Command Resolution #4038

Fix Command Resolution #4038

Uh oh!

Conversation

SDGGiesbrecht commented Jan 21, 2022

Uh oh!

SDGGiesbrecht 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SDGGiesbrecht Jan 21, 2022 • 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!

abertelrud commented Jan 21, 2022

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!

abertelrud Jan 22, 2022 • 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!

SDGGiesbrecht Jan 22, 2022 • 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!

abertelrud commented Jan 22, 2022

Uh oh!

tomerd commented Jan 25, 2022

Uh oh!

SDGGiesbrecht commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SDGGiesbrecht commented Feb 11, 2022

Uh oh!

SDGGiesbrecht commented Feb 19, 2022

Uh oh!

abertelrud commented Mar 7, 2022

Uh oh!

SDGGiesbrecht commented May 19, 2022

Uh oh!

tomerd commented May 20, 2022

SDGGiesbrecht Jan 21, 2022 •

edited

Loading

abertelrud Jan 22, 2022 •

edited

Loading

SDGGiesbrecht Jan 22, 2022 •

edited

Loading

SDGGiesbrecht commented Feb 9, 2022 •

edited

Loading

samdeane commented Jul 22, 2022 •

edited

Loading