Skip to content
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

Add subcommands to install and uninstall packages #6768

Merged
merged 21 commits into from
Aug 3, 2023

Conversation

NSAntoine
Copy link
Contributor

@NSAntoine NSAntoine commented Aug 1, 2023

Rewritten continuation of #5591, however the update subcommand has been taken out temporarily as we figure out the remote-scm situation right now.

This adds swift package experimental-install and swift package experimental-install. The experimental- prefix is added since this hasn't gone through Swift Evolution yet.

@NSAntoine
Copy link
Contributor Author

@tomerd @MaxDesiatov

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Overall I'm happy to see that the scope of the change is reduced, but this needs path and directory layout handling unified, formatting cleanups, usage of CLI options instead of interactive input, and improved testability through the use of FileSystem.

@NSAntoine
Copy link
Contributor Author

Overall I'm happy to see that the scope of the change is reduced, but this needs path and directory layout handling unified, formatting cleanups, usage of CLI options instead of interactive input, and improved testability through the use of FileSystem.

Seems like everything here has been addressed, could you take a look now?

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

We're getting much closer, but I still have a couple of comments on naming and formatting.

Sources/Commands/PackageTools/InstalledPackages.swift Outdated Show resolved Hide resolved
Sources/Commands/PackageTools/InstalledPackages.swift Outdated Show resolved Hide resolved
@@ -465,7 +485,7 @@ extension FileSystem {
return try swiftSDKsDirectory
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unnecessary trailing newline. To get rid of these altogether you can run ./Utilities/soundness.sh in this package, assuming you have SwiftFormat installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolving locally

Copy link
Contributor

Choose a reason for hiding this comment

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

would you be able to push that formatting fix to this branch as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

still present in the diff

Sources/Commands/PackageTools/InstalledPackages.swift Outdated Show resolved Hide resolved
Sources/Basics/FileSystem/FileSystem+Extensions.swift Outdated Show resolved Hide resolved
Sources/Commands/PackageTools/InstalledPackages.swift Outdated Show resolved Hide resolved
Sources/Commands/PackageTools/ResetCommands.swift Outdated Show resolved Hide resolved
Sources/Commands/PackageTools/InstalledPackages.swift Outdated Show resolved Hide resolved
Sources/Commands/PackageTools/SwiftPackageTool.swift Outdated Show resolved Hide resolved
}

try tool.fileSystem.removeFileTree(whatWeWantToRemove.url)
print("Removed \(name).")
Copy link
Contributor

@MaxDesiatov MaxDesiatov Aug 2, 2023

Choose a reason for hiding this comment

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

Make this more descriptive:

Suggested change
print("Removed \(name).")
print("Executable product `\(name)` was successfully uninstalled from \(removedExecutable.path).")

@NSAntoine
Copy link
Contributor Author

@MaxDesiatov All addressed.

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Thanks! I've addressed formatting issues in a follow-up commit. For the future when contributing to SwiftPM I highly recommend following CONTRIBUTING.md, esp. item 6 WRT formatting and use of SwiftFormat.

@MaxDesiatov
Copy link
Contributor

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@MaxDesiatov MaxDesiatov self-assigned this Aug 3, 2023
@MaxDesiatov
Copy link
Contributor

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

1 similar comment
@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@MaxDesiatov MaxDesiatov merged commit c410d6d into swiftlang:main Aug 3, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants