Skip to content

Do not store products in resolved file #2792

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 30, 2020
Merged

Do not store products in resolved file #2792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 5 additions & 14 deletions Sources/PackageGraph/PinsStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,12 @@ public final class PinsStore {
/// The pinned state.
public let state: CheckoutState

/// The product filter applied by the pin.
public let productFilter: ProductFilter

public init(
packageRef: PackageReference,
state: CheckoutState,
productFilter: ProductFilter
state: CheckoutState
) {
self.packageRef = packageRef
self.state = state
self.productFilter = productFilter
}
}

Expand Down Expand Up @@ -88,13 +83,11 @@ public final class PinsStore {
/// - state: The state to pin at.
public func pin(
packageRef: PackageReference,
state: CheckoutState,
productFilter: ProductFilter
state: CheckoutState
) {
pinsMap[packageRef.identity] = Pin(
packageRef: packageRef,
state: state,
productFilter: productFilter
state: state
)
}

Expand All @@ -117,7 +110,7 @@ public final class PinsStore {
public func createConstraints() -> [RepositoryPackageConstraint] {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove this method, it's dead code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, I'll make a follow up to clean this up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2793

return pins.map({ pin in
return RepositoryPackageConstraint(
container: pin.packageRef, requirement: pin.state.requirement(), products: pin.productFilter)
container: pin.packageRef, requirement: pin.state.requirement(), products: .everything)
Copy link
Contributor

Choose a reason for hiding this comment

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

Won’t this constraint mean that every pin could potentially want more of its transitive dependencies after it’s loaded than when it was pinned?

If a package is resolved was at 1.0.0 for use of product B, then it will be pinned at 1.0.0 with no product information (which ought to be somehow inferable). Upon loading the pins, it will be loaded at 1.0.0 for use of products A and B, and the resolver will go hunting for all of product A’s dependencies. Or is that not what happens?

Passing .specific([]) might work instead, because I think the resolver will still get the remaining product‐based constraints from the manifests anyway.

I will try both variants and get back to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These constraints are no longer actually being used by the resolver, I removed this method in #2793

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I see that now. It makes things much simpler.

})
}
}
Expand Down Expand Up @@ -159,16 +152,14 @@ extension PinsStore.Pin: JSONMappable, JSONSerializable, Equatable {
let ref = PackageReference(identity: identity, path: url)
self.packageRef = name.flatMap(ref.with(newName:)) ?? ref
self.state = try json.get("state")
self.productFilter = try json.get("products")
}

/// Convert the pin to JSON.
public func toJSON() -> JSON {
return .init([
"package": packageRef.name.toJSON(),
"repositoryURL": packageRef.path,
"state": state,
"products": productFilter,
"state": state
])
}
}
2 changes: 1 addition & 1 deletion Sources/SPMTestSupport/TestWorkspace.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ public final class TestWorkspace {
let pinsStore = try workspace.pinsStore.load()

for (ref, state) in pins {
pinsStore.pin(packageRef: ref, state: state.version, productFilter: state.products)
pinsStore.pin(packageRef: ref, state: state.version)
}

for dependency in managedDependencies {
Expand Down
2 changes: 1 addition & 1 deletion Sources/Workspace/Workspace.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1541,7 +1541,7 @@ extension Workspace {
// Clone the required pins.
for pin in requiredPins {
diagnostics.wrap {
_ = try self.clone(package: pin.packageRef, at: pin.state, productFilter: pin.productFilter)
_ = try self.clone(package: pin.packageRef, at: pin.state, productFilter: .everything)
}
}

Expand Down
3 changes: 1 addition & 2 deletions Sources/Workspace/misc.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ extension PinsStore {

self.pin(
packageRef: dependency.packageRef,
state: checkoutState,
productFilter: dependency.productFilter)
state: checkoutState)
}
}

Expand Down
2 changes: 1 addition & 1 deletion Tests/PackageGraphTests/PubgrubTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2120,7 +2120,7 @@ class DependencyGraphBuilder {
let store = try! PinsStore(pinsFile: AbsolutePath("/tmp/Package.resolved"), fileSystem: fs)

for (package, pin) in pins {
store.pin(packageRef: reference(for: package), state: pin.0, productFilter: pin.1)
store.pin(packageRef: reference(for: package), state: pin.0)
}

try! store.saveState()
Expand Down
18 changes: 8 additions & 10 deletions Tests/WorkspaceTests/PinsStoreTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ final class PinsStoreTests: XCTestCase {

let state = CheckoutState(revision: revision, version: v1)
let products: ProductFilter = .everything
let pin = PinsStore.Pin(packageRef: fooRef, state: state, productFilter: products)
let pin = PinsStore.Pin(packageRef: fooRef, state: state)
// We should be able to round trip from JSON.
XCTAssertEqual(try PinsStore.Pin(json: pin.toJSON()), pin)

Expand All @@ -44,7 +44,7 @@ final class PinsStoreTests: XCTestCase {
XCTAssert(!fs.exists(pinsFile))
XCTAssert(store.pins.map{$0}.isEmpty)

store.pin(packageRef: fooRef, state: state, productFilter: products)
store.pin(packageRef: fooRef, state: state)
try store.saveState()

XCTAssert(fs.exists(pinsFile))
Expand All @@ -63,13 +63,12 @@ final class PinsStoreTests: XCTestCase {
}

// We should be able to pin again.
store.pin(packageRef: fooRef, state: state, productFilter: products)
store.pin(packageRef: fooRef, state: state)
store.pin(
packageRef: fooRef,
state: CheckoutState(revision: revision, version: "1.0.2"),
productFilter: .specific(["some", "products"])
state: CheckoutState(revision: revision, version: "1.0.2")
)
store.pin(packageRef: barRef, state: state, productFilter: products)
store.pin(packageRef: barRef, state: state)
try store.saveState()

store = try PinsStore(pinsFile: pinsFile, fileSystem: fs)
Expand All @@ -79,8 +78,7 @@ final class PinsStoreTests: XCTestCase {
do {
store.pin(
packageRef: barRef,
state: CheckoutState(revision: revision, branch: "develop"),
productFilter: .specific(["a", "product"])
state: CheckoutState(revision: revision, branch: "develop")
)
try store.saveState()
store = try PinsStore(pinsFile: pinsFile, fileSystem: fs)
Expand All @@ -94,7 +92,7 @@ final class PinsStoreTests: XCTestCase {

// Test revision pin.
do {
store.pin(packageRef: barRef, state: CheckoutState(revision: revision), productFilter: .specific(["other", "product"]))
store.pin(packageRef: barRef, state: CheckoutState(revision: revision))
try store.saveState()
store = try PinsStore(pinsFile: pinsFile, fileSystem: fs)

Expand Down Expand Up @@ -156,7 +154,7 @@ final class PinsStoreTests: XCTestCase {

let fooRef = PackageReference(identity: "foo", path: "/foo")
let revision = Revision(identifier: "81513c8fd220cf1ed1452b98060cd80d3725c5b7")
store.pin(packageRef: fooRef, state: CheckoutState(revision: revision, version: v1), productFilter: .specific([]))
store.pin(packageRef: fooRef, state: CheckoutState(revision: revision, version: v1))

XCTAssert(!fs.exists(pinsFile))

Expand Down
2 changes: 1 addition & 1 deletion Tests/WorkspaceTests/WorkspaceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3608,7 +3608,7 @@ final class WorkspaceTests: XCTestCase {
let revision = try fooRepo.resolveRevision(tag: "1.0.0")
let newState = CheckoutState(revision: revision, version: "1.0.0")

pinsStore.pin(packageRef: fooPin.packageRef, state: newState, productFilter: .specific(["Foo"]))
pinsStore.pin(packageRef: fooPin.packageRef, state: newState)
try pinsStore.saveState()
}

Expand Down