Skip to content
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.

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

All statisfied requirements map to dependencies in InstallableUnitSlicer.computeDirectDependencies #3197

Closed
merks opened this issue Dec 9, 2023 · 4 comments

Comments

@merks
Copy link
Contributor

merks commented Dec 9, 2023

There are two cases where a requirement should not map to a dependency.

  1. When Requirement.getMax() == 0, i.e., for so-called negative requirements.
  2. When IRequirement.getFilter() != null and the filter is no applicable.

Mapping those two cases inappropriately can and does result in dependency cycles that break a build (for builds that worked for Tycho 2.x and 3.x).

Currently org.eclipse.tycho.p2maven.InstallableUnitSlicer.computeDirectDependencies(Collection<IInstallableUnit>, IQueryable<IInstallableUnit>) does not have enough context to determine if a filter is applicable or not, so the best we can do in the absence of such information is to assume the filter isn't applicable.

@laeubi
Copy link
Member

laeubi commented Dec 9, 2023

@merks can you explain what context are needed? ws/os and maybe system properties?

@laeubi
Copy link
Member

laeubi commented Dec 10, 2023

@merks I now have analyzed the problem and it is actually a bit more complex as initially thought:

  1. first you are right that negative requirements are handled as regular ones, but the more general problem is that Tycho does not consider the req.getMax() at all, this is quite easy to fix and I'll provide a PR for that.
  2. You are also right that filters are not considered, I have a solution to evaluate them but this can't be applied at the moment because it revealed another problem here see below.
  3. The root cause of all evil (beside the rather trivial problem 1.) is that currently we compute the project dependencies for each project and then hold them in a map. Based on that we now compute the inter-dependencies and projects to consider as an additional requirement. Now the following can (and does happen, e.g. for SWT build):

There is a project (look for example at p2Inf.hostRequireFragment, where there is a host a fragment and the host requires the fragment with a p2.inf but disable the filter with a property.

The consumer requires the host and therefore should require the fragment, and do not disable this by a property.

Because we compute the dependencies only once for a project, it is not included! Instead one needs to recompute the dependencies to be used based on the profile properties of the requesting project!

@laeubi
Copy link
Member

laeubi commented Dec 10, 2023

While your testcase will be good to verify case (1) I think one needs a similar construct like this:

to reproduce case (2) fully, where there is a bundle that wants to require another bundle (not fragment!) with p2.inf but only for one platform on a multi-platform build. Such a testcase can be quite tricky to build i fear.

laeubi added a commit to laeubi/tycho that referenced this issue Dec 10, 2023
Currently all matching units are considered even if there are only a
lower number to use.

This adds a filtering after the initial collection phase to limit the
dependencies to the max of the requirement and also adds the
infrastructure to support filtering based on the profile properties.

relates to eclipse-tycho#3197
laeubi added a commit that referenced this issue Dec 10, 2023
Currently all matching units are considered even if there are only a
lower number to use.

This adds a filtering after the initial collection phase to limit the
dependencies to the max of the requirement and also adds the
infrastructure to support filtering based on the profile properties.

relates to #3197
github-actions bot pushed a commit that referenced this issue Dec 10, 2023
Currently all matching units are considered even if there are only a
lower number to use.

This adds a filtering after the initial collection phase to limit the
dependencies to the max of the requirement and also adds the
infrastructure to support filtering based on the profile properties.

relates to #3197

(cherry picked from commit a350281)
laeubi added a commit that referenced this issue Dec 11, 2023
Currently all matching units are considered even if there are only a
lower number to use.

This adds a filtering after the initial collection phase to limit the
dependencies to the max of the requirement and also adds the
infrastructure to support filtering based on the profile properties.

relates to #3197

(cherry picked from commit a350281)
laeubi added a commit to laeubi/tycho that referenced this issue Dec 12, 2023
Currently the context IUs are not used to filter project requirements,
this can result in dependencies that are actually disabled to be
included in the project dependencies.

This now removes the previous special handling of fragments by fully
depend on only the context IUs and maximum requirements, for this
purpose the filtering has to be made on project specific contextIUs and
can't just store one collection of filtered items for all callers.

Fix eclipse-tycho#3197
laeubi added a commit to laeubi/tycho that referenced this issue Dec 15, 2023
Currently the context IUs are not used to filter project requirements,
this can result in dependencies that are actually disabled to be
included in the project dependencies.

This now removes the previous special handling of fragments by fully
depend on only the context IUs and maximum requirements, for this
purpose the filtering has to be made on project specific contextIUs and
can't just store one collection of filtered items for all callers.

Fix eclipse-tycho#3197
laeubi added a commit to laeubi/tycho that referenced this issue Dec 15, 2023
Currently the context IUs are not used to filter project requirements,
this can result in dependencies that are actually disabled to be
included in the project dependencies.

This now removes the previous special handling of fragments by fully
depend on only the context IUs and maximum requirements, for this
purpose the filtering has to be made on project specific contextIUs and
can't just store one collection of filtered items for all callers.

Fix eclipse-tycho#3197
@laeubi laeubi closed this as completed in e09e8f0 Dec 15, 2023
laeubi added a commit to laeubi/tycho that referenced this issue Dec 15, 2023
Currently the context IUs are not used to filter project requirements,
this can result in dependencies that are actually disabled to be
included in the project dependencies.

This now removes the previous special handling of fragments by fully
depend on only the context IUs and maximum requirements, for this
purpose the filtering has to be made on project specific contextIUs and
can't just store one collection of filtered items for all callers.

Fix eclipse-tycho#3197
@merks
Copy link
Contributor Author

merks commented Dec 15, 2023

You're awesome. 🎖️

laeubi added a commit that referenced this issue Dec 16, 2023
Currently the context IUs are not used to filter project requirements,
this can result in dependencies that are actually disabled to be
included in the project dependencies.

This now removes the previous special handling of fragments by fully
depend on only the context IUs and maximum requirements, for this
purpose the filtering has to be made on project specific contextIUs and
can't just store one collection of filtered items for all callers.

Fix #3197
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants