Skip to content

Restore Target‐Based Resolution #3006

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

Jump to bottom
Merged
merged 7 commits into from
Nov 10, 2020
Merged

Restore Target‐Based Resolution #3006

merged 7 commits into from
Nov 10, 2020

Conversation

SDGGiesbrecht
Copy link
Contributor

Continued from #2998, which temporarily disabled them.

@neonichu

@SDGGiesbrecht SDGGiesbrecht mentioned this pull request Oct 30, 2020
Merged
@SDGGiesbrecht
Copy link
Contributor Author

Note that this fixes the example provided to the public over in #2998, but I have no idea if all the private radar issues mentioned are due to the same bug, or if there are others that need fixing too. Someone on the inside will have to verify that.

@tomerd
Copy link
Contributor

tomerd commented Oct 30, 2020

@swift-ci please test

@abertelrud
Copy link
Contributor

@swift-ci please smoke test

@abertelrud
Copy link
Contributor

abertelrud commented Oct 30, 2020 •
edited
Loading

Thanks, @SDGGiesbrecht! I will do some additional testing on this.

@abertelrud
Copy link
Contributor

The CI issues look unrelated. Trying again.

@abertelrud
Copy link
Contributor

@swift-ci please smoke test

@abertelrud
Copy link
Contributor

The test error is related to the ToolSupportCore change:

        try testWithTemporaryDirectory { tmpdir in
            ^~~~~~~~~~~~~~~~~~~~~~~~~~
error: fatalError

@abertelrud
Copy link
Contributor

I think just rerunning the test with latest toolsupportcore should address that. Restarting.

@abertelrud
Copy link
Contributor

@swift-ci please test

@abertelrud
Copy link
Contributor

@swift-ci please smoke test

1 similar comment
@tomerd
Copy link
Contributor

tomerd commented Oct 31, 2020

@swift-ci please smoke test

@tomerd
Copy link
Contributor

tomerd commented Oct 31, 2020

@swift-ci please test

abertelrud
abertelrud reviewed Nov 2, 2020
View reviewed changes
Sources/Workspace/Workspace.swift
let name: String
let filter: ProductFilter
}
let allManifestsWithPossibleDuplicates = try! topologicalSort(inputManifests.map({ KeyedPair(($0, ProductFilter.everything), key: NameAndFilter(name: $0.name, filter: .everything)) })) { node in
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit surprised that topologicalSort() would return duplicates. That might be something to look into as well (not necessarily as part of this PR). I might be misreading what the underlying code does here, but is the change in this PR a workaround of something that should not have returned duplicates in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

topologicalSort() is working just fine. The duplicates here are because each node in the sort is a package–filter pair. (PackageA using ProductB ≠ PackageA using ProductC) Later, after the topological sort, the map operation drops the filter, keeping only the raw manifest. This removes the difference between some nodes. (PackageA = PackageA) Operations that follow are preconditioned such that each manifest should occur only once in the array.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, got it. Thanks for clarifying it!

@neonichu
Copy link
Contributor

neonichu commented Nov 3, 2020

Thanks a lot for the PR, @SDGGiesbrecht, it looks good to me in principle. However, it doesn't fix some of the issues we have been seeing, I will try to distill those into examples we can share.

Would you mind removing the revert from this PR so we can merge this fix independently?

@SDGGiesbrecht
Copy link
Contributor Author

I reverted the reverts.

@abertelrud
Copy link
Contributor

@swift-ci please smoke test

@SDGGiesbrecht
Copy link
Contributor Author

7‐day ping.

Do I still need to do something with this, or is the wait due to Apple internals?

@federicobucchi
Copy link
Member

I think this just needs to be merged. Maybe a commit squashing to remove the "Resolve Conflicts" commits?

@neonichu neonichu merged commit 9f4d4ec into swiftlang:main Nov 10, 2020
@neonichu
Copy link
Contributor

Sorry, I just forgot to merge this after the smoke tests finished.

@SDGGiesbrecht SDGGiesbrecht deleted the sdg‐resolution branch November 10, 2020 22:28
@neonichu neonichu mentioned this pull request Nov 13, 2020
Closed
@SDGGiesbrecht SDGGiesbrecht mentioned this pull request Dec 17, 2020
Closed
federicobucchi pushed a commit to federicobucchi/swift-package-manager that referenced this pull request Jan 6, 2021
@SDGGiesbrecht @federicobucchi
Restore Target‐Based Resolution (swiftlang#3006)
c187d86 
* Reverted 27f444f (leaving conflicts marked).

* Resolved conflicts.

* Added failing test.

* Fixed cache.

* Patched other potentential issue.

* Revert "Resolved conflicts."

This reverts commit bcf41e7.

* Revert "Reverted 27f444f (leaving conflicts marked)."

This reverts commit 4a0abc0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abertelrud abertelrud abertelrud left review comments

@federicobucchi federicobucchi federicobucchi approved these changes

@neonichu neonichu neonichu approved these changes

@aciidgh aciidgh Awaiting requested review from aciidgh

@friedbunny friedbunny Awaiting requested review from friedbunny

Assignees

@neonichu neonichu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SDGGiesbrecht @tomerd @abertelrud @neonichu @federicobucchi