Skip to content

Commit c684554

Browse files
tomerdtomerd
authored
fix package.resolved interplay with mirrors (#3447) (#3452)
motivation: mirror urls leak today to package.resolved changes: * add storage for "original urls" in the pins store * pass the mirors to the pins file handler * transform the url when encoding and decoding the pins store such that the package.resolved file contains the non-mirrored URL but SwiftPM internals work witht mirrored ones * adjust call-sites * add and adjust tests rdar://52529014 rdar://52529011
1 parent 07e808e commit c684554

File tree

11 files changed

+134
-54
lines changed

11 files changed

+134
-54
lines changed

‎Sources/Commands/SwiftPackageTool.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -801,7 +801,7 @@ extension SwiftPackageTool.Config {
801801
throw ExitCode.failure
802802
}
803803

804-
if let mirror = config.mirrors.getMirror(forURL: originalURL) {
804+
if let mirror = config.mirrors.mirrorURL(for: originalURL) {
805805
print(mirror)
806806
} else {
807807
stderrStream <<< "not found\n"

‎Sources/PackageGraph/DependencyMirrors.swift

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -22,25 +22,30 @@ public final class DependencyMirrors {
2222
case mirrorNotFound
2323
}
2424

25-
private var storage: [String: String]
25+
private var index: [String: String]
26+
private var reverseIndex: [String: String]
2627

2728
private init(_ mirrors: [Mirror]) {
28-
self.storage = Dictionary(mirrors.map({ ($0.original, $0.mirror) }), uniquingKeysWith: { first, _ in first })
29+
self.index = Dictionary(mirrors.map({ ($0.original, $0.mirror) }), uniquingKeysWith: { first, _ in first })
30+
self.reverseIndex = Dictionary(mirrors.map({ ($0.mirror, $0.original) }), uniquingKeysWith: { first, _ in first })
2931
}
3032

3133
/// Sets a mirror URL for the given URL.
3234
public func set(mirrorURL: String, forURL url: String) {
33-
storage[url] = mirrorURL
35+
self.index[url] = mirrorURL
36+
self.reverseIndex[mirrorURL] = url
3437
}
3538

3639
/// Unsets a mirror for the given URL.
3740
/// - Parameter originalOrMirrorURL: The original URL or the mirrored URL
3841
/// - Throws: `Error.mirrorNotFound` if no mirror exists for the provided URL.
3942
public func unset(originalOrMirrorURL: String) throws {
40-
if storage.keys.contains(originalOrMirrorURL) {
41-
storage[originalOrMirrorURL] = nil
42-
} else if let mirror = storage.first(where: { $0.value == originalOrMirrorURL }) {
43-
storage[mirror.key] = nil
43+
if let value = self.index[originalOrMirrorURL] {
44+
self.index[originalOrMirrorURL] = nil
45+
self.reverseIndex[value] = nil
46+
} else if let mirror = self.index.first(where: { $0.value == originalOrMirrorURL }) {
47+
self.index[mirror.key] = nil
48+
self.reverseIndex[originalOrMirrorURL] = nil
4449
} else {
4550
throw Error.mirrorNotFound
4651
}
@@ -49,36 +54,44 @@ public final class DependencyMirrors {
4954
/// Returns the mirrored URL for a package dependency URL.
5055
/// - Parameter url: The original URL
5156
/// - Returns: The mirrored URL, if one exists.
52-
public func getMirror(forURL url: String) -> String? {
53-
return storage[url]
57+
public func mirrorURL(for url: String) -> String? {
58+
return self.index[url]
5459
}
5560

5661
/// Returns the effective URL for a package dependency URL.
5762
/// - Parameter url: The original URL
5863
/// - Returns: The mirrored URL if it exists, otherwise the original URL.
59-
public func effectiveURL(forURL url: String) -> String {
60-
return getMirror(forURL: url) ?? url
64+
public func effectiveURL(for url: String) -> String {
65+
return self.mirrorURL(for: url) ?? url
6166
}
67+
68+
/// Returns the original URL for a mirrored package dependency URL.
69+
/// - Parameter url: The mirror URL
70+
/// - Returns: The original URL, if one exists.
71+
public func originalURL(for url: String) -> String? {
72+
return self.reverseIndex[url]
73+
}
74+
6275
}
6376

6477
extension DependencyMirrors: Collection {
6578
public typealias Index = Dictionary<String, String>.Index
6679
public typealias Element = String
6780

6881
public var startIndex: Index {
69-
storage.startIndex
82+
self.index.startIndex
7083
}
7184

7285
public var endIndex: Index {
73-
storage.endIndex
86+
self.index.endIndex
7487
}
7588

7689
public subscript(index: Index) -> Element {
77-
storage[index].value
90+
self.index[index].value
7891
}
7992

8093
public func index(after index: Index) -> Index {
81-
storage.index(after: index)
94+
self.index.index(after: index)
8295
}
8396
}
8497

@@ -94,7 +107,7 @@ extension DependencyMirrors: JSONMappable, JSONSerializable {
94107
}
95108

96109
public func toJSON() -> JSON {
97-
let mirrors = storage.map { Mirror(original: $0.key, mirror: $0.value) }
110+
let mirrors = self.index.map { Mirror(original: $0.key, mirror: $0.value) }
98111
return .array(mirrors.sorted(by: { $0.original < $1.mirror }).map { $0.toJSON() })
99112
}
100113
}

‎Sources/PackageGraph/PinsStore.swift

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,12 @@ public final class PinsStore {
3838
static let schemaVersion: Int = 1
3939

4040
/// The path to the pins file.
41-
fileprivate let pinsFile: AbsolutePath
41+
private let pinsFile: AbsolutePath
4242

4343
/// The filesystem to manage the pin file on.
44-
fileprivate var fileSystem: FileSystem
44+
private var fileSystem: FileSystem
45+
46+
private let mirrors: DependencyMirrors
4547

4648
/// The pins map.
4749
public fileprivate(set) var pinsMap: PinsMap
@@ -58,15 +60,16 @@ public final class PinsStore {
5860
/// - Parameters:
5961
/// - pinsFile: Path to the pins file.
6062
/// - fileSystem: The filesystem to manage the pin file on.
61-
public init(pinsFile: AbsolutePath, fileSystem: FileSystem) throws {
63+
public init(pinsFile: AbsolutePath, fileSystem: FileSystem, mirrors: DependencyMirrors) throws {
6264
self.pinsFile = pinsFile
6365
self.fileSystem = fileSystem
6466
self.persistence = SimplePersistence(
6567
fileSystem: fileSystem,
6668
schemaVersion: PinsStore.schemaVersion,
6769
statePath: pinsFile,
6870
prettyPrint: true)
69-
pinsMap = [:]
71+
self.pinsMap = [:]
72+
self.mirrors = mirrors
7073
do {
7174
_ = try self.persistence.restoreState(self)
7275
} catch SimplePersistence.Error.restoreFailure(_, let error) {
@@ -85,7 +88,7 @@ public final class PinsStore {
8588
packageRef: PackageReference,
8689
state: CheckoutState
8790
) {
88-
pinsMap[packageRef.identity] = Pin(
91+
self.pinsMap[packageRef.identity] = Pin(
8992
packageRef: packageRef,
9093
state: state
9194
)
@@ -95,24 +98,24 @@ public final class PinsStore {
9598
///
9699
/// This will replace any previous pin with same package name.
97100
public func add(_ pin: Pin) {
98-
pinsMap[pin.packageRef.identity] = pin
101+
self.pinsMap[pin.packageRef.identity] = pin
99102
}
100103

101-
/// Unpin all of the currently pinnned dependencies.
104+
/// Unpin all of the currently pinned dependencies.
102105
///
103106
/// This method does not automatically write to state file.
104107
public func unpinAll() {
105108
// Reset the pins map.
106-
pinsMap = [:]
109+
self.pinsMap = [:]
107110
}
108111

109112
public func saveState() throws {
110-
if pinsMap.isEmpty {
113+
if self.pinsMap.isEmpty {
111114
// Remove the pins file if there are zero pins to save.
112115
//
113116
// This can happen if all dependencies are path-based or edited
114117
// dependencies.
115-
return try fileSystem.removeFileTree(pinsFile)
118+
return try self.fileSystem.removeFileTree(pinsFile)
116119
}
117120

118121
try self.persistence.saveState(self)
@@ -124,8 +127,15 @@ public final class PinsStore {
124127
extension PinsStore: JSONSerializable {
125128
/// Saves the current state of pins.
126129
public func toJSON() -> JSON {
130+
// rdar://52529014, rdar://52529011: pin file should store the original location but remap when loading
131+
let pinsWithOriginalLocations = self.pins.map { pin -> Pin in
132+
let url = self.mirrors.originalURL(for: pin.packageRef.location) ?? pin.packageRef.location
133+
let identity = PackageIdentity(url: url) // FIXME: pin store should also encode identity
134+
return Pin(packageRef: .remote(identity: identity, location: url), state: pin.state)
135+
}
136+
127137
return JSON([
128-
"pins": pins.sorted(by: { $0.packageRef.identity < $1.packageRef.identity }).toJSON(),
138+
"pins": pinsWithOriginalLocations.sorted(by: { $0.packageRef.identity < $1.packageRef.identity }).toJSON(),
129139
])
130140
}
131141
}
@@ -144,9 +154,9 @@ extension PinsStore.Pin: JSONMappable, JSONSerializable {
144154
/// Convert the pin to JSON.
145155
public func toJSON() -> JSON {
146156
return .init([
147-
"package": packageRef.name.toJSON(),
148-
"repositoryURL": packageRef.location,
149-
"state": state
157+
"package": self.packageRef.name.toJSON(),
158+
"repositoryURL": self.packageRef.location,
159+
"state": self.state
150160
])
151161
}
152162
}
@@ -155,6 +165,13 @@ extension PinsStore.Pin: JSONMappable, JSONSerializable {
155165

156166
extension PinsStore: SimplePersistanceProtocol {
157167
public func restore(from json: JSON) throws {
158-
self.pinsMap = try Dictionary(json.get("pins").map({ ($0.packageRef.identity, $0) }), uniquingKeysWith: { first, _ in throw StringError("duplicated entry for package \"\(first.packageRef.name)\"") })
168+
let pins: [Pin] = try json.get("pins")
169+
// rdar://52529014, rdar://52529011: pin file should store the original location but remap when loading
170+
let pinsWithMirroredLocations = pins.map { pin -> Pin in
171+
let url = self.mirrors.effectiveURL(for: pin.packageRef.location)
172+
let identity = PackageIdentity(url: url) // FIXME: pin store should also encode identity
173+
return Pin(packageRef: .remote(identity: identity, location: url), state: pin.state)
174+
}
175+
self.pinsMap = try Dictionary(pinsWithMirroredLocations.map({ ($0.packageRef.identity, $0) }), uniquingKeysWith: { first, _ in throw StringError("duplicated entry for package \"\(first.packageRef.name)\"") })
159176
}
160177
}

‎Sources/SPMTestSupport/MockWorkspace.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ public final class MockWorkspace {
5353
self.archiver = archiver
5454
self.checksumAlgorithm = checksumAlgorithm
5555
self.config = try config ?? Workspace.Configuration(path: sandbox.appending(component: "swiftpm"), fs: fs)
56-
self.identityResolver = DefaultIdentityResolver(locationMapper: self.config.mirrors.effectiveURL(forURL:))
56+
self.identityResolver = DefaultIdentityResolver(locationMapper: self.config.mirrors.effectiveURL(for:))
5757
self.roots = roots
5858
self.packages = packages
5959

‎Sources/Workspace/Workspace.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,7 @@ public class Workspace {
307307
self.checkoutsPath = self.dataPath.appending(component: "checkouts")
308308
self.artifactsPath = self.dataPath.appending(component: "artifacts")
309309

310-
self.identityResolver = identityResolver ?? DefaultIdentityResolver(locationMapper: config.mirrors.effectiveURL(forURL:))
310+
self.identityResolver = identityResolver ?? DefaultIdentityResolver(locationMapper: config.mirrors.effectiveURL(for:))
311311

312312
self.containerProvider = RepositoryPackageContainerProvider(
313313
repositoryManager: repositoryManager,
@@ -319,7 +319,7 @@ public class Workspace {
319319
self.fileSystem = fileSystem
320320

321321
self.pinsStore = LoadableResult {
322-
try PinsStore(pinsFile: pinsFile, fileSystem: fileSystem)
322+
try PinsStore(pinsFile: pinsFile, fileSystem: fileSystem, mirrors: config.mirrors)
323323
}
324324
self.state = WorkspaceState(dataPath: dataPath, fileSystem: fileSystem)
325325
}
@@ -1120,7 +1120,7 @@ extension Workspace {
11201120
requiredIdentities = inputIdentities.union(requiredIdentities)
11211121

11221122
let availableIdentities: Set<PackageReference> = Set(manifestsMap.map {
1123-
let url = workspace.config.mirrors.effectiveURL(forURL: $0.1.packageLocation)
1123+
let url = workspace.config.mirrors.effectiveURL(for: $0.1.packageLocation)
11241124
return PackageReference(identity: $0.key, kind: $0.1.packageKind, location: url)
11251125
})
11261126
// We should never have loaded a manifest we don't need.

‎Tests/CommandsTests/PackageToolTests.swift

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -691,7 +691,7 @@ final class PackageToolTests: XCTestCase {
691691
// Try to pin bar at a branch.
692692
do {
693693
try execute("resolve", "bar", "--branch", "YOLO")
694-
let pinsStore = try PinsStore(pinsFile: pinsFile, fileSystem: localFileSystem)
694+
let pinsStore = try PinsStore(pinsFile: pinsFile, fileSystem: localFileSystem, mirrors: .init())
695695
let state = CheckoutState(revision: yoloRevision, branch: "YOLO")
696696
let identity = PackageIdentity(path: barPath)
697697
XCTAssertEqual(pinsStore.pinsMap[identity]?.state, state)
@@ -700,7 +700,7 @@ final class PackageToolTests: XCTestCase {
700700
// Try to pin bar at a revision.
701701
do {
702702
try execute("resolve", "bar", "--revision", yoloRevision.identifier)
703-
let pinsStore = try PinsStore(pinsFile: pinsFile, fileSystem: localFileSystem)
703+
let pinsStore = try PinsStore(pinsFile: pinsFile, fileSystem: localFileSystem, mirrors: .init())
704704
let state = CheckoutState(revision: yoloRevision)
705705
let identity = PackageIdentity(path: barPath)
706706
XCTAssertEqual(pinsStore.pinsMap[identity]?.state, state)
@@ -741,7 +741,7 @@ final class PackageToolTests: XCTestCase {
741741

742742
// Test pins file.
743743
do {
744-
let pinsStore = try PinsStore(pinsFile: pinsFile, fileSystem: localFileSystem)
744+
let pinsStore = try PinsStore(pinsFile: pinsFile, fileSystem: localFileSystem, mirrors: .init())
745745
XCTAssertEqual(pinsStore.pins.map{$0}.count, 2)
746746
for pkg in ["bar", "baz"] {
747747
let path = try SwiftPMProduct.packagePath(for: pkg, packageRoot: fooPath)
@@ -760,7 +760,7 @@ final class PackageToolTests: XCTestCase {
760760
// Try to pin bar.
761761
do {
762762
try execute("resolve", "bar")
763-
let pinsStore = try PinsStore(pinsFile: pinsFile, fileSystem: localFileSystem)
763+
let pinsStore = try PinsStore(pinsFile: pinsFile, fileSystem: localFileSystem, mirrors: .init())
764764
let identity = PackageIdentity(path: barPath)
765765
XCTAssertEqual(pinsStore.pinsMap[identity]?.state.version, "1.2.3")
766766
}
@@ -784,7 +784,7 @@ final class PackageToolTests: XCTestCase {
784784
// We should be able to revert to a older version.
785785
do {
786786
try execute("resolve", "bar", "--version", "1.2.3")
787-
let pinsStore = try PinsStore(pinsFile: pinsFile, fileSystem: localFileSystem)
787+
let pinsStore = try PinsStore(pinsFile: pinsFile, fileSystem: localFileSystem, mirrors: .init())
788788
let identity = PackageIdentity(path: barPath)
789789
XCTAssertEqual(pinsStore.pinsMap[identity]?.state.version, "1.2.3")
790790
try checkBar(5)

‎Tests/FunctionalTests/DependencyResolutionTests.swift

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,10 +123,11 @@ class DependencyResolutionTests: XCTestCase {
123123
XCTAssertTrue(output.stdout.contains("barmirror<\(prefix.pathString)/BarMirror@unspecified"), output.stdout)
124124
XCTAssertFalse(output.stdout.contains("bar<\(prefix.pathString)/Bar@unspecified"), output.stdout)
125125

126+
// rdar://52529014 mirrors should not be reflected in pins file
126127
let pins = try String(bytes: localFileSystem.readFileContents(appPinsPath).contents, encoding: .utf8)!
127128
XCTAssertTrue(pins.contains("\"\(prefix.pathString)/Foo\""), pins)
128-
XCTAssertTrue(pins.contains("\"\(prefix.pathString)/BarMirror\""), pins)
129-
XCTAssertFalse(pins.contains("\"\(prefix.pathString)/Bar\""), pins)
129+
XCTAssertTrue(pins.contains("\"\(prefix.pathString)/Bar\""), pins)
130+
XCTAssertFalse(pins.contains("\"\(prefix.pathString)/BarMirror\""), pins)
130131

131132
XCTAssertBuilds(appPath)
132133
}

‎Tests/PackageGraphTests/PackageGraphTests.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1275,8 +1275,8 @@ class PackageGraphTests: XCTestCase {
12751275
""")
12761276

12771277
let fs = InMemoryFileSystem(emptyFiles: [])
1278-
let store = try PinsStore(pinsFile: AbsolutePath("/pins"), fileSystem: fs)
1279-
XCTAssertThrows(StringError("duplicated entry for package \"Yams\""), { try store.restore(from: json) })
1278+
let store = try PinsStore(pinsFile: AbsolutePath("/pins"), fileSystem: fs, mirrors: .init())
1279+
XCTAssertThrows(StringError("duplicated entry for package \"yams\""), { try store.restore(from: json) })
12801280
}
12811281

12821282
func testTargetDependencies_Pre52() throws {

‎Tests/PackageGraphTests/PubgrubTests.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2201,7 +2201,7 @@ class DependencyGraphBuilder {
22012201
/// Creates a pins store with the given pins.
22022202
func create(pinsStore pins: [String: (CheckoutState, ProductFilter)]) -> PinsStore {
22032203
let fs = InMemoryFileSystem()
2204-
let store = try! PinsStore(pinsFile: AbsolutePath("/tmp/Package.resolved"), fileSystem: fs)
2204+
let store = try! PinsStore(pinsFile: AbsolutePath("/tmp/Package.resolved"), fileSystem: fs, mirrors: .init())
22052205

22062206
for (package, pin) in pins {
22072207
store.pin(packageRef: reference(for: package), state: pin.0)

0 commit comments

Comments
 (0)