CollectConstraints from root project as well

[tinnefeld]

The collectConstraints() function in status.go incorporates effective
constraints from the root projects manifest. Issue #1452.

What does this do / why do we need it?

This patch includes the collection of constraints as defined in the root projects manifest as described in issue 1452. It ensures that the to-be-included constraint is effective which means that it refers to a direct dependency of the project.

What should your reviewer look out for in this PR?

At a first glance, it might seem desirable to collect the root projects constraints also in the for loop over the constraints from its direct dependencies. However, since the to-be-iterated data types and the way how the manifest is resolved are different, imho it's cleaner to collect the constraints from the root project separately.

Do you need help or clarification on anything?

collectConstraints() does currently not resolve the overwrite constraints from the root project's manifest ( if there are any ). A short clarification if this guarantee is desired or if it should be incorporated in a follow-up patch would be nice.

Which issue(s) does this PR fix?

fixes #1452

[darkowlzz]

I'm a little confused about this. I would need a little help with the usage of binary search here. Maybe a little explanation?

[tinnefeld]

FindIneffectualConstraints returns a sorted list of all the ineffective project roots. For every project root defined in the constraints of the projects manifest, we need to make sure it is not an ineffectual constraint. Instead of converting the result from FindIneffectualConstraints into a map or iterating over the whole range of its result, we utilize the property that it is sorted.

Please lmk if you want me to add the explanation to the code as well or if you prefer a different approach altogether.

[darkowlzz]

We don't need to use FindIneffectualConstraints. We can just use directDeps to check if the project root (pr) of the constraint is a direct dependency of the main root project. If yes, add it in the constraintCollection, else check the next project in manifest. Also, since this is collecting the constraints set by the main root project, it should be named as "root". So that would look something like this:

if _, ok := directDeps[pr]; !ok {
    continue
}

tempCC := append(
    constraintCollection[string(pr)],
    projectConstraint{"root", pp.Constraint},
)

Sorry I couldn't come up with all these last week. A little out of touch, took some time to recall 😅

[darkowlzz]

collectConstraints() does currently not resolve the overwrite constraints from the root project's manifest ( if there are any ). A short clarification if this guarantee is desired or if it should be incorporated in a follow-up patch would be nice.

Yes, the override constraints should also be considered, as they are part of the root project manifest.

The PR looks good overall, but I need a little help with that binary search mentioned in the in-line comment.

[darkowlzz]

Sorry for the delay.

But the good part is that I recalled the proper idea behind this issue.

So, the idea behind collectConstraints was to have a map of project name and list of project constraints (project name + constraint) for each of the dependencies, map[string][]projectConstraint, that's the type definition.

The result would be something like:

map[
  depA : [{projX ^1.0.0} {projY ^1.0.0}]
  depB : [{projY ^2.0.0}]
]

So it's a dependency and the project that has a constraint on that dependency. In the above, projX and projY have defined constraints on depA. projY has also defined constraints on depB.
We have this right now. The missing part was the constraints from the root project, i.e. the constraints set by the root project.
If, for example, the main root project has defined a constraint on depB in the manifest, the collectConstraints would have an entry like:

depB: [{projY ^2.0.0} {root ^2.0.0}]

I don't think it was documented anywhere to use the word "root", but I remember discussing it in a meeting and it was in my mind. Sorry for that.

Hope this explains what we need here. I've left some inline comment about the same. And please change the tests accordingly, if needed.

Thanks for the patience :)

[darkowlzz]

We don't need this. We can use directDeps that we have above in this function. This is because, any constraint in manifest that's a direct dependency is strictly applied. If we can't apply it, dep's solver would fail.

[darkowlzz]

We don't need to use FindIneffectualConstraints. We can just use directDeps to check if the project root (pr) of the constraint is a direct dependency of the main root project. If yes, add it in the constraintCollection, else check the next project in manifest. Also, since this is collecting the constraints set by the main root project, it should be named as "root". So that would look something like this:

if _, ok := directDeps[pr]; !ok {
    continue
}

tempCC := append(
    constraintCollection[string(pr)],
    projectConstraint{"root", pp.Constraint},
)

Sorry I couldn't come up with all these last week. A little out of touch, took some time to recall 😅

[tinnefeld]

Thank you for taking the time and providing such detailed feedback, I incorporated it accordingly.

[darkowlzz]

Thanks a lot 👍
LGTM!

[darkowlzz]

We can combine p.Manifest.Constraints and p.Manifest.Ovr into a single map, with override value taking precedence over any duplicate constraints entry, and iterate through overrides as well. Can do it as a follow-up PR.