Re-enable target-based dependency resolution #3121
Conversation
Looks like we were not necessarily fetching dependencies of packages with pre-5.2 tools versions during resolution. It seems correct to not consider the product filter during dependency resolution for pre-5.2 packages, since it won't be correct for them by definition. rdar://70633425
neonichu
requested review from
abertelrud,
aciidgh and
friedbunny
as code owners
|
@swift-ci please smoke test |
| @@ -222,6 +222,7 @@ public class RepositoryPackageContainer: PackageContainer, CustomStringConvertib | |||
| productFilter: ProductFilter | |||
| ) throws -> (Manifest, [Constraint]) { | |||
| let manifest = try self.loadManifest(at: revision, version: version) | |||
| let productFilter = manifest.toolsVersion < ToolsVersion.v5_2 ? ProductFilter.everything : productFilter | |||
There was a problem hiding this comment.
This is a bit of a blunt instrument, but it should yield correct results.
There was a problem hiding this comment.
This single line fixes the top secret errors that were reported?
I’m curious if the errors occur generally during fresh resolution, or if they only occur in the presence of a pins file?
I ask because the way I implemented the filtering, manifests < 5.2 asked for every dependency product from every package. Essentially that meant any package that might contain the intended dependency would be resolved. The intent was that if a 5.1 manifest depends on a package that later gets a minor version bump with a 5.2 manifest, that transitive 5.2 manifest knows which of its products were actually requested and can therefore meaningfully filter its own dependencies. What occurs to me know is that had a side effect of culling some dependencies in a way that is logically valid, but not stable compared to previous treatment of 5.2 manifests.
For example, consider the case of a 5.2 manifest which declares a slew of package dependencies, but its actual targets declare no dependencies whatsoever. Under a 5.1 toolchain, all those dependencies were resolved. Under the initial filtering algorithm, all those dependencies were ignored. Ignoring them is valid as far as the build process is concerned. However, because it produces different pins, it introduces an instability where old and new toolchains would each be insisting that the other’s pins are invalid.
Can you look into it to see if this seems to be the real problem, or at least if it is the deciding factor that channels execution into the problematic code branch. If so, then the correct solution is to change the handling of pre‐5.2 manifests to request absolutely everything absolutely all the time.
Such a fix would be done most cleanly here, right where it originates, by injecting something like this:
if toolsVersion < ToolsVersion.v5_2 {
return dependencies.map { $0.filtered(by: .everything) }
}From that simple change, everything else should cascade back into place such that manifests from 5.1 and earlier are always and everywhere treated exactly as they were by the 5.1 toolchain. It would mean some graph pruning opportunities would missed, but it would ensure pin stability. Those missed optimizations would only effect old manifests, which presumably will become less and less common over time anyway. (5.2 manifests and above would still be properly skipping everything that isn’t used.)
There was a problem hiding this comment.
The issue I have been seeing is that we can miss dependencies of 5.1 and earlier packages that are actually required, the scenario is the same package being required by multiple different packages transitively. I need to dig in a bit more, I put this up more as a straw man so that we have some solution to reactivate target-based dependency resolution that we can land in 5.4.
Where I am at right now, I'm seeing that we don't download a few dependencies as part of resolution which leads us to not being able to include them later on here. This piece of code in Workspace is kind of subtle, because unless we have previously fetched a given package, we'll just fail and ignore it. I'll report back once I have more.
There was a problem hiding this comment.
I need to dig in a bit more, I put this up more as a straw man so that we have some solution to reactivate target-based dependency resolution that we can land in 5.4.
Yes, that makes sense.
I still think moving the workaround to the Manifest type like I suggested would be more consistent. I don’t think RepositoryPackageContainer should alter the resolution strategy, as it won’t apply to other conformers of the PackageContainer protocol.
the scenario is the same package being required by multiple different packages transitively
That sounds like there is another place in need of a similar fix to this part of #3006. If a topologicalSort or similar graph traversal uses a node definition of just the package—instead of the package–filter pair—then it would think it had already visited that node and neglect to look at its dependencies even if the filter is different the second time the node shows up. The result would be exactly the symptoms you describe.
|
|
|
I think both of these tests simply need updating, they were partially disabled as part of disabling target-based dependency resolution and therefore weren't updated as part of the pubgrub diagnostics changes. |
|
@swift-ci please smoke test |
|
|
I can reproduce the failures locally, but I don't understand them at all:
It somewhat sounds as if we are switching derived data paths mid-build? I don't really understand how that could be caused by my changes. |
|
I can solve this particular issue by setting an explicit |
There was a problem hiding this comment.
(My tabs got in a fight with GitHub and I cannot tell if my last post attempt succeeded or not. I apologize if this results in a duplicate comment.)
See the inline comment.
I concur with that diagnosis.
I do not understand these other errors either. I think I will have time tomorrow to clone and take a closer look. |
Thanks for offering to help, but it seems like these issues are completely unrelated to target-based dependency resolution. Feel free to have a look anyway, of course :) |
|
@swift-ci please smoke test macos self hosted |
|
Based on your description, I attempted to reproduce it. I created a simple skeleton 4‐node diamond where all packages have a 5.1 manifest. When the top package has no
Does this sound like what you are seeing? I am attempting to debug it right now. |
|
Ignore my last comment. 🤦♂️ I had generated the legacy pins by running |
|
I'm seeing the same symptom, but it happens with or without a |
|
Yes, I believe you. It is just that as someone on the outside without access to the project that triggers it, I am left trying to debug a black box whose execution flow I cannot trace. I try to think through the possibilities and be as helpful as I can based on what I can glean from your comments, but I have run out of ideas for the moment.
If you think it would be helpful. Are there other, legitimate ways that could flag, or does this state unquestionably indicate a programmer error? |
|
Is it actually correct that we unconditionally return |
|
I think that line is correct. That method is below the level where the product filter is applied. The intent is that it simply answers the question, “In order to build target list x, which dependencies would(/might) I need?”
If an actually required dependency is landing in that third branch, then the first question is whether it is declared explicitly or not?
|
|
Here is another stab in the dark (ignore it if your insider information suggests it is way off): Was it originally possible to have a target and a dependency’s product share the same name, and then the by‐name target dependency successfully pointed at both at once? i.e. given... // ...
dependencies: [
.package(url: "somewhere.com/SameString", /* ... */)
// (↑ Has a “SameString” product with a “Dependency” target.)
],
targets: [
.target(name: "SameString"),
.target(name: "Target", dependencies: ["SameString"])
]
// ......was import Dependency // *
import SameStringIf that used to work, then the assumption underpinning this line is faulty. That is a plausible hypothesis for how a corner‐case dependency could end up dropped. The only other code paths that |
|
It appears that I was wrong about the concrete scenario and that is because it is actually non-deterministic which required packages we are dropping. That also explains why I have been having a hard time debugging this since I tried to zero in on a specific one of them, but that didn't work because of the non-determinism. One source of non-determinism in the resolution process that I noticed is here. Since each |
|
I concur. |
|
Something like the following would at least be deterministic (instead of {
(counts[$0]!, $0.node.package.name, $0.node.specificProduct ?? "")
< (counts[$1]!, $1.node.package.name, $1.node.specificProduct ?? "")
} |
|
This seems problematic, it means we will potentially skip creating incompatibilities for certain product filters and in conjunction with the non-determinism in picking which product filter comes "first", it seems non-deterministic which ones we are skipping. Changing this to be aware of product filters indeed helps, but I'm wondering if that's actually necessary. Presumably, we will avoid creating all these additional So possibly using |
|
Regarding the integration test failures, there were actually two separate issues. The module cache one I was seeing locally happens if there's a leftover The CI failure is actually the |
…y resolution As discussed in the original PR swiftlang#2749, we no longer support building arbitrary targets using `--target`, but this test was relying on it. We removed a similar test from the unit tests and should do the same with this one.
|
@swift-ci please smoke test |
Probably. I’ll take a closer look in a minute. When I did the implementation, I swapped in the new node type and made sure its I plan on re‐auditing PubGrub’s actual use of the nodes right away. |
Yep, that's true. What I was thinking was that in the case of 5.2 packages, we should end up with only one |
|
I did a project‐wide search for “DependencyResolutionNode” and inspected each function that referenced it in its signature. I made the following adjustments and just launched the test suite locally. Maybe you can try them on the actual problematic package while I wait for the result? Oops. In the process of pasting that I noticed that this part... for node in dep.nodes() {
if emittedIncompatibilities[node]?.contains(version) != true {
constraints.append(dep)
}
}...really ought to be... |
|
Nope. No dice. I broke something in the process. |
|
Okay, take two passed the test suite locally (aside from two diagnostics whose order was reversed): |
|
My brain is really having trouble parsing this line: if dep.nodes().contains(where: { emittedIncompatibilities[$0]?.contains(version) != true }) {There are too many logical negatives in the original condition and I find myself unconfident whether the new one needs to be |
|
This is very similar to what I came up with, but it doesn't really work for my reproducer if there's no pre-existing resolved file. It'll trigger the "Two products in one package resolved to different versions" assertion. Could/should we possibly change |
Are you speaking in terms of the first diff or the second one? For the first, that is the very error triggered by the test suite when I ran it. The second diff made it go away. (They are separate; don’t stack them.)
That had been my original idea long ago, but because of the set logic the resolver does on versions all over, it led to an explosion in complexity. (If a is the requirement of Package[ProductA, ProductB] at version 3..<8, what is ¬a and how do you represent it?) |
|
I was only talking about the second diff, didn't even look at the first one at all since you had already posted the update. I'll dive a bit more into how the assertion is happening, it doesn't actually make a whole lot of sense to me for the particular case, since we are seemingly choosing a lower version of a dependency for no apparent reason. It might be related to how In any case, I think I need to spend some time to come up with a few actual unit tests here, it doesn't really scale to figure this out with the large project I got. |
|
Starting with |
This is probably just a variation of the same problem. Each product node has a dependency on its own package at the same version, which is how separate products are prohibited from being split across versions. But if excessively broad cache keys are re‐returning mismatched dependency lists, then this error would result whenever the dropped node happened to be the synthesized one that forces the entire package to use a single version. |
|
Alright, the following seems “most correct” to me, adjusts It is almost identical to my very first diff posted in this thread, except that it renames one of the new loop variables so as not to inadvertently shadow an outer variable. That shadowing had originally resulted in a child node being passed to a function expecting a parent node. In turn that derailed everything and lead to the crash in my first round of testing that made me think I had horribly misunderstood something. Let me know if it works on your top secret package. |
|
New patch seems to exhibit the same behavior as the previous one for me. I'll dig in a bit more today. |
|
Closing this for now in favor of #3162 |
Looks like we were not necessarily fetching dependencies of packages with pre-5.2 tools versions during resolution. It seems correct to not consider the product filter during dependency resolution for pre-5.2 packages, since it won't be correct for them by definition.
rdar://70633425