Skip to content

load dependency graph based on data of workspace-state.json #378

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

aelam
Copy link
Contributor

@aelam aelam commented Jul 30, 2022

Changes

  • load edited/local/remote packages info from workspace-state.json
  • side-fix: edited package source is loaded from Packages or custom local path, originally it was still loaded from .build/checkouts after ran swift package edit xxx

Note

  • TODO: delete getRemotePackages, getLocalPackages, getEditPackages,
  • editing packages are showing editing rather than displaying version.
    Do you prefer I fix it in this PR or create another PR (I need to read baseOn node from the json and probably showing like editing 2.3.1 may be better)

@swift-server-bot
Copy link

Can one of the admins verify this patch?

3 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

@adam-fowler adam-fowler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good. I was going to hold off on merging this because I was just about to do a new release but it looks like it is going to be delayed by a few days, so these changes should have time to bed in.

Thanks for your contribution

@adam-fowler
Copy link
Contributor

@swift-server-bot test this please

Copy link
Contributor

@adam-fowler adam-fowler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just caught a minor issue

} else {
// remote
const buildDirectory = buildDirectoryFromWorkspacePath(workspaceFolder, true);
return path.join(buildDirectory, "checkouts", dependency.subpath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Package path is used for different things, based on the kind of dependency you have. Remote dependencies store the web address of the repo in there so the View Repository item in the right click menu works. You can either resurrect this setup or add a location parameter to PackageNode which stores the repo URL (Your call). If you add the location parameter you will need to change the code for openInExternalEditor in commands.ts to use this parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
let me add a location

Do we show View Repository for editing packages?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at the moment, but if we have a separate parameter we could.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed it in 8d7b216

@adam-fowler adam-fowler linked an issue Jul 30, 2022 that may be closed by this pull request
@aelam
Copy link
Contributor Author

aelam commented Jul 31, 2022

#370 (comment)

About workspace-state.json, I still get some concerns

I have read the code of Swift package manager
https://github.com/apple/swift-package-manager/blob/main/Sources/Workspace/Workspace.swift.
Here I get some understandings.

  • save() action of workspace-state.json is performed quite often even there is no change.
  • once a package is added and resolved, it will be treated as a managed dependency, even you delete it from your package.swift fully. it will not be deleted until engineers run swift package reset or delete workspace-state.json

(ideally spm should not change workspace-state.json and package.resolved just as they like it)

it may be not a problem for engineers for showing the unused dependencies
But please evaluate it about the final solution

@adam-fowler
Copy link
Contributor

adam-fowler commented Jul 31, 2022

Hmmm. I didn't realise remote dependencies weren't removed from the workspace-state.json when they are removed from the Package.swift. Looking at the SwiftPM code it has an early out, which avoids resolution if the current set of repo versions fits the package version constraints. Which is true even if you have extra packages lying around.

We could delete the workspace-state.json but that could have unintended consequences.

@aelam
Copy link
Contributor Author

aelam commented Jul 31, 2022

remote dependencies weren't removed

local dependencies are the same case

Hmmm. I didn't realise remote dependencies weren't removed from the workspace-state.json when they are removed from the Package.swift. Looking at the SwiftPM code it has an early out, which avoids resolution if the current set of repo versions fits the package versions required. Which is true even if you have extra packages lying around.

That's the reason I think we should traverse all the dependencies without changing workspace-state.json by ourselves which is doing the same thing as show-dependency minus file changing of workspace-state.json
(I think apple should fix it?)

the time complexity is O(n). and if we can handle the file change event well. there should be no problem in performance.

We could delete the workspace-state.json but that could have unintended consequences

This is true, and the file watchers might need to be organized again.

@adam-fowler
Copy link
Contributor

Traversing all the dependencies could be expensive (Vapor projects have over 20 dependencies). Also there are some large packages out there. It is frustrating we would have to repeat the work that SwiftPM has already done.

@aelam
Copy link
Contributor Author

aelam commented Jul 31, 2022

the cost is linear time, I'm not sure the performance of swift package describe, it may be expensive or maybe not.

so we get two solutions.

  • just show the dependencies from workspace-state.json. engineers run swift package reset when needed
  • traversing all dependencies (cache result and update UI until necessary)

Which one do you prefer?

@adam-fowler
Copy link
Contributor

Well what we have on main works for remote dependencies, because it is using the Package.resoved file. I guess you could implement traversing local dependencies and use the Package.resolved for remote dependencies.

By the way I have packages where running swift package describe on all their dependents takes over 20 seconds which isn't really acceptable, since previously it took less than a second.

@aelam
Copy link
Contributor Author

aelam commented Aug 1, 2022

#382

Please check this one. here should cover all cases

@aelam
Copy link
Contributor Author

aelam commented Aug 1, 2022

By the way I have packages where running swift package describe on all their dependents takes over 20 seconds which isn't really acceptable, since previously it took less than a second.

hmm, not very sure what it's doing inside
I don't understand why Apple designed the resolved file like that which means there is no actual file to record depdency tree except parsing based on several files

@aelam
Copy link
Contributor Author

aelam commented Aug 1, 2022

@aelam aelam closed this Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Locally edited packages in Packages folder should be listed
3 participants