improve output when resolving packages #3427

tomerd · 2021-04-23T00:05:58Z

motivation: when resolving packages, the output is lacking key information about computing the appropriate package version. this can mislead users about what SwiftPM is doing and where the time is spent.

changes:

expose willComputeVersion/didComputeVersion on WorkspaceDelegate
use willComputeVersion/didComputeVersion in SwiftTool::ToolWorkspaceDelegate to print information about the version computation stage
add duration information to WorkspaceDelegate::fetchingDidFinish and WorkspaceDelegate::repositoryDidUpdate
print duration infortmation SwiftTool::ToolWorkspaceDelegate about fetching and updating, in addition to computing
remove empty delegates since they are potentialy hiding dperecations
adjust test

motivation: when resolving packages, the output is lacking key information about computing the appropriate package version. this can mislead users about what SwiftPM is doing and where the time is spent. changes: 1. expose willComputeVersion/didComputeVersion on WorkspaceDelegate 2. use willComputeVersion/didComputeVersion in SwiftTool::ToolWorkspaceDelegate to print information about the version computation stage 3. add duration information to WorkspaceDelegate::fetchingDidFinish and WorkspaceDelegate::repositoryDidUpdate 4. print duration infortmation SwiftTool::ToolWorkspaceDelegate about fetching and updating, in addition to computing 5. remove empty delegates since they are potentialy hiding dperecations 6. adjust test

tomerd · 2021-04-23T00:12:05Z

Sources/Commands/SwiftTool.swift

    return AbsolutePath(env, relativeTo: workingDir)
 }

-/// Returns the sandbox profile to be used when parsing manifest on macOS.


this is just dead code I removed, probably need separate PR

tomerd · 2021-04-23T00:13:07Z

Sources/Workspace/Workspace.swift

    func didDownloadBinaryArtifacts()
 }

-public extension WorkspaceDelegate {


this is likely to impact users of libSwiftPM, but I think worth doing o that the delegates dont run into runtime issues when we deprecate APIs

Agreed, this is worth resolving in the client. Once we adopt semantic versioning this will also stop becoming an issue.

tomerd · 2021-04-23T00:17:59Z

old output

❯ rm -rf .build && swift package update
Fetching https://github.com/apple/swift-atomics.git from cache
Fetching https://github.com/apple/swift-numerics.git from cache
Fetching https://github.com/apple/swift-nio.git from cache
Cloning https://github.com/apple/swift-atomics.git
Resolving https://github.com/apple/swift-atomics.git at 0.0.3
Cloning https://github.com/apple/swift-nio.git
Resolving https://github.com/apple/swift-nio.git at 2.27.0
Cloning https://github.com/apple/swift-numerics.git
Resolving https://github.com/apple/swift-numerics.git at 0.1.0

new output (updated)

❯ rm -rf .build && ~/code/swift/swift-package-manager/.build/debug/swift-package update
Fetching https://github.com/apple/swift-atomics.git from cache
Fetching https://github.com/apple/swift-numerics.git from cache
Fetching https://github.com/apple/swift-nio.git from cache
Fetched https://github.com/apple/swift-numerics.git (1.10s)
Fetched https://github.com/apple/swift-atomics.git (1.10s)
Fetched https://github.com/apple/swift-nio.git (1.15s)
Computing version for https://github.com/apple/swift-atomics.git
Computed https://github.com/apple/swift-atomics.git at 0.0.3 (0.03s)
Computing version for https://github.com/apple/swift-numerics.git
Computed https://github.com/apple/swift-numerics.git at 0.1.0 (0.03s)
Computing version for https://github.com/apple/swift-nio.git
Computed https://github.com/apple/swift-nio.git at 2.27.0 (0.04s)
Creating working copy for https://github.com/apple/swift-numerics.git
Working copy of https://github.com/apple/swift-numerics.git resolved at 0.1.0
Creating working copy for https://github.com/apple/swift-nio.git
Working copy of https://github.com/apple/swift-nio.git resolved at 2.27.0
Creating working copy for https://github.com/apple/swift-atomics.git
Working copy of https://github.com/apple/swift-atomics.git resolved at 0.0.3

tomerd · 2021-04-23T00:18:40Z

@swift-ci please smoke test

Sources/Commands/SwiftTool.swift

abertelrud · 2021-04-23T00:42:39Z

Sources/Workspace/Workspace.swift

+    /// Called when the resolver begins to be compute the version for the repository.
+    func willComputeVersion(package: PackageIdentity, location: String)
+    /// Called when the resolver finished computing the version for the repository.
+    func didComputeVersion(package: PackageIdentity, location: String, version: String, duration: DispatchTimeInterval)


Adding this callback is particularly great — is it guaranteed that the package is "done" at this point, i.e. checked out to the right version and there is no chance that PubGrub might go back and check out a different version?

Also, I think that the "version" at this point could be anything, right? A branch name, etc? I wonder if we should have a more generic name for the parameter.

Adding this callback is particularly great — is it guaranteed that the package is "done" at this point, i.e. checked out to the right version and there is no chance that PubGrub might go back and check out a different version?

afaict it is

Also, I think that the "version" at this point could be anything, right? A branch name, etc? I wonder if we should have a more generic name for the parameter.

I believe this is actually only used for actual semantic versions, using branches will trigger a different flow where this will not be called afaict.

see #3427 (comment)

I see. It makes sense, since these are all resolved messages that are being added. I think it's fairly useful, at least in a UI, to see what branches individual packages get checked out to — that would be an argument for keeping the output from the checkout callbacks.

Maybe "evaluate"?

Evaluating version for https://github.com/apple/swift-argument-parser.git Evaluated https://github.com/apple/swift-argument-parser.git to 0.3.2 (0.04s)

"Compute" is fine though Computed ... at ... doesn't sound right. 🤔

I'm wondering if "checking out" wouldn't still be a better term for the actual git checkout at the end, since that does match the semantics of the Git operation.

part of the idea here is to decouple this from git terminology as with SE-0292 some of these will not be git operations at all

I'm wondering if "checking out" wouldn't still be a better term for the actual git checkout at the end, since that does match the semantics of the Git operation.

part of the idea here is to decouple this from git terminology as with SE-0292 some of these will not be git operations at all

That's a good point. In that sense, does it make sense to combine "creating working copy" and "resolved" to just "creating working copy for at " since in the case of SE-0292 there won't be a separate copying vs checking out step.

I guess I didn't realize until just now that the "working copy resolved" is the matching "did" part of the "creating working copy" message.

I guess I didn't realize until just now that the "working copy resolved" is the matching "did" part of the "creating working copy" message.

its actually not :) they are two distinct actions: creating the working copy, then pointing it to the computed version. In the suggested output above, we are printing on "willCreateWorkingCopy" and "didCheckout". printing before creating the working directory is important because sometime its slow so we need to give the user an indication of what is happening. printing when checkout is complete is to let the user know which revision (version, branch) we landed on eventually and is actually not super useful other than when using branch dependencies.

I see. Either way I think that it would be good to start each line with a verb, for consistency. But it's a minor point.

tomerd · 2021-04-23T00:51:09Z

@swift-ci please smoke test

tomerd · 2021-04-23T01:18:02Z

@swift-ci please smoke test

tomerd · 2021-04-23T03:44:20Z

@swift-ci please smoke test

Sources/PackageGraph/DependencyResolver.swift

abertelrud

LGTM. Thanks for taking this on!

tomerd · 2021-04-23T16:30:19Z

@swift-ci please smoke test

motivation: when resolving packages, the output is lacking key information about computing the appropriate package version. this can mislead users about what SwiftPM is doing and where the time is spent. changes: 1. expose willComputeVersion/didComputeVersion on WorkspaceDelegate 2. use willComputeVersion/didComputeVersion in SwiftTool::ToolWorkspaceDelegate to print information about the version computation stage 3. add duration information to WorkspaceDelegate::fetchingDidFinish and WorkspaceDelegate::repositoryDidUpdate 4. print duration infortmation SwiftTool::ToolWorkspaceDelegate about fetching and updating, in addition to computing 5. remove empty delegates since they are potentialy hiding dperecations 6. adjust test

tomerd requested review from abertelrud, friedbunny and neonichu as code owners April 23, 2021 00:05

tomerd commented Apr 23, 2021

View reviewed changes

abertelrud reviewed Apr 23, 2021

View reviewed changes

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

abertelrud reviewed Apr 23, 2021

View reviewed changes

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

fixup

d438a67

tomerd force-pushed the feature/resolver-delegate-2 branch from fb8868f to d438a67 Compare April 23, 2021 01:15

fixup

109979a

abertelrud reviewed Apr 23, 2021

View reviewed changes

Sources/PackageGraph/DependencyResolver.swift Show resolved Hide resolved

abertelrud approved these changes Apr 23, 2021

View reviewed changes

Merge branch 'main' into feature/resolver-delegate-2

a343097

tomerd self-assigned this Apr 23, 2021

neonichu approved these changes Apr 26, 2021

View reviewed changes

tomerd merged commit fc36e07 into swiftlang:main Apr 27, 2021

tomerd mentioned this pull request Apr 27, 2021

[5.5] improve output when resolving packages #3446

Merged

tomerd added a commit that referenced this pull request Apr 27, 2021

improve output when resolving packages (#3427) (#3446)

0bc0a71

improve output when resolving packages #3427

improve output when resolving packages #3427

Uh oh!

Conversation

tomerd commented Apr 23, 2021

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 Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomerd commented Apr 23, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Apr 23, 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!

tomerd Apr 23, 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!

tomerd commented Apr 23, 2021

Uh oh!

tomerd commented Apr 23, 2021

Uh oh!

tomerd commented Apr 23, 2021

Uh oh!

Uh oh!

abertelrud left a comment

Choose a reason for hiding this comment

Uh oh!

tomerd commented Apr 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tomerd commented Apr 23, 2021 •

edited

Loading

tomerd Apr 23, 2021 •

edited

Loading

tomerd Apr 23, 2021 •

edited

Loading