Finish SE‐0226 (Ignore Unused Products)

[SDGGiesbrecht]

This pull request should complete the implementation of SE‐0226: Package Manager Target Based Dependency Resolution. @hartbit already taught the resolver to only care about products in #2424. This takes that work further so that the resolver only cares about products that are actually used.

This pull request is sizeable, so I’ll elaborate the main points here.

The changes broadly fall into four groups:

1. The manifest is now able to compute the downward‐facing products required for any upward‐facing product set. This is mostly housed in PackageModel/Manifest.swift, and boils down to turning @hartbit’s allRequiredDependencies into dependenciesRequired(for:), along with the input type ProductFilter.

2. The resolver now operates on abstract nodes. See DependencyResolutionNode for the nitty‐gritty details. Essentially, it resolves individual products instead of whole packages. The new node type has simply been slotted in to replace the PackageReferences that were in use before. None of @kiliankoe’s Pubgrub decision making processes has been changed in any way. The diagnostics have also been left alone except for the inclusion of the specific product alongside the package name where relevant.

3. Dozens of new method parameters and instance properties have been added to various types and methods in order to pass the new information along from where it originates to where it is needed. Unfortunately, this affects a whole swath of things across the PackageModel, PackageLoading, PackageGraph, Workspace, Commands and SPMTestSupport modules, but all of these changes are straightforward.

4. Many tests required fixing. Most of them merely needed to provide the new parameters required in order to satisfy the new API. Several were expecting failures that were no longer reachable, and needed dependencies to be made explicit so that the resolver would even enter the code branch in question. All but three are straightforward. (See below.)

---

While I was working through the tests, three things came up which need a closer look. They are tests that expected behaviour at least partially at odds with SE‐0226.

1. The test suite expected unreachable executable products to be available. swift run dependency‐executable is apparently supposed to work, even if no dependency is declared on that product. I can see how that could be useful, and it wouldn’t surprise me if users are taking advantage of it in their workflows. But SE‐0226 says this:

   Another example of packages requiring additional dependencies is for sample code targets. A library package may want to create an executable target which demonstrates some functionality of the library. This executable may require other dependencies such as a command-line argument parser.

   So including such executables would undermine one of the motivating use cases for SE‐0226. As such, what I have implemented right now does not resolve such executables by default.

   However, as a compromise to maintain backwards compatibility, I have channeled the argument provided to any swift run invocation through to the resolver, so that if you explicitly ask to run an executable of an immediate dependency, you will still be able to. Since it has some minor drawbacks, I consider this an interim solution. This implementation will suffice for now, but a separate evolution proposal should probably be made to design something better if we want to continue supporting this long‐term. See the Fix‐Me in the initializer for PackageGraphRoot for more details.

2. The test suite expected unreachable targets to be available to a swift build --target dependency‐target invocation. Since there is no infrastructure in the resolver to handle targets, there is no easy way to locate or load such a target. Since it seems backwards to SE‐0226 and has no practical use I can think of, I simply removed the test.

3. The test suite expected legacy system packages to work without an explicit target dependency declaration. Such packages have been triggering a deprecation warning since Swift 4.2. Continuing to support them implicitly is problematic, because there is no way to know if that’s what a package is until after fetching and loading it, so it would defeat the whole of SE‐0226.

   As a compromise to maintain as much backwards compatibility as possible, I have taught the manifest loader to internally convert such a package into a modern‐style one with a system target and product, which inherit the package’s name and details. This allows the current toolchain to still use such dependencies by referencing the synthesized product. However, if the client package wants to support both old and new toolchains at once (such as 5.1 and 5.4), it will have to make use of #if compiler, because the old toolchain will be unable to locate the declared product, whereas the new toolchain will not resolve an undeclared product.

[aciidgh]

😱😱😱

[rballard]

This is a wonderful contribution, Jeremy!

[SDGGiesbrecht]

The test suite expected legacy system packages to work without an explicit target dependency declaration.

Actually, I just realized this was completely broken already in Swift 5.2. The 5.2 release included the first half of the SE‐0226 work, and that meant that such an implicit dependency was being skipped during resolution. With no product to point at either, a legacy system package was impossible to reference from a manifest with tools version 5.2.

However, you could still get at it indirectly if you inserted a proxy package in between that had a 5.1 tools version or earlier. Since the test was grandfathered in and its client’s manifest was super old, this is how the test continued to pass without anyone noticing the breakage.

Were there any bugs reported for Swift 5.2 complaining that it didn’t work? Or had everyone dealt with the deprecation warnings (since 4.2) by then so that no one was using system packages anymore?

I ask because an alternative to the fix I’ve implemented here would be to just use the opportunity to turn the deprecation into an obsoletion and forego the extra complexity. 5.2 already effectively obsoleted it anyway, even if by accident.

Let me know which route you’d rather take.

[hartbit]

First things first: amazing work! This was a big task and it’s great to know that SE-0226 will be completed very soon 😊

To answer the different points:

1. When I designed swift run, part of the reason was to be able to use SPM to resolve specific versions of development CLI tools like Sourcery, SwiftGen and be able to run them against my projects. I expect other people are also depending on swift run dependency‐executable to work, so it’s great that you found a way to keep supporting that. Why do you say that solution has some minor drawbacks?

But it’s true that perhaps we ought to design a way of depending on executable dependency products.

2. I think you made the right choice here. I’d be surprised if anybody depends on swift build —target to build dependency targets.

3. I don’t have any strong opinion here.

[SDGGiesbrecht]

Why do you say that solution has some minor drawbacks?

I included a brief explanation in the fix‐me directly in source.

1. It means swift run foo, swift run bar and swift build all have potentially different dependency graphs (because the executables might need extra packages). If you go back and forth, it may have to re‐resolve each time. Your pins could jump back and forth.

   Workaround: You can unify the dependency set by creating an otherwise empty internal target which explicitly depends on the executables you care about. Or you can add them as explicit dependencies of your tests or some other existing internal target.

2. If that executable is the only thing you are using from that dependency, then you will have to live with a warning about the dependency being unused.

   Workaround: Explicitly attach it to something, just like for 1.

This is why I think a new manifest entry for “tools needed during development, but not needed for any target” could be useful. It would remove the need to hook them up to some arbitrary target just to avoid these two inconveniences. But I think that can be done as a separate evolution proposal later on.

[hartbit]

Thanks for the details. Perhaps it could be designed with the external build script functionality.

…

On 15 May 2020, at 00:25, Jeremy David Giesbrecht ***@***.***> wrote: Why do you say that solution has some minor drawbacks? I included a brief explanation in the fix‐me directly in source <https://github.com/apple/swift-package-manager/blob/4f434c0f5405e45033c36bcfb389c798ac0dd004/Sources/PackageGraph/PackageGraphRoot.swift#L108-L119>. It means swift run foo, swift run bar and swift build all have potentially different dependency graphs (because the executables might need extra packages). If you go back and forth, it may have to re‐resolve each time. Your pins could jump back and forth. Workaround: You can unify the dependency set by creating an otherwise empty internal target which explicitly depends on the executables you care about. Or you can add them as explicit dependencies of your tests or some other existing internal target. If that executable is the only thing you are using from that dependency, then you will have to live with a warning about the dependency being unused. Workaround: Explicitly attach it to something, just like for 1. This is why I think a new manifest entry for “tools needed during development, but not needed for any product” could be useful. It would remove the need to hook them up to some arbitrary target just to avoid these two inconveniences. But I think that can be done as a separate evolution proposal later on. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2749 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACPPDY7XKP7VVUVAY7HVGDRRRVXBANCNFSM4NAJAOGQ>.

[abertelrud]

Firstly, as others have said, thanks a ton for all the work you've put into this! This is huge progress.

I agree completely with the comments about the quirks resulting from the ad hoc product dependencies created by mentioning otherwise unreferenced products on the command line. In particular, there should absolutely be follow-on evolution proposals to deal with build-time products as well as examples — both would benefit from being able to be specifically annotated as such in the manifest, especially since build-time artifacts also have to be built differently (in particular, for the host platform).

I agree with @hartbit that the tradeoff made here is the right one for now. One question: in the code the explicit product is an optional; would it make sense to make it an array from the start, or do you feel that that would just exacerbate the problem? (I don't actually know whether it's possible to build multiple products at once from the command line, but it seems logical to do).

[SDGGiesbrecht]

One question: in the code the explicit product is an optional; would it make sense to make it an array from the start, or do you feel that that would just exacerbate the problem? (I don't actually know whether it's possible to build multiple products at once from the command line, but it seems logical to do)

I used String? without much thought, because that was what the CLI currently provides.

Looking closer now, while tagging on of external products also happens to work with swift build --product foo, it exists primarily to support swift run foo. For the latter case, I cannot think of a sensible CLI design that would result in more than one product per invocation.

SwiftPM doesn’t currently provide a way to build multiple specific products from the command line. A CLI for something like swift build --products foo bar would definitely be useful, but it seems like an independent feature to me. I imagine the primary use case for that being products in the root package, which are already part of the graph and don’t need special handling.

I envision all of these explicitProduct parameters becoming obsolete and vanishing if we create a way to declare such dependencies in the manifest. At that point, since the manifest would know about them, they would just be handled automatically by standard resolution.

Tying a hypothetical --products external‐foo external‐bar into this interim system would result in new oddities. If it were to add both products to the same graph resolution, then it could be that swift run external‐foo would succeed and swift run external‐bar would succeed, but swift build --products external‐foo external‐bar would trigger an objection that the package graph is unresolvable. If such a CLI needs to be rushed into production, I think it would be better to internally transform swift build --products external‐foo external‐bar into swift build --product external‐foo ; swift build --product external‐bar and execute it that way instead. But my real preference would be to not create a --products option in the first place until the manifest can declare them properly (or else to limit --products to internal ones until then).

So I guess my opinion is still weakly in favour of String? over [String] for now.

[neonichu]

@swift-ci please smoke test

[SDGGiesbrecht]

I’m pleasantly surprised that no merge conflicts accumulated in the last two weeks. 😄

The Linux failure looks unrelated to me; it was in llbuild before even touching SwiftPM. Let me know if I need to do something about it.

The macOS job was simply cancelled before it got anywhere. (Presumably due to this.)

[neonichu]

@swift-ci please smoke test

[SDGGiesbrecht]

While drafting the comment below, I received a notification of #2769, which makes me think the CI error has nothing to do with this PR. (I have still included the draft below in case any of the information in it turns out to be useful.)

---

I think I’m going to need help figuring out why only 3 out of 4 jobs passed. 🤔

• It doesn’t appear to be a Linux difference, because the self‐hosted variant passed. (Right?)
• It doesn’t appear to be a self/toolchain‐hosted difference, since both passed for macOS. (Right?)
• Nothing in the PR is platform‐specific.
• The offending test is a package with no dependencies, so it barely touches graph resolution.
• The error message is <unknown>:0: warning: missing submodule 'CLib'. If I tinker locally to actually remove CLib, that’s not the message I get. What I get instead is [...]/Sources/SwiftExec/main.swift:1:8: error: no such module 'CLib'.

(My Linux machine does not have sufficient space to build the entire toolchain locally. I can run the self‐hosted tests, but they aren’t the problem.)

[neonichu]

As you saw in #2769, this failure doesn't seem to be related to your change at all, sorry about that. Once the test suite is green again, we can re-run tests for this PR.

[aciidgh]

Is CI the only thing holding this PR back?

[aciidgh]

@swift-ci smoke test

[rballard]

For those of you who've done some code review on this one (@abertelrud @neonichu @aciidb0mb3r @hartbit), do you feel you've done a thorough review and think this is ready to take? This is a big set of changes with risk, but also a very important contribution. I'd like to take this once we've got confidence in the changes.

[SDGGiesbrecht]

15:58:25 [92/529] Testing BuildTests.IncrementalBuildTests/testBuildManifestCaching
15:58:30 FATAL: command execution failed
15:58:30 
java.io.EOFException
15:58:30 	at java.io.ObjectInputStream$PeekInputStream.readFully(ObjectInputStream.java:2737)

Does that mean the test crashed, or that Jenkins’ monitoring crashed?

[SDGGiesbrecht]

For reviewers, an interesting real‐world use case that would be affected by the executable handling (1 above) just popped up in the forums.

[abertelrud]

So I guess my opinion is still weakly in favour of String? over [String] for now.

Thanks a lot for the detailed reasoning here. I agree that supporting multiple opens up a whole bunch of issues that would only complicate this PR.

[abertelrud]

For reviewers, an interesting real‐world use case that would be affected by the executable handling (1 above) just popped up in the forums.

That's an interesting case. But such packages (where there are conflicting dependencies used in disjoint sets of targets) would still have problems with commands that operate on the whole package, such as show-dependencies, so I'm not sure how great the workflow will be when packages have conflicting dependencies (even with this PR). It's something that we should consider making better, but it seems that any package-wide command would then need to take a context in which to evaluate the dependency graph. That could get complicated too.

[neonichu]

@swift-ci please smoke test

[neonichu]

Thanks so much for your PR, @SDGGiesbrecht, this is awesome. Sorry that it took a while to get merged.

[neonichu]

Seeing an issue with https://github.com/vapor/database-kit:

error: the Package.resolved file is most likely severely out-of-date and is preventing correct resolution; delete the resolved file and try again

I think we also missed that the Package.resolved version needs to be updated since we changed the format.

[SDGGiesbrecht]

Yes, your observation is likely correct. Looking closer.

[SDGGiesbrecht]

Presumably just incrementing this? I’ll try it.

[neonichu]

This works for fixing the package I mentioned: neonichu@ffef731

There's probably a much better fix, though. It also seems as if we have some tests that are incorrect, because they are assuming that the product filter is applied even pre 5.2 -- but maybe I am missing something.

[SDGGiesbrecht]

Yes, it seems to be more complicated, but I’m still working on sorting it out. It affects workspace-state.json too.

[SDGGiesbrecht]

Okay, it was confusing me because TSC is swallowing the real error, and it needs repairing.

diff --git a/Sources/TSCUtility/SimplePersistence.swift b/Sources/TSCUtility/SimplePersistence.swift
index dbc550e..6621c80 100644
--- a/Sources/TSCUtility/SimplePersistence.swift
+++ b/Sources/TSCUtility/SimplePersistence.swift
@@ -87,6 +87,8 @@ public final class SimplePersistence {
     public func restoreState(_ object: SimplePersistanceProtocol) throws -> Bool {
         do {
             return try _restoreState(object)
+        } catch let stateError as Error {
+            throw stateError
         } catch {
             throw Error.restoreFailure(stateFile: statePath, error: error)
         }

---

Since this looks like the first time the pins schema has changed, you’ll need to decide how such updates should work.

If the pins should not autoupdate, then the error message simply needs improvement.

diff --git a/Sources/PackageGraph/PinsStore.swift b/Sources/PackageGraph/PinsStore.swift
index 94fc207a..07f8bde8 100644
--- a/Sources/PackageGraph/PinsStore.swift
+++ b/Sources/PackageGraph/PinsStore.swift
@@ -40,7 +40,8 @@ public final class PinsStore {
     /// The schema version of the resolved file.
     ///
     /// * 1: Initial version.
-    static let schemaVersion: Int = 1
+    /// * 2: With products (for SE‐0226); not released yet, but some point after Swift 5.2.
+    static let schemaVersion: Int = 2
 
     /// The path to the pins file.
     fileprivate let pinsFile: AbsolutePath
@@ -76,6 +77,8 @@ public final class PinsStore {
             _ = try self.persistence.restoreState(self)
         } catch SimplePersistence.Error.restoreFailure {
             throw StringError("Package.resolved file is corrupted or malformed; fix or delete the file to continue")
+        } catch SimplePersistence.Error.invalidSchemaVersion {
+            throw StringError("Package.resolved file is in an old, unsupported format; delete the file to continue")
         }
     }

If the pins should autoupdate, then the workspace needs to catch the version mismatch and reset the pins file to a clean state before continuing.

diff --git a/Sources/PackageGraph/PinsStore.swift b/Sources/PackageGraph/PinsStore.swift
index 94fc207a..9de1e8c6 100644
--- a/Sources/PackageGraph/PinsStore.swift
+++ b/Sources/PackageGraph/PinsStore.swift
@@ -40,7 +40,8 @@ public final class PinsStore {
     /// The schema version of the resolved file.
     ///
     /// * 1: Initial version.
-    static let schemaVersion: Int = 1
+    /// * 2: With products (for SE‐0226); not released yet, but some point after Swift 5.2.
+    static let schemaVersion: Int = 2
 
     /// The path to the pins file.
     fileprivate let pinsFile: AbsolutePath
diff --git a/Sources/Workspace/Workspace.swift b/Sources/Workspace/Workspace.swift
index b2d9ad26..49527f26 100644
--- a/Sources/Workspace/Workspace.swift
+++ b/Sources/Workspace/Workspace.swift
@@ -440,7 +440,13 @@ public class Workspace {
         self.fileSystem = fileSystem
 
         self.pinsStore = LoadableResult {
-            try PinsStore(pinsFile: pinsFile, fileSystem: fileSystem)
+            do {
+                return try PinsStore(pinsFile: pinsFile, fileSystem: fileSystem)
+            } catch SimplePersistence.Error.invalidSchemaVersion {
+                // Automatically reset.
+                try fileSystem.removeFileTree(pinsFile)
+                return try PinsStore(pinsFile: pinsFile, fileSystem: fileSystem)
+            }
         }
         self.state = WorkspaceState(dataPath: dataPath, fileSystem: fileSystem)
     }

Some of the tests will also need minor adjustment to correspond to expected behaviour.

---

It’s late where I am, so I’m done for the day. If there is a hurry, feel free to go ahead and implement either solution without waiting for me. Otherwise I’ll check for your answer tomorrow.

[neonichu]

There's no rush, thanks for looking into this!

I think we would want to auto update, but ideally in an way that carries the exact resolved versions forward, but we can do that as an incremental change to having updating at all.

[hartbit]

I'm not sure I understand your fix to SimplePersistence. Is Error in the diff SimplePersistence.Error? So you're just not wrapping in restoreError when its another SimplePersistence.Error?

[neonichu]

I think we can actually avoid changing the resolved file format entirely, will make a PR soon.

[SDGGiesbrecht]

I'm not sure I understand your fix to SimplePersistence.

SimplePersistence.Error has two cases:

1. case invalidSchemaVersion(Int)
2. case restoreFailure(stateFile: AbsolutePath, error: Swift.Error)

The loading method first loads raw JSON, checks the version, and throws the invalidSchemaVersion case if it doesn’t match before actually trying to decode the rest of the data. If any of the file operations or data decoding fail, miscellaneous errors will be passed on.

That method is wrapped inside a do‐catch block which catches any error and wraps it into the restoreFailure case before rethrowing it.

I assume that outer method was only intended to catch and wrap the miscellaneous errors, but the way it’s structured, it also wraps the invalidSchemaVersion error. The only way a invalidSchemaVersion error can occur is nested, i.e. restoreFailure(stateFile: _, error: .invalidSchemaVersion). That doesn’t seem quite right to me as it makes it unintuitive to catch. While I was debugging, attempting to directly catch .invalidSchemaVersion wasn’t catching anything, so for a long time I thought the problem must actually be somewhere else.

My fix suggestion was to forward the .invalidSchemaVersion straight through without wrapping it, so that on the SwiftPM side, the error was intuitively catchable (and the two suggestion diffs relied on it).

I think we can actually avoid changing the resolved file format entirely, will make a PR soon.

Great. If that works, it is a much better solution.