Skip to content

remove resolution retry on resolution failure #3853

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
Nov 18, 2021
Merged

remove resolution retry on resolution failure #3853

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
4 changes: 2 additions & 2 deletions Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1394,14 +1394,14 @@ private final class ContainerProvider {
/// Get the container for the given identifier, loading it if necessary.
func getContainer(for package: PackageReference, completion: @escaping (Result<PubGrubPackageContainer, Error>) -> Void) {
// Return the cached container, if available.
if let container = self.containersCache[package] {
if let container = self.containersCache[package], package.equalsIncludingLocation(container.package) {
return completion(.success(container))
}

if let prefetchSync = self.prefetches[package] {
// If this container is already being prefetched, wait for that to complete
prefetchSync.notify(queue: .sharedConcurrent) {
if let container = self.containersCache[package] {
if let container = self.containersCache[package], package.equalsIncludingLocation(container.package) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abertelrud @neonichu fixed the logic so now the test passes, its actually the right thing to do anyways

// should be in the cache once prefetch completed
return completion(.success(container))
} else {
Expand Down
2 changes: 1 addition & 1 deletion Sources/Workspace/ManagedDependency.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ extension Workspace {
/// - unmanagedPath: A custom absolute path instead of the subpath.
public func edited(subpath: RelativePath, unmanagedPath: AbsolutePath?) throws -> ManagedDependency {
guard case .sourceControlCheckout = self.state else {
throw InternalError("invalid depenedency state: \(self.state)")
throw InternalError("invalid dependency state: \(self.state)")
}
return ManagedDependency(
packageRef: self.packageRef,
Expand Down
51 changes: 5 additions & 46 deletions Sources/Workspace/Workspace.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2400,8 +2400,6 @@ extension Workspace {
explicitProduct: String? = nil,
forceResolution: Bool,
constraints: [PackageContainerConstraint],
retryOnPackagePathMismatch: Bool = true,
resetPinsStoreOnFailure: Bool = true,
observabilityScope: ObservabilityScope
) throws -> DependencyManifests {
// Ensure the cache path exists and validate that edited dependencies.
Expand Down Expand Up @@ -2488,51 +2486,12 @@ extension Workspace {

// Update the pinsStore.
let updatedDependencyManifests = try self.loadDependencyManifests(root: graphRoot, observabilityScope: observabilityScope)

// If we still have required URLs, we probably cloned a wrong URL for
// some package dependency.
//
// This would usually happen when we're resolving from scratch and the
// resolved file has an outdated entry for a transitive dependency whose
// URL was changed. For e.g., the resolved file could refer to a dependency
// through a ssh url but its new reference is now changed to http.
// If we still have missing packages, something is fundamentally wrong with the resolution of the graph
let stillMissingPackages = updatedDependencyManifests.computePackages().missing
if !stillMissingPackages.isEmpty {
if retryOnPackagePathMismatch {
// Retry resolution which will most likely resolve correctly now since
// we have the manifest files of all the dependencies.
return try self.resolve(
root: root,
explicitProduct: explicitProduct,
forceResolution: forceResolution,
constraints: constraints,
retryOnPackagePathMismatch: false,
resetPinsStoreOnFailure: resetPinsStoreOnFailure,
observabilityScope: observabilityScope
)
} else if resetPinsStoreOnFailure, !pinsStore.pinsMap.isEmpty {
// If we weren't able to resolve properly even after a retry, it
// could mean that the dependency at fault has a different
// version of the manifest file which contains dependencies that
// have also changed their package references.
pinsStore.unpinAll()
try pinsStore.saveState(toolsVersion: rootManifestsMinimumToolsVersion)
// try again with pins reset
return try self.resolve(
root: root,
explicitProduct: explicitProduct,
forceResolution: forceResolution,
constraints: constraints,
retryOnPackagePathMismatch: false,
resetPinsStoreOnFailure: false,
observabilityScope: observabilityScope
)
} else {
// give up
let missing = stillMissingPackages.map{ $0.description }
observabilityScope.emit(error: "exhausted attempts to resolve the dependencies graph, with '\(missing.joined(separator: "', '"))' unresolved.")
return updatedDependencyManifests
}
guard stillMissingPackages.isEmpty else {
let missing = stillMissingPackages.map{ $0.description }
observabilityScope.emit(error: "exhausted attempts to resolve the dependencies graph, with '\(missing.joined(separator: "', '"))' unresolved.")
return updatedDependencyManifests
}

self.pinAll(
Expand Down
147 changes: 115 additions & 32 deletions Tests/WorkspaceTests/WorkspaceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3237,7 +3237,10 @@ final class WorkspaceTests: XCTestCase {
}
}

func testDependencySwitchWithSameIdentity() throws {

// Test that switching between two same local packages placed at
// different locations works correctly.
func testDependencySwitchLocalWithSameIdentity() throws {
let sandbox = AbsolutePath("/tmp/ws/")
let fs = InMemoryFileSystem()

Expand Down Expand Up @@ -3279,9 +3282,6 @@ final class WorkspaceTests: XCTestCase {
]
)

// Test that switching between two same local packages placed at
// different locations works correctly.

var deps: [MockDependency] = [
.fileSystem(path: "./Foo", products: .specific(["Foo"])),
]
Expand Down Expand Up @@ -3315,6 +3315,84 @@ final class WorkspaceTests: XCTestCase {
}
}

// Test that switching between two remote packages at
// different locations works correctly.
func testDependencySwitchRemoteWithSameIdentity() throws {
let sandbox = AbsolutePath("/tmp/ws/")
let fs = InMemoryFileSystem()

let workspace = try MockWorkspace(
sandbox: sandbox,
fs: fs,
roots: [
MockPackage(
name: "Root",
targets: [
MockTarget(name: "Root", dependencies: []),
],
products: [],
dependencies: []
),
],
packages: [
MockPackage(
name: "Foo",
url: "https://scm.com/org/foo",
targets: [
MockTarget(name: "Foo"),
],
products: [
MockProduct(name: "Foo", targets: ["Foo"]),
],
versions: ["1.0.0"]
),
MockPackage(
name: "Foo",
url: "https://scm.com/other/foo",
targets: [
MockTarget(name: "OtherFoo"),
],
products: [
MockProduct(name: "OtherFoo", targets: ["OtherFoo"]),
],
versions: ["1.1.0"]
),
]
)

var deps: [MockDependency] = [
.sourceControl(url: "https://scm.com/org/foo", requirement: .exact("1.0.0")),
]
try workspace.checkPackageGraph(roots: ["Root"], deps: deps) { graph, diagnostics in
XCTAssertNoDiagnostics(diagnostics)
PackageGraphTester(graph) { result in
result.check(roots: "Root")
result.check(packages: "Foo", "Root")
}
}
workspace.checkManagedDependencies { result in
result.check(dependency: "foo", at: .checkout(.version("1.0.0")))
}
do {
let ws = try workspace.getOrCreateWorkspace()
XCTAssertEqual(ws.state.dependencies[.plain("foo")]?.packageRef.locationString, "https://scm.com/org/foo")
}

deps = [
.sourceControl(url: "https://scm.com/other/foo", requirement: .exact("1.1.0"))
]
try workspace.checkPackageGraph(roots: ["Root"], deps: deps) { _, diagnostics in
XCTAssertNoDiagnostics(diagnostics)
}
workspace.checkManagedDependencies { result in
result.check(dependency: "foo", at: .checkout(.version("1.1.0")))
}
do {
let ws = try workspace.getOrCreateWorkspace()
XCTAssertEqual(ws.state.dependencies[.plain("foo")]?.packageRef.locationString, "https://scm.com/other/foo")
}
}

func testResolvedFileUpdate() throws {
let sandbox = AbsolutePath("/tmp/ws/")
let fs = InMemoryFileSystem()
Expand Down Expand Up @@ -3525,6 +3603,9 @@ final class WorkspaceTests: XCTestCase {
}
}

// In this test, we get into a state where an entry in the resolved
// file for a transitive dependency whose URL is later changed to
// something else, while keeping the same package identity.
func testTransitiveDependencySwitchWithSameIdentity() throws {
let sandbox = AbsolutePath("/tmp/ws/")
let fs = InMemoryFileSystem()
Expand All @@ -3536,44 +3617,59 @@ final class WorkspaceTests: XCTestCase {
MockPackage(
name: "Root",
targets: [
MockTarget(name: "Root", dependencies: ["Bar"]),
MockTarget(
name: "Root",
dependencies: [
.product(name: "Bar", package: "bar")
]),
],
products: [],
dependencies: [
.sourceControl(path: "./Bar", requirement: .upToNextMajor(from: "1.0.0")),
.sourceControl(url: "https://scm.com/org/bar", requirement: .upToNextMajor(from: "1.0.0")),
]
),
],
packages: [
MockPackage(
name: "Bar",
url: "https://scm.com/org/bar",
targets: [
MockTarget(name: "Bar", dependencies: ["Foo"]),
MockTarget(
name: "Bar",
dependencies: [
.product(name: "Foo", package: "foo")
]),
],
products: [
MockProduct(name: "Bar", targets: ["Bar"]),
],
dependencies: [
.sourceControl(path: "./Foo", requirement: .upToNextMajor(from: "1.0.0")),
.sourceControl(url: "https://scm.com/org/foo", requirement: .upToNextMajor(from: "1.0.0")),
],
versions: ["1.0.0"]
),
MockPackage(
name: "Bar",
url: "https://scm.com/org/bar",
targets: [
MockTarget(name: "Bar", dependencies: ["Nested/Foo"]),
MockTarget(
name: "Bar",
dependencies: [
.product(name: "OtherFoo", package: "foo")
]),
],
products: [
MockProduct(name: "Bar", targets: ["Bar"]),
],
dependencies: [
.sourceControl(path: "Nested/Foo", requirement: .upToNextMajor(from: "1.0.0")),
.sourceControl(url: "https://scm.com/other/foo", requirement: .upToNextMajor(from: "1.0.0")),
],
versions: ["1.1.0"],
toolsVersion: .v5
),
MockPackage(
name: "Foo",
url: "https://scm.com/org/foo",
targets: [
MockTarget(name: "Foo"),
],
Expand All @@ -3584,40 +3680,27 @@ final class WorkspaceTests: XCTestCase {
),
MockPackage(
name: "Foo",
path: "Nested/Foo",
url: "https://scm.com/other/foo",
targets: [
MockTarget(name: "Foo"),
MockTarget(name: "OtherFoo"),
],
products: [
MockProduct(name: "Nested/Foo", targets: ["Foo"]),
MockProduct(name: "OtherFoo", targets: ["OtherFoo"]),
],
versions: ["1.0.0"]
),
]
)

// In this test, we get into a state where add an entry in the resolved
// file for a transitive dependency whose URL is later changed to
// something else, while keeping the same package identity.
//
// This is normally detected during pins validation before the
// dependency resolution process even begins but if we're starting with
// a clean slate, we don't even know about the correct urls of the
// transitive dependencies. We will end up fetching the wrong
// dependency as we prefetch the pins. If we get into this case, it
// should kick off another dependency resolution operation which will
// have enough information to remove the invalid pins of transitive
// dependencies.

var deps: [MockDependency] = [
.sourceControl(path: "./Bar", requirement: .exact("1.0.0"), products: .specific(["Bar"])),
.sourceControl(url: "https://scm.com/org/bar", requirement: .exact("1.0.0")),
]
try workspace.checkPackageGraph(roots: ["Root"], deps: deps) { graph, diagnostics in
XCTAssertNoDiagnostics(diagnostics)
PackageGraphTester(graph) { result in
result.check(roots: "Root")
result.check(packages: "Bar", "Foo", "Root")
}
XCTAssertNoDiagnostics(diagnostics)
}
workspace.checkManagedDependencies { result in
result.check(dependency: "foo", at: .checkout(.version("1.0.0")))
Expand All @@ -3626,22 +3709,22 @@ final class WorkspaceTests: XCTestCase {

do {
let ws = try workspace.getOrCreateWorkspace()
XCTAssertEqual(ws.state.dependencies[.plain("foo")]?.packageRef.locationString, "/tmp/ws/pkgs/Foo")
XCTAssertEqual(ws.state.dependencies[.plain("foo")]?.packageRef.locationString, "https://scm.com/org/foo")
}

workspace.checkReset { diagnostics in
XCTAssertNoDiagnostics(diagnostics)
}

deps = [
.sourceControl(path: "./Bar", requirement: .exact("1.1.0"), products: .specific(["Bar"])),
.sourceControl(url: "https://scm.com/org/bar", requirement: .exact("1.1.0")),
]
try workspace.checkPackageGraph(roots: ["Root"], deps: deps) { graph, diagnostics in
XCTAssertNoDiagnostics(diagnostics)
PackageGraphTester(graph) { result in
result.check(roots: "Root")
result.check(packages: "Bar", "Foo", "Root")
}
XCTAssertNoDiagnostics(diagnostics)
}
workspace.checkManagedDependencies { result in
result.check(dependency: "foo", at: .checkout(.version("1.0.0")))
Expand All @@ -3650,7 +3733,7 @@ final class WorkspaceTests: XCTestCase {

do {
let ws = try workspace.getOrCreateWorkspace()
XCTAssertEqual(ws.state.dependencies[.plain("foo")]?.packageRef.locationString, "/tmp/ws/pkgs/Nested/Foo")
XCTAssertEqual(ws.state.dependencies[.plain("foo")]?.packageRef.locationString, "https://scm.com/other/foo")
}
}

Expand Down