-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 package registry implementation #3023
Conversation
4fd0fdb
to
fe744a4
Compare
763834d
to
aad4db1
Compare
d8c3f98
to
4af54b0
Compare
|
||
private func manifest(for version: Version) throws -> Manifest { | ||
try manifestsCache.memoize(version) { | ||
try tsc_await { registryManager.fetchManifest(for: version, of: identifier, using: manifestLoader, completion: $0) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably turn manifest(for version: Version
) into an async function, it makes sense in all cases really (does not need to be in this PR)
client.execute(request) { result in | ||
switch result { | ||
case .success(let response): | ||
if response.statusCode == 303, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the http client allows you to configure "allowed response codes" which does this check for you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that, but I didn't see a way to conveniently configure that on a per-request basis as would be needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let `extension` = `extension`.spm_dropPrefix(".") | ||
return AbsolutePath(self, RelativePath("..")).appending(component: "\(basename).\(`extension`)") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to Basics?
@@ -2380,32 +2394,39 @@ extension Workspace { | |||
requirement: PackageStateChange.Requirement, | |||
productFilter: ProductFilter | |||
) throws -> AbsolutePath { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels like another method that should become async. not for this PR tho
@tomerd Thanks for reviewing these latest changes. I'm working through your feedback now. In related news, I'm excited to share some initial benchmarking results for the package registry. I published a harness here that automates (or at least formalizes) many of the steps in described above to get everything set up. Check it out! https://github.com/mattt/swift-registry-benchmark-harness (/cc @neonichu @yim-lee @abertelrud) Preliminary results show comparable performance to cloning repositories for clean builds:
(The numbers look a lot better after 74625e4; I should have a PR to TSCBasic for this sometime next week) |
74625e4
to
31f3155
Compare
RegistryManager.discover(for: identifier) { result in | ||
switch result { | ||
case .success(let registryManager): | ||
RegistryPackageContainer.create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do this entire operation on the queue (untested code):
queue.async {
RegistryPackageContainer.create(
for: identifier,
mirrors: self.mirrors,
registryManager: registryManager,
manifestLoader: self.manifestLoader,
toolsVersionLoader: self.toolsVersionLoader,
currentToolsVersion: self.currentToolsVersion
) { result in
switch result {
case .success(let container):
self.containerCache[identifier] = container
completion(.success(container))
case .failure(let error):
completion(.failure(error))
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternatively, if RegistryPackageContainer.create is a async, pass the queue to it and then we can simplify further
var configuration = HTTPClientConfiguration() | ||
configuration.followRedirects = false | ||
|
||
return HTTPClient(configuration: configuration, handler: nil, diagnosticsEngine: nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we would want to pass in a diagnosticsEngine here, so maybe clientFactory should take one, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added with 15c90e0. I started add diagnostics engine parameters to getContainer
, but I quickly got outside my comfort zone for this PR. Something to consider in a follow-up /cc @abertelrud
for package: PackageReference, | ||
completion: @escaping (Result<RegistryManager, Error>) -> Void | ||
) { | ||
guard let url = URL(string: "https://\(package.identity)") else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is confusing to me.. are we using the package identity as the URL? what am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's correct. This is discussed in § 4.1.1 Content Negotiation.
The new (non-legacy) package identity is a canonicalized, scheme-agnostic URL. The process of "discovering" registry support involves sending a HEAD
to that URL with the right headers to see if the server redirects us to a package registry API endpoint.
…ding manifests Non-legacy package identifiers may include slashes, which cause precondition failures when constructing AbsolutePath values
Add followRedirects property to HTTPClientConfiguration
Add PackageRegistry module target Add PackageRegistryTests module target Add --enable-package-registry command-line option Cache registry managers by base URL Set _useLegacyIdentities in SwiftTool initialization Incorporate review feedback for use of tsc_await Reuse HTTP clients in registry manager Remove custom userAgent This is already provided by HTTPClient Add internal archiverFactory to RegistryManager for DI Add queue parameter to RegistryManager.discover Add diagnosticsEngine parameter to RegistryManager Replace MockPackageRegistryURLProtocol with HTTPClient handlers Remove use of tsc_await in RegistryManager.discover Stash progress
While benchmarking registry performance, I was surprised that RegistryManager.downloadSourceArchive was taking so long. After some further investigation, I found that >90% of the time was spent calculating the SHA256 checksum. This change improves my local end-to-end clean registry build from ~70 seconds to ~15 seconds.
57109a7
to
7e99e02
Compare
As of 5e91c76 (latest force-push), package releases downloaded from a registry are sent to their own, dedicated cache directory. For now, this is called |
Closing in favor of a new PR that implements a future revision to SE-0292. See https://forums.swift.org/t/urls-as-swift-package-identifiers/43404/128 |
Related to #2967.
This PR implements the client-side integration necessary for Swift Package Manager to fetch and resolve dependencies using a package registry.
A bunch of unit tests are currently failing because this integration necessitates a semantic change in how packages are identified. Whereas previously, a package was identified by name, this PR introduces a new
PackageIdentity
type with a more comprehensive notion of what it means for two packages to be the same.Instructions
Edit: Check out the Swift registry benchmark harness, which automates a lot of this setup.
Download the project
Clone the
swift-package-manager
project and check out thepackage-registry-implementation
branch.Create
spm
shimTo try out the local development build of Swift Package Manager in your other project, you can create a shim:
You can now invoke
spm
as you wouldswift package
.Create a package registry
Use the reference implementation to serve a package registry locally. Follow the instructions provided in the README under “Command Line Interface” to publish releases of packages that your project uses.
Generate a local HTTPS certificate
Because the package registry requires all connections to use
https
, you’ll need to serve the package registry with a locally-trusted development certificate. My go-to option for this ismkcert
.Alternatively, you can use ngrok to tunnel local traffic from a public URL over HTTPS.
Mirror your package dependencies to your local
Once your registry is running an accessible from a URL with
https
, add a mirror for any external dependencies that you’d like to serve over the registry.For example, if you’re running a registry locally at
https://localhost:8080
and have a package with a dependency onhttps://github.com/apple/swift-argument-parser
, you would add a mirror with the following command:Build your project
Finally, use
spm
to build your project, cleaning your project beforehand to ensure that dependencies are fetched.