Add `swift package-registry` command #3647

mattt · 2021-08-04T18:22:06Z

This PR implements the swift package-registry command with its set and unset subcommands as described in "Registry configuration subcommands"

Motivation:

Implement SE-0292.

Modifications:

Add PackageRegistry library and test target containing
Create a RegistryConfiguration type in the PackageRegistry module
Create a new swift-package-registry executable target
~~Implement .netrc support~~ (see Add swift package-registry command #3647 (comment))

Result:

With this change, Swift Package Manager will be able to read and write registry configuration files.

mattt · 2021-08-04T18:25:27Z

@tomerd @yim-lee I spun this off from #3640 in the interest of taking incremental steps towards SE-0292 implementation. Let's focus on this chunk first, before moving onto the actual dependency resolution logic.

Sources/Commands/SwiftPackageRegistryTool.swift

mattt · 2021-08-05T17:07:29Z

@tomerd I started to implement .netrc support, but realized that this would require a lot more work. From SE-0292:

Specifying credentials for a custom registry

Some servers may require a username and password.
The user can provide credentials when setting a custom registry by passing the --login and --password options.

When credentials are provided, the corresponding object in the registries.json file includes a login key with the passed value. If the project's .netrc file has an existing entry for a given machine and login, it's updated with the new password; otherwise, a new entry is added. If no .netrc file exists, a new one is created and populated with the new entry.

Right now, the Netrc struct in swift-tools-support-core supports read-only access, but the feature described here requires updating the file with new entries. Adding new entries should be easy enough — just append it to the end of the file. However, updating existing entries is more difficult to get right.

How do you think we should proceed? I can think of a few possibilities:

Defer: We temporarily remove the --login and --password options for the swift package-registry set command, and require users to manage credentials themselves. (This is what we have with the latest commit for this PR, ffebeb6)
Append-only: We do the 80% solution and update .netrc files by adding a new machine only if it doesn't already exist. If there's already an entry for a machine, we bail and emit a warning. (If we do this, we should probably amend the proposal to describe this new behavior).
Full authoring support: We go all the way and make a compliant .netrc file manager.

I suspect we don't really want to get in the business of .netrc file authoring, so option 3 is probably a no-go. I don't feel too strongly between 1 and 2. If you'd like, we can even do both — accept what we have now and punt on the 80% solution for later.

What do you think?

Package.swift

Sources/Commands/SwiftPackageRegistryTool.swift

Sources/PackageRegistry/CMakeLists.txt

tomerd · 2021-08-06T18:22:08Z

Sources/Commands/SwiftPackageRegistryTool.swift

+// MARK: -
+
+private extension Decodable {
+    static func readFromJSONFile(in fileSystem: FileSystem, at path: AbsolutePath) throws -> Self {


we typically prefer to put the visibility modifier on the method and not the extension, i.e.

extension Decodable { private static func readFromJSON ...

I personally like ACL on private and fileprivate extensions, as it prevents helper APIs from leaking out to internal. But I'm happy to make this change if you feel strongly about it.

The only hitch is that the current .swiftformat configuration falls back to the default extensionAccessControl rule behavior of putting ACL on extensions rather than declarations. If we're still using this tool, should we update that to match project conventions?

Sources/Commands/SwiftPackageRegistryTool.swift

Sources/PackageRegistry/RegistryConfiguration.swift

tomerd · 2021-08-06T18:40:19Z

Sources/swift-package-registry/main.swift

+
+import Commands
+
+SwiftPackageRegistryTool.main()


we should probably transition to @main at this point.

@abertelrud @neonichu wdyt about a PR to move SwiftPM to tools version 5.5 and make use of some of the new capabilities?

Tests/PackageRegistryTests/RegistryConfigurationTests.swift

tomerd

looking great. added a couple of ideas about the configuration, happy to help with doing that in a separate PR if it helps.

tomerd · 2021-08-06T18:44:55Z

Right now, the Netrc struct in swift-tools-support-core supports read-only access, but the feature described here requires updating the file with new entries. Adding new entries should be easy enough — just append it to the end of the file. However, updating existing entries is more difficult to get right.

How do you think we should proceed? I can think of a few possibilities:

Defer: We temporarily remove the --login and --password options for the swift package-registry set command, and require users to manage credentials themselves. (This is what we have with the latest commit for this PR, ffebeb6)

Append-only: We do the 80% solution and update .netrc files by adding a new machine only if it doesn't already exist. If there's already an entry for a machine, we bail and emit a warning. (If we do this, we should probably amend the proposal to describe this new behavior).

Full authoring support: We go all the way and make a compliant .netrc file manager.
I suspect we don't really want to get in the business of .netrc file authoring, so option 3 is probably a no-go. I don't feel too strongly between 1 and 2. If you'd like, we can even do both — accept what we have now and punt on the 80% solution for later.

What do you think?

I agree and think option 2 is sufficient at this point

mattt · 2021-08-09T20:56:30Z

@swift-ci Please smoke test

mattt · 2021-08-09T21:07:34Z

@tomerd Aside from the unresolved feedback threads and .netrc, is there anything else you'd like to see for this PR?

Re: .netrc handling — Could we implement this in a follow-up PR? There are a lot of moving pieces between #3640 and #3652 that impact how this would be implemented. Specifically, these two chunks:

https://github.com/apple/swift-package-manager/pull/3640/files#diff-134223fe78a1e46f08f8c1e9d166408b7105f941cb2cc4e844ebceb87a8f4ec8R1850

https://github.com/apple/swift-package-manager/pull/3652/files#diff-8d396837a0426c26967e471b82d8f2eaf31f9a7ae2e03308300799edbac4bd18L563

Add RegistryConfiguration and RegistryConfigurationTests

Co-authored-by: Yim Lee <[email protected]>

…s body

mattt · 2021-08-24T13:44:59Z

@swift-ci Please smoke test

mattt · 2021-08-24T13:47:40Z

@tomerd I just rebased my original PR to follow the pattern you introduced in #3670. Please take a look and let me know if you have any concerns with that approach.

Aside from the unresolved feedback threads and .netrc, is there anything else you'd like to see for this PR?

Sources/Commands/SwiftPackageRegistryTool.swift

Sources/Commands/SwiftTool.swift

Sources/SPMTestSupport/JSONDecoder+Extensions.swift

tomerd

looks good. some leftovers that can be removed, I suspect, but looking great otherwise.

Fix logic for location registries configuration

…s.json

mattt · 2021-08-30T12:09:41Z

@swift-ci Please smoke test

tomerd · 2021-08-30T21:48:26Z

@mattt @elsh will fix the API digester issue on linux and then we will merge this. does that work / is it ready?

tomerd · 2021-08-31T02:45:36Z

@swift-ci smoke test

mattt · 2021-08-31T07:43:12Z

@tomerd Sounds good. Yes, this is all ready to be merged once CI is passing.

mattt requested review from tomerd and yim-lee August 4, 2021 18:22

mattt commented Aug 4, 2021

View reviewed changes

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

tomerd self-assigned this Aug 4, 2021

mattt marked this pull request as ready for review August 5, 2021 17:10

mattt requested review from abertelrud, friedbunny and neonichu as code owners August 5, 2021 17:10

This comment has been minimized.

Sign in to view

yim-lee reviewed Aug 5, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

tomerd reviewed Aug 6, 2021

View reviewed changes

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

tomerd reviewed Aug 6, 2021

View reviewed changes

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

tomerd reviewed Aug 6, 2021

View reviewed changes

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

tomerd reviewed Aug 6, 2021

View reviewed changes

Sources/PackageRegistry/RegistryConfiguration.swift Show resolved Hide resolved

tomerd reviewed Aug 6, 2021

View reviewed changes

Tests/PackageRegistryTests/RegistryConfigurationTests.swift Outdated Show resolved Hide resolved

tomerd reviewed Aug 6, 2021

View reviewed changes

tomerd added the WIP Work in progress label Aug 19, 2021

mattt and others added 5 commits August 24, 2021 03:16

Add PackageRegistry and PackageRegistryTests targets

9e7c32d

Add RegistryConfiguration and RegistryConfigurationTests

Add swift-package-registry command

a59f70e

Temporarily disable login and password options for set command

14243fe

Update comment in package manifest

d4c835d

Co-authored-by: Yim Lee <[email protected]>

Update copyright year

5be6170

Co-authored-by: Yim Lee <[email protected]>

mattt and others added 8 commits August 24, 2021 03:16

Update copyright year

2a01a5a

Co-authored-by: Yim Lee <[email protected]>

Update command abstract

ba06e23

Co-authored-by: Yim Lee <[email protected]>

Fix error: variable declared in 'guard' condition is not usable in it…

45a32f9

…s body

Use JSONEncoder/JSONDecoder makeWithDefaults()

cd0997f

Remove default fileSystem argument in getRegistryConfigurationPath

c5e703e

Create extension on JSONDecoder to decode directly from JSON string

b60f47f

Refactor registry configuration to new pattern

61ee7ad

Fix test failure by using atomic write instead of file locks

d76923f

mattt force-pushed the package-registry-command branch from 05481e2 to d76923f Compare August 24, 2021 13:44

mattt requested a review from tomerd August 24, 2021 13:47

mattt added ready Author believes the PR is ready to be merged & any feedback has been addressed and removed WIP Work in progress labels Aug 25, 2021

tomerd reviewed Aug 27, 2021

View reviewed changes

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

tomerd reviewed Aug 27, 2021

View reviewed changes

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

tomerd reviewed Aug 27, 2021

View reviewed changes

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

tomerd reviewed Aug 27, 2021

View reviewed changes

Sources/SPMTestSupport/JSONDecoder+Extensions.swift Outdated Show resolved Hide resolved

tomerd approved these changes Aug 27, 2021

View reviewed changes

mattt added 3 commits August 30, 2021 05:07

Refactor logic for getRegistriesConfig helper

c0120a2

Fix logic for location registries configuration

Move JSONDecoder+Extensions to Basics module

050909a

Look for registries configuration at .swiftpm/configuration/registrie…

6aa9be2

…s.json

mattt force-pushed the package-registry-command branch from c7820d9 to 6aa9be2 Compare August 30, 2021 12:07

tomerd merged commit 3ba72d1 into swiftlang:main Aug 31, 2021

mattt deleted the package-registry-command branch August 31, 2021 18:16

mattt mentioned this pull request Sep 30, 2021

Add keychain auth provider #3768

Merged


		import Commands

		SwiftPackageRegistryTool.main()

Add swift package-registry command #3647

Add swift package-registry command #3647

Uh oh!

Conversation

mattt commented Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation:

Modifications:

Result:

Uh oh!

mattt commented Aug 4, 2021

Uh oh!

Uh oh!

mattt commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Specifying credentials for a custom registry

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

tomerd Aug 6, 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!

mattt Aug 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomerd Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomerd left a comment

Choose a reason for hiding this comment

Uh oh!

tomerd commented Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattt commented Aug 9, 2021

Uh oh!

mattt commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattt commented Aug 24, 2021

Uh oh!

mattt commented Aug 24, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomerd left a comment

Choose a reason for hiding this comment

Uh oh!

mattt commented Aug 30, 2021

Uh oh!

tomerd commented Aug 30, 2021

Uh oh!

tomerd commented Aug 31, 2021

Uh oh!

mattt commented Aug 31, 2021

Uh oh!

Uh oh!

Add `swift package-registry` command #3647

Add `swift package-registry` command #3647

mattt commented Aug 4, 2021 •

edited

Loading

mattt commented Aug 5, 2021 •

edited

Loading

tomerd Aug 6, 2021 •

edited

Loading

tomerd commented Aug 6, 2021 •

edited

Loading

mattt commented Aug 9, 2021 •

edited

Loading