traverse local depedencies from package.swift #382
Conversation
|
Can one of the admins verify this patch? |
3 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
aelam
force-pushed
the
fix/wl-depedencies-from-workspace-state-traverse
branch
from
c76f207 to
a29a4ce
Compare
| if (!workspaceState) { | ||
| return []; | ||
| } | ||
| const inUseDependencies = await this.getInUseDependencies(workspaceState, folderContext); |
There was a problem hiding this comment.
Can we add a comment saying why we have to do this
src/ui/PackageDependencyProvider.ts
Outdated
| return new Set<string>(folderContext?.swiftPackage.resolved?.pins.map(pin => pin.identity)); | ||
| } | ||
|
|
||
| private async getAllLocalDependencySet( |
There was a problem hiding this comment.
Function headers? You've added them for the other functions
src/ui/PackageDependencyProvider.ts
Outdated
| * @param showingDependencies result of dependencies that are showing in the tree | ||
| * @returns | ||
| */ | ||
| private async getChildLocalDependencySet( |
There was a problem hiding this comment.
Is it worth having this.getChildLocalDependencySet return a Set instead of passing a Set in to be filled. When you are parsing children of children you could combine the sets. It makes for slightly cleaner code.
There was a problem hiding this comment.
Maybe convert to a stack version?
| * Why not using `workspace-state.json` directly? | ||
| * * `workspace-state.json` contains all necessary dependencies but it also contains dependencies that are not in use. | ||
| * Here is the implementation details: | ||
| * 1. local/remote/edited dependency has remote/edited dependencies, Package.resolved covers them |
There was a problem hiding this comment.
After I tried more cases, I just found item 1 is wrong. originally I saw edited remote package is in Package.resolved <-- but it's not . perhaps I prepared a wrong test case
the test case is
- A depends on B, B depends on C
swift package edit C, C will dissapear fromPackage.resolved
This means we can only check if C is in use by traversing from root or we just show edited dependencies out anyway.
then we have to go back to the original discuss point.
It seems traversing is the only way to get correct dependencies graph
Currently I added getEditedDependencySet which read edited packages from workspace-state.json, but it's still wrong. because once A is removed from Package.swift. C is still in workspace-state.json as edited. so it will still be in the list (deleting state.json could fix the issue)
lemme summarize a bit again
- tranverse from root, this is the best result [ no missing items, no extra items, but concern about performance]
- load dependencies from workspace-state.json [no missing items, but sometimes extra items, no performance issue (deleting state.json could fix the issue)]
- current PR, traversing local dependencies + reading editing from state.json + remote packages from pin [extra editing packages sometimes (deleting state.json could fix the issue)]
my preference is 1 >= 2 >= 3
What do you think
There was a problem hiding this comment.
- This is a real performance issue
- Yeah workspace-state.json would be the best solution, but it isn't kept up to date. I don't know what the implications of deleting the workspace-state.json every time we resolve is. I have added this issue to the SwiftPM repo Removing a dependency from the Package.swift does not remove it from workspace-state.json swift-package-manager#5727
- I'm happy to go with this temporarily. It improves on our current situation.
There was a problem hiding this comment.
I also created a PR swiftlang/swift-package-manager#5700
if it can be accepted, workspace-state.json file could be watched in future
swiftlang/swift-package-manager#5727 I'm not sure if they will treat it as a bug, because they may still treat the unused dependencies as "ManagedDepdency", hopefully, they can fix this issue
3: requires traverse, 4: reads from
package.resolved