[SR-7836] Added swift package update --dry-run #2380

tgymnich · 2019-10-12T21:42:41Z

Added swift package --dry-run
Added prettyPrinted to Requirement
Added test

Sources/Workspace/Workspace.swift

ankitspd · 2019-10-12T23:21:07Z

Sources/Workspace/Workspace.swift

+
+        guard !dryRun else {
+            let changes = diagnostics.wrap { return try computePackageStateChanges(root: graphRoot, resolvedDependencies: updateResults, updateBranches: true) } ?? []
+            logPackageChanges(changes: changes, pins: pinsStore)


Workspace is a high-level library so it'll be better if we return a structured object from here. Clients can then do whatever they want with the object. In this particular case, maybe you can accept an inout array to fill for dry run.

Great idea! I think it looks way better than splitting updateDependencies into updateDependencies and updateDependenciesDryRun

ankitspd · 2019-10-12T23:21:24Z

Thanks! This is great. I left some comments inline

Sources/Workspace/Workspace.swift

Sources/Commands/SwiftPackageTool.swift

ankitspd · 2019-10-15T19:39:15Z

Sources/Commands/SwiftPackageTool.swift

+            }
+            stream <<< "\n"
+        }
+        stream.flush()


Can you post what an example output looks like? Just curious!

2019-10-16 13:21:43.723885+0200 swift-package[53147:783737] [default] Starting resolution using pubgrub resolver Updating https://github.com/attaswift/BigInt.git Updating https://github.com/apple/swift-protobuf.git 3 dependencies have changed: + SwiftProtobuf 1.7.0 ~ BigInt 4.0.0 -> BigInt 5.0.0 - swift-nio 2.8.0 Program ended with exit code: 0

ankitspd · 2019-10-15T19:40:50Z

Tests/CommandsTests/PackageToolTests.swift

@@ -69,6 +69,26 @@ final class PackageToolTests: XCTestCase {
            XCTAssertEqual(GitRepository(path: path).tags, ["1.2.3", "1.2.4"])
        }
    }
+
+    func testUpdateDryRun() throws {


It would be better if you can write a unit test since they're much much faster to run. You can see WorkspaceTests.swift for examples. Don't worry about it if it's turning out to be too hard.

ankitspd · 2019-10-18T19:41:11Z

Hey @TG908. I ended up being a bit busier than usual this week but I hope to re-review the PR early next week.

ankitspd · 2019-10-28T15:57:32Z

Sources/Commands/SwiftPackageTool.swift

+            } else if let pinsStore = diagnostics.wrap({ try workspace.pinsStore.load() }),
+                let changes = try workspace.updateDependenciesDryRun(
+                    root: getWorkspaceRoot(),
+                    diagnostics: diagnostics) {


Didn't we discuss passing an inout array to get the changes? Any particular reason for switching to a new method?

Since I had to create a new method due to API stability anyway, I chose to use a return value instead of inout because I think the in part of inout would never be used.

I was thinking the parameter would default to nil and we'd use the in part to determine if this is a dry run operation. Something like:

public func updateDependencies( root: PackageGraphRootInput, dryRunResults: [Result]? = nil diagnostics: DiagnosticsEngine ) { ... // Fill the dry run results array if one is provided and return without performing the actual update. if dryRunResults != nil { dryRunResults = updateResults return } ... }

This won't work in swift 5.1 inout parameters cannot have a default value.

Ah. What if we add a specialized method ourselves?

public func updateDependencies( root: PackageGraphRootInput, diagnostics: DiagnosticsEngine ) { updateDependencies(root: root, dryRunResults: nil, diagnostics: diagnostics) } public func updateDependencies( root: PackageGraphRootInput, dryRunResults: [Result]?, diagnostics: DiagnosticsEngine ) { ... }

Is it obvious to someone reading the code for the first time that passing a dry run array causes the actual update not to happen?

Without reading the documentation it is more obvious that the method updateDependencies performs an actual update and updateDependenciesDryRun does not.

You're right, it's not very obvious especially for API consumers. What if we always return the update result and make this method @discardableResult? Then this method could just take a simple Bool dryRun.

- Added swift package --dry-run - Added prettyPrinted to Requirement - Added tests

Sources/Workspace/Workspace.swift

ankitspd

I have some very minor nits but otherwise this is ready to go. Thanks a lot for working on this!

tgymnich · 2019-10-31T11:00:02Z

You are welcome. Thank you for your input!

ankitspd · 2019-10-31T16:13:04Z

@swift-ci smoke test

helje5 · 2019-11-12T12:47:22Z

Given the output in the sample above:

2019-10-16 13:21:43.723885+0200 swift-package[53147:783737] [default] Starting resolution using pubgrub resolver
Updating https://github.com/attaswift/BigInt.git
Updating https://github.com/apple/swift-protobuf.git

3 dependencies have changed:
+ SwiftProtobuf 1.7.0
~ BigInt 4.0.0 -> BigInt 5.0.0
- swift-nio 2.8.0
Program ended with exit code: 0

This isn't a proper --dry-run, right? It'll "wet" the fetch state of the git repositories? Can we add something to avoid that (i.e. move the checkouts to a temporary location or do some form of APFS clone when available)?

ankitspd · 2019-11-12T18:56:31Z

That fetch state is supposed to be an implementation detail. What's your use case?

helje5 · 2019-11-12T19:25:19Z

As part of the Catalog App I'd like to show a user projects he could update and ideally what is new (just like the Updates section in App Store). But in no way I want to modify any of his precious stuff before he clicks an actual "update" button.

Besides, the core definition of --dry-run is that it has zarro side effects.

I understand that this seems quite hard w/ just git, so I'm probably going to APFS clone projects and run a full update.

In any case I suggest that the dox to this "dry-run" explicitly documents that this is going to fetch and not be dry. It's quite unexpected and counter to the point.

ankitspd · 2019-11-12T20:16:01Z

I don't want this behavior documented because it's an implementation detail and we'll probably want to have local shared cache at some point. But it seems fine to consider this behavior as a bug in the current --dry-run implementation.

helje5 · 2019-11-12T20:20:30Z

A local shared cache
😍

tgymnich · 2019-11-14T01:33:05Z

The best solution would be to just copy the workspace into a temporary directory. In case of a dry run a normal update would be performed on a temporary Workspace. I would also remove the dry run argument for updateDependencies again and just always print the summary of changed packages (should I keep returning the changed packages?).

helje5 · 2019-11-15T11:22:35Z

That is what I also plan to do for now. Unfortunately it's a little hard to clone into the same directory :-)

BTW: Something which would also be appreciated is if this would get the JSON output treatment.

tgymnich · 2019-11-15T12:14:30Z

For getting easy to parse output you could also use the swift package manger library to get the outdated packages.

helje5 · 2019-11-15T12:41:08Z

I think that's too much coupling for my purpose, I just want to use the Swift the user has active.

tgymnich · 2019-11-17T02:40:31Z

I created an issue

tgymnich requested review from abertelrud, ankitspd and neonichu as code owners October 12, 2019 21:42

ankitspd reviewed Oct 12, 2019

View reviewed changes

Sources/Workspace/Workspace.swift Outdated Show resolved Hide resolved

ankitspd reviewed Oct 12, 2019

View reviewed changes

Sources/Workspace/Workspace.swift Outdated Show resolved Hide resolved

ankitspd reviewed Oct 12, 2019

View reviewed changes

tgymnich requested a review from ankitspd October 14, 2019 08:09

ankitspd reviewed Oct 15, 2019

View reviewed changes

Sources/Workspace/Workspace.swift Outdated Show resolved Hide resolved

ankitspd reviewed Oct 15, 2019

View reviewed changes

Sources/Workspace/Workspace.swift Outdated Show resolved Hide resolved

ankitspd reviewed Oct 15, 2019

View reviewed changes

Sources/Commands/SwiftPackageTool.swift Outdated Show resolved Hide resolved

ankitspd reviewed Oct 15, 2019

View reviewed changes

tgymnich requested a review from ankitspd October 18, 2019 10:27

ankitspd reviewed Oct 28, 2019

View reviewed changes

[SR-7836] Added swift package update --dry-run

8738e15

- Added swift package --dry-run - Added prettyPrinted to Requirement - Added tests

ankitspd reviewed Oct 30, 2019

View reviewed changes

Sources/Workspace/Workspace.swift Outdated Show resolved Hide resolved

ankitspd reviewed Oct 30, 2019

View reviewed changes

Sources/Workspace/Workspace.swift Outdated Show resolved Hide resolved

ankitspd reviewed Oct 30, 2019

View reviewed changes

Sources/Workspace/Workspace.swift Outdated Show resolved Hide resolved

ankitspd approved these changes Oct 30, 2019

View reviewed changes

tgymnich added 2 commits October 31, 2019 11:55

removed unused function

ad37d82

replaced guard with if

b85811c

ankitspd merged commit c43b137 into apple:master Oct 31, 2019

swift-ci mentioned this pull request May 30, 2020

[SR-7836] SwiftPM should have a command that displays which packages can be updated #4795

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SR-7836] Added swift package update --dry-run #2380

[SR-7836] Added swift package update --dry-run #2380

tgymnich commented Oct 12, 2019

ankitspd Oct 12, 2019

tgymnich Oct 13, 2019

ankitspd commented Oct 12, 2019

ankitspd Oct 15, 2019

tgymnich Oct 16, 2019 •

edited

ankitspd Oct 15, 2019

ankitspd commented Oct 18, 2019

ankitspd Oct 28, 2019

tgymnich Oct 28, 2019

ankitspd Oct 28, 2019 •

edited

tgymnich Oct 28, 2019 •

edited

ankitspd Oct 28, 2019

tgymnich Oct 28, 2019 •

edited

ankitspd Oct 28, 2019

ankitspd left a comment

tgymnich commented Oct 31, 2019 •

edited

ankitspd commented Oct 31, 2019

helje5 commented Nov 12, 2019

ankitspd commented Nov 12, 2019

helje5 commented Nov 12, 2019 •

edited

ankitspd commented Nov 12, 2019

helje5 commented Nov 12, 2019

tgymnich commented Nov 14, 2019 •

edited

helje5 commented Nov 15, 2019

tgymnich commented Nov 15, 2019

helje5 commented Nov 15, 2019

tgymnich commented Nov 17, 2019

[SR-7836] Added swift package update --dry-run #2380

[SR-7836] Added swift package update --dry-run #2380

Conversation

tgymnich commented Oct 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankitspd commented Oct 12, 2019

Choose a reason for hiding this comment

tgymnich Oct 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankitspd commented Oct 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankitspd Oct 28, 2019 • edited

Choose a reason for hiding this comment

tgymnich Oct 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgymnich Oct 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankitspd left a comment

Choose a reason for hiding this comment

tgymnich commented Oct 31, 2019 • edited

ankitspd commented Oct 31, 2019

helje5 commented Nov 12, 2019

ankitspd commented Nov 12, 2019

helje5 commented Nov 12, 2019 • edited

ankitspd commented Nov 12, 2019

helje5 commented Nov 12, 2019

tgymnich commented Nov 14, 2019 • edited

helje5 commented Nov 15, 2019

tgymnich commented Nov 15, 2019

helje5 commented Nov 15, 2019

tgymnich commented Nov 17, 2019

tgymnich Oct 16, 2019 •

edited

ankitspd Oct 28, 2019 •

edited

tgymnich Oct 28, 2019 •

edited

tgymnich Oct 28, 2019 •

edited

tgymnich commented Oct 31, 2019 •

edited

helje5 commented Nov 12, 2019 •

edited

tgymnich commented Nov 14, 2019 •

edited