Reinstate 5.3 behavior of target-based dependency resolution #3162
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This brings back the 5.3 implementation of `dependenciesRequired(for:)` (slightly modified) and also makes it so that `targetsRequired(for:)` for non-root packages only includes targets required by any products. Together this brings the behaviour of dependency resolution to the state of swiftlang#2424.
neonichu
requested review from
abertelrud,
aciidgh and
friedbunny
as code owners
|
@swift-ci please smoke test |
abertelrud
approved these changes
Jan 6, 2021
tomerd
approved these changes
Jan 6, 2021
|
This PR Looks fine to me. It’s too bad the project that reproduces the error cannot be shared. Let me know when you’ve narrowed in on something that you can share. I’m still happy to help. |
|
thanks for all your work on this @SDGGiesbrecht, I believe @neonichu has spend great amount of time trying to nail down the issues and hopefully can share details soon. iiuc its mostly not in the original patch but how the resolver works |
|
Thanks @SDGGiesbrecht. I'm planning to create a set of comprehensive tests when we revisit this so that we can address these kind of issues in a more systematic manner instead of having to rely on ad-hoc testing with specific (internal) projects. Unfortunately, it's getting rather late in the release cycle so this seems to be the right approach for now. |
neonichu
added a commit
to neonichu/swift-package-manager
that referenced
this pull request
Jan 7, 2021
This is a follow-up to swiftlang#3162. While that brought back the code for `dependenciesRequired(for:)`, the guard based on `productFilter` actually made it so that it was practically never executed. Instead, the guard should be based on whether the package in question is a root package. Additionally, this made the order of the returned dependencies unstable, due to use of `Set`. Since `dependenciesRequired(for: keepUnused:)` is now only used if `ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION` is set, I removed some dead code from there as well.
neonichu
added a commit
to neonichu/swift-package-manager
that referenced
this pull request
Jan 7, 2021
This is a follow-up to swiftlang#3162. While that brought back the code for `dependenciesRequired(for:)`, the guard based on `productFilter` actually made it so that it was practically never executed. Instead, the guard should be based on whether the package in question is a root package. Additionally, this made the order of the returned dependencies unstable, due to use of `Set`. Since `dependenciesRequired(for: keepUnused:)` is now only used if `ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION` is set, I removed some dead code from there as well.
neonichu
added a commit
to neonichu/swift-package-manager
that referenced
this pull request
Jan 11, 2021
This is a follow-up to swiftlang#3162. While that brought back the code for `dependenciesRequired(for:)`, the guard based on `productFilter` actually made it so that it was practically never executed. Instead, the guard should be based on whether the package in question is a root package. Additionally, this made the order of the returned dependencies unstable, due to use of `Set`. Since `dependenciesRequired(for: keepUnused:)` is now only used if `ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION` is set, I removed some dead code from there as well.
neonichu
added a commit
that referenced
this pull request
Jan 11, 2021
…3166) Actually skip not needed dependencies during dependency resolution This is a follow-up to #3162. While that brought back the code for `dependenciesRequired(for:)`, the guard based on `productFilter` actually made it so that it was practically never executed. Instead, the guard should be based on whether the package in question is a root package. Additionally, this made the order of the returned dependencies unstable, due to use of `Set`. Since `dependenciesRequired(for: keepUnused:)` is now only used if `ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION` is set, I removed some dead code from there as well.
neonichu
added a commit
to neonichu/swift-package-manager
that referenced
this pull request
Jan 11, 2021
…wiftlang#3166) Actually skip not needed dependencies during dependency resolution This is a follow-up to swiftlang#3162. While that brought back the code for `dependenciesRequired(for:)`, the guard based on `productFilter` actually made it so that it was practically never executed. Instead, the guard should be based on whether the package in question is a root package. Additionally, this made the order of the returned dependencies unstable, due to use of `Set`. Since `dependenciesRequired(for: keepUnused:)` is now only used if `ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION` is set, I removed some dead code from there as well. (cherry picked from commit da4d643)
neonichu
added a commit
that referenced
this pull request
Jan 14, 2021
…3166) (#3185) Actually skip not needed dependencies during dependency resolution This is a follow-up to #3162. While that brought back the code for `dependenciesRequired(for:)`, the guard based on `productFilter` actually made it so that it was practically never executed. Instead, the guard should be based on whether the package in question is a root package. Additionally, this made the order of the returned dependencies unstable, due to use of `Set`. Since `dependenciesRequired(for: keepUnused:)` is now only used if `ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION` is set, I removed some dead code from there as well. (cherry picked from commit da4d643)
|
@neonichu, is there any chance this sort of identity issue might have been at the root of our troubles? I can imagine how such a mismatch could cleave the entire graph in two in a target‐based model, but still have appeared to work in the old model due to both halves of the graph ending up identical to each other. The redundancies would already have been wrong in the old model, but they would never have resulted in stuff being missing. (You don’t have to answer now. Just keep it in the back of your mind for when you do come back to this.) |
|
@neonichu Is there any timeline on when target based dependency resolution will be addressed? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Early in the 5.4 development process, we merged #2749 to bring a full implementation of target-based dependency resolution, but we disabled that later because of regressions with some real-world packages. The plan was to bring this back for the final 5.4, but after some discussion in #3121, we have identified a few issues without a clear fix that doesn't carry significant risk this late in the release cycle.
However, the current state is also not acceptable for the 5.4 release, since this regresses dependency resolution back to the state before #2424 which could break real world packages.
This PR brings back the 5.3 behavior of taking only into account targets that are required by any products and omitting any dependencies that aren't required by any products. We can then revisit target-based dependency resolution in a future release.
Modifications:
This brings back the 5.3 implementation of
dependenciesRequired(for:)(slightly modified) and also makes it so thattargetsRequired(for:)for non-root packages only includes targets required by any products. Together this brings the behaviour of dependency resolution to the state of #2424.Result:
Dependency resolution will behave the same as it did in SwiftPM 5.3.