Actually skip not needed dependencies during dependency resolution #3166
Conversation
neonichu
requested review from
abertelrud,
aciidgh and
friedbunny
as code owners
|
@swift-ci please smoke test |
Sources/PackageModel/Manifest.swift
Outdated
|
|
||
| let dependenciesByURL = Dictionary(dependencies.map({ ($0.url, $0) }), uniquingKeysWith: { $1 }) | ||
| let requiredDependencies = requiredDependencyURLs.compactMap({ dependenciesByURL[$0] }) | ||
| return requiredDependencies |
There was a problem hiding this comment.
It looks as though the dependencies will still be returned in arbitrary order. Should they be sorted by URL as they're converted from Set to Array?
There was a problem hiding this comment.
You're right, I wonder how this used to work? I actually went back to the old implementation here (just swapping name for URL).
There was a problem hiding this comment.
In any case, I think we can just return self.dependencies (which is already sorted as desired) filtered by requiredDependencyURLs.
neonichu
force-pushed
the
semi-target-based-dependencies-2
branch
from
58d25eb to
b2f40e2
Compare
|
@swift-ci please smoke test |
|
could we add a test for this? |
|
Is there any chance this could be cherry-picked for |
|
@MaxDesiatov yep, the intent is for this to be in 5.4 since this really should have been part of #3162 |
|
@tomerd I think we probably already have a test for this, but it is disabled/skipped as part of |
|
@swift-ci please smoke test |
|
I re-enabled the one test that is relevant for this, but it might make sense to resurrect a few more tests in a follow-up PR. When I initially introduced |
|
@tomerd looks like one of the new tests is potentially flaky? 🤔
|
|
@swift-ci please smoke test |
@neonichu actually I think the change in this PR simply changes the resolution semantics leading to a different failure when names are not specified. this way it behaves in this PR is consistent with how it behaves in 5.3 so we may just need to amend the test |
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
force-pushed
the
semi-target-based-dependencies-2
branch
from
a01d30c to
9d0601a
Compare
|
@swift-ci please smoke test |
…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)
…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)
This is a follow-up to #3162. While that brought back the code for
dependenciesRequired(for:), the guard based onproductFilteractually 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. SincedependenciesRequired(for: keepUnused:)is now only used ifENABLE_TARGET_BASED_DEPENDENCY_RESOLUTIONis set, I removed some dead code from there as well.