eclipse-equinox / p2

Eclipse Public License 2.0
15 stars 41 forks source link

Only select IUs up to the max of a requirement using highest version #432

Closed laeubi closed 9 months ago

laeubi commented 9 months ago

Currently if the slicer is given a set of IUs where multiple ones match a requirement, all of them are selected independently of the actual number of max constraint.

This is now changed by collecting all IUs that satisfy the requirement, then sort them by highest version and restrict the result to the maximum number per id.

Important: This potentially can change the behavior of the slicer! So it might be good to test it with some different use cases.

vogella commented 9 months ago

So finally the slicer would select the highest version? That would be really nice

laeubi commented 9 months ago

it depends a bit, be aware that we are talking about p2 units, but a requirement can be anything (e.g. a package) so if two bundles with different name provide the same package still both are used because we can't know what is "the highest" and highest is not always best, so I'm just offering this here as a way to evaluate this more.

Another approach would be to make the slicer more extensible and let the user decide...

github-actions[bot] commented 9 months ago

Test Results

    6 files   -   3      6 suites   - 3   34m 7s :stopwatch: +14s 2 067 tests  - 116  2 056 :white_check_mark:  - 123  1 :zzz:  - 3   4 :x: + 4   6 :fire: + 6  6 291 runs   - 348  6 259 :white_check_mark:  - 369  2 :zzz:  - 9  12 :x: +12  18 :fire: +18 

For more details on these failures and errors, see this check.

Results for commit 5c310106. ± Comparison against base commit 61dd7059.

This pull request removes 116 tests. ``` org.eclipse.equinox.p2.tests.importexport.ImportExportRemoteTests ‑ Unknown test org.eclipse.equinox.p2.tests.importexport.ImportExportTests ‑ testAllowExportFeaturesInstalledFromLocal org.eclipse.equinox.p2.tests.importexport.ImportExportTests ‑ testExportFeaturesInstalledFromLocal org.eclipse.equinox.p2.tests.importexport.ImportExportTests ‑ testIncompatibleP2f org.eclipse.equinox.p2.tests.importexport.ImportExportTests ‑ testLoadP2f org.eclipse.equinox.p2.tests.importexport.ImportExportTests ‑ testLoadUnknownP2f org.eclipse.equinox.p2.tests.ui.actions.ElementUtilsTest ‑ testElements org.eclipse.equinox.p2.tests.ui.actions.ElementUtilsTest ‑ testEmpty org.eclipse.equinox.p2.tests.ui.actions.ElementUtilsTest ‑ testIUs org.eclipse.equinox.p2.tests.ui.actions.ElementUtilsTest ‑ testInvalid … ```
laeubi commented 9 months ago

And as we see this fails some tests already :-\

I'll wait for @merks review here and probably downgrade to the "more extensible" approach then...

merks commented 9 months ago

I have a feeling there is an assumption here that a requirement is necessarily selecting IUs with the same BSN. But a requirement could be a package requirement and it could select IUs with different BSN. Also that's the case for properties matches...

merks commented 9 months ago

Hmmm. I think the group by is dealing with that.

laeubi commented 9 months ago

Hmmm. I think the group by is dealing with that.

Yes but it seems not working, I get the impression that one would need a two step approach here:

1) slice like today 2) preprocess (e.g. with a planner like) that decides if there are units that are effectively unreachable or some kind of duplicate filtering ... I think I'll drop this for now as it seems not really an easy win.

@vogella If you have an issue with duplicate items with the target platform please open an issue at PDE with a reproducer.