Skip to content

Fix force resolved versions for transitive local dependencies #3691

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
Aug 27, 2021
Merged

Fix force resolved versions for transitive local dependencies #3691

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
24 changes: 10 additions & 14 deletions Sources/Workspace/Workspace.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1425,7 +1425,8 @@ extension Workspace {
// FIXME: this logic should be changed to use identity instead of location once identity is unique
public func loadDependencyManifests(
root: PackageGraphRoot,
diagnostics: DiagnosticsEngine
diagnostics: DiagnosticsEngine,
automaticallyAddManagedDependencies: Bool = false
) throws -> DependencyManifests {
// Utility Just because a raw tuple cannot be hashable.
struct Key: Hashable {
Expand Down Expand Up @@ -1476,6 +1477,13 @@ extension Workspace {
let allManifestsWithPossibleDuplicates = try topologicalSort(inputPairs) { pair in
// optimization: preload manifest we know about in parallel
let dependenciesRequired = pair.item.dependenciesRequired(for: pair.key.productFilter)
// prepopulate managed dependencies if we are asked to do so
if automaticallyAddManagedDependencies {
dependenciesRequired.filter { $0.isLocal }.forEach { dependency in
state.dependencies.add(ManagedDependency.local(packageRef: dependency.createPackageRef()))
}
diagnostics.wrap { try state.saveState() }
}
let dependenciesRequiredURLs = dependenciesRequired.map{ $0.location }.filter { !loadedManifests.keys.contains($0) }
// note: loadManifest emits diagnostics in case it fails
let dependenciesRequiredManifests = try temp_await { self.loadManifests(forURLs: dependenciesRequiredURLs, diagnostics: diagnostics, completion: $0) }
Expand Down Expand Up @@ -2013,19 +2021,7 @@ extension Workspace {
}
}

// Save state for local packages, if any.
//
// FIXME: This will only work for top-level local packages right now.
for rootManifest in rootManifests.values {
Copy link
Contributor

Choose a reason for hiding this comment

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

So the reason this can be removed is that the dependenciesRequired above always already contain all the root manifests?

Copy link
Contributor Author

@neonichu neonichu Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, that's right, we are already processing the direct dependencies of the roots there, no need to do it twice. The roots themselves do not end up in managed dependencies.

let dependencies = rootManifest.dependencies.filter{ $0.isLocal }
for localPackage in dependencies {
let package = localPackage.createPackageRef()
state.dependencies.add(ManagedDependency.local(packageRef: package))
}
}
diagnostics.wrap { try state.saveState() }

let currentManifests = try self.loadDependencyManifests(root: graphRoot, diagnostics: diagnostics)
let currentManifests = try self.loadDependencyManifests(root: graphRoot, diagnostics: diagnostics, automaticallyAddManagedDependencies: true)

let precomputationResult = try precomputeResolution(
root: graphRoot,
Expand Down