refactor workspace repository handling #3766
Conversation
tomerd
requested review from
abertelrud,
friedbunny and
neonichu
as code owners
|
needs #3698 first |
tomerd
force-pushed
the
refactor/workspace-package-container-provider
branch
2 times, most recently
from
1dc9c22 to
7859936
Compare
motivation: make supporting registry depedencies easier changes: * rename LocalPackageContainer to FileSystemPackageContainer and move it to workspace module as internal struct * rename RepositoryPackageContainer to SourceControlPackageContainer and move it to the workspace module as internal class * remove RepositoryPackageContainerProvider and move its logic to workspace itself, including the logic to switch over dependecy type (local/remote) and providing the correct container type * conform workspace to PackageContainerProvider and change getContiner call-sites to use it * reorgnize repository related methods in a consistent manner so its easier to read the code that deals with repositories * adjust call sites and tests rdar://81621441
tomerd
force-pushed
the
refactor/workspace-package-container-provider
branch
from
7859936 to
6a99db8
Compare
|
@swift-ci please smoke test |
|
|
||
| extension Workspace { | ||
| extension Workspace: PackageContainerProvider { | ||
| public func getContainer( |
There was a problem hiding this comment.
👀 this is the key change, it replaces RepositoryPackageContainerProvider which was removed
Sources/Workspace/Workspace.swift
Outdated
| case .localSourceControl, .remoteSourceControl: | ||
| return try self.checkoutRepository(package: package, at: checkoutState) | ||
| case .registry: | ||
| fatalError("registry dependencies are supported at this point") |
There was a problem hiding this comment.
@mattt this is where you can plug the registry "fetch" operation. we may want to do something about CheckoutState since its very source control centric. open to ideas
Sources/Workspace/Workspace.swift
Outdated
| case .localSourceControl, .remoteSourceControl: | ||
| try self.removeRepository(dependency: dependencyToRemove) | ||
| case .registry: | ||
| fatalError("registry dependencies are supported at this point") |
There was a problem hiding this comment.
@mattt this is where you would plug the registry remove handler, if one is needed (for cleanup)
| } | ||
|
|
||
| private extension Workspace.ManagedArtifact { | ||
| fileprivate extension Workspace.ManagedArtifact { |
There was a problem hiding this comment.
the diff above is a mess, but nothing material has changed, just moved methods around to better organize the code in this huge file
|
@swift-ci please smoke test |
| case .root, .fileSystem: | ||
| fatalError("local dependencies are supported") | ||
| case .localSourceControl, .remoteSourceControl: | ||
| try self.removeRepository(dependency: dependencyToRemove) |
There was a problem hiding this comment.
Should these two cases be routed to the same method? It seems to me that the current implementation of removeRepository(dependency:) handles only remote source control dependencies, but not local repositories.
tomerd
force-pushed
the
refactor/workspace-package-container-provider
branch
from
1713cb1 to
904cf8d
Compare
|
@swift-ci please smoke test |
tomerd
added
the
ready
motivation: make supporting registry depedencies easier
changes:
rdar://81621441