Clean-up, simplify and modernize Slicer, PermissiveSlicer ans SimplePlanner #349

HannesWell · 2023-10-09T23:16:20Z

No description provided.

HannesWell · 2023-10-09T23:18:31Z

...les/org.eclipse.equinox.p2.director/src/org/eclipse/equinox/internal/p2/director/Slicer.java

 		this.possibilites = possibilites;
 		this.selectionContext = selectionContext;
 		this.considerMetaRequirements = considerMetaRequirements;
-		slice = new HashMap<>();
-		result = new MultiStatus(DirectorActivator.PI_DIRECTOR, IStatus.OK, Messages.Planner_Problems_resolving_plan, null);
 	}

 	public IQueryable<IInstallableUnit> slice(IInstallableUnit[] ius, IProgressMonitor monitor) {


This is an internal class but I know that for example Tycho is using it.
Tycho can be adapted (and would actually benefit from less to-array conversion), but I wonder if there are any know other external user?
Should we keep this method or just delete it and only provide the replacement with an Collection argument?

if Oomph is not using it (@merks ?) I think it can be removed.

What is the benefit over deleting it / replacing it with a collection type method? Beside for me it has shown that sharing such code with P2 has litte to no benefit so one maybe probably better copy that over to Tycho as it is much more flexible and there are no discussions about changes here and then...

Oomph doesn't use the Slicer.

if Oomph is not using it (@merks ?) I think it can be removed.

Removed it now.

What is the benefit over deleting it / replacing it with a collection type method?

Less toArray() conversion which make the code more readable and a bit faster (but probably not measurable).

Beside for me it has shown that sharing such code with P2 has litte to no benefit so one maybe probably better copy that over to Tycho as it is much more flexible and there are no discussions about changes here and then...

I understand and agree that discussing things in multiple repos can be annoying and slowing down development, but eventually the changes should be done up-stream (which is P2 in this case). Because otherwise one probably borrows the saved time only from the future if duplicated maintenance is necessary when new features/fixes arrive or just another developers wonders why there is a copy of an up-stream class and trys merge it again. And in worst case bug-fixes got missed.
For example this is in preparation to contribute the changes made in Tycho's eclipse-tycho/tycho#2852 to P2.

That a lot "maybe" and "if" that never happened in the past, instead all kind of fruitless discussions and "but what if this breaks any consumer we don't know" has arises, see also:

Get rid of P2 in tycho-core eclipse-tycho/tycho#2019

Could be, but I don't really see it in this case.

So is everybody fine with this change or are there objections? If not, I would like to submit this.

Seems okay.

github-actions · 2023-10-09T23:40:42Z

Test Results

      9 files ±0       9 suites ±0 36m 41s ⏱️ - 1m 23s
2 177 tests ±0 2 173 ✔️ ±0   4 💤 ±0 0 ❌ ±0
6 621 runs ±0 6 610 ✔️ ±0 11 💤 ±0 0 ❌ ±0

Results for commit 012d53e. ± Comparison against base commit 158291b.

♻️ This comment has been updated with latest results.

laeubi · 2023-10-22T05:36:47Z

...les/org.eclipse.equinox.p2.director/src/org/eclipse/equinox/internal/p2/director/Slicer.java

 	}

-	public IQueryable<IInstallableUnit> slice(IInstallableUnit[] ius, IProgressMonitor monitor) {
+	public IQueryable<IInstallableUnit> slice(Collection<IInstallableUnit> ius, IProgressMonitor monitor) {


This seem to have broken the i-build:
https://ci.eclipse.org/releng/job/Builds/job/I-build-4.30/78/console

00:16:41 [ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:4.0.4-SNAPSHOT:compile (default-compile) on project org.eclipse.pde.core: Compilation failure: Compilation failure:
00:16:41 [ERROR] /home/jenkins/agent/workspace/Builds/I-build-4.30/eclipse.platform.releng.aggregator/eclipse.platform.releng.aggregator/cje-production/gitCache/eclipse.platform.releng.aggregator/eclipse.pde/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IUBundleContainer.java:[321]
00:16:41 [ERROR] IQueryable slice = slicer.slice(fUnits, new NullProgressMonitor());
00:16:41 [ERROR] ^^^^^
00:16:41 [ERROR] The method slice(Collection, IProgressMonitor) in the type Slicer is not applicable for the arguments (IInstallableUnit[], NullProgressMonitor)
00:16:41 [ERROR] /home/jenkins/agent/workspace/Builds/I-build-4.30/eclipse.platform.releng.aggregator/eclipse.platform.releng.aggregator/cje-production/gitCache/eclipse.platform.releng.aggregator/eclipse.pde/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java:[1364]
00:16:41 [ERROR] IQueryable slice = slicer.slice(units, subMonitor.split(50));
00:16:41 [ERROR] ^^^^^
00:16:41 [ERROR] The method slice(Collection, IProgressMonitor) in the type Slicer is not applicable for the arguments (IInstallableUnit[], SubMonitor)
00:16:41 [ERROR] 2 problems (2 errors)
00:16:41 [ERROR] -> [Help 1]

This is probably unrelated to this PR but I think changing the signature of the slice method also broke the installation process (Oomph) so I'm commenting here anyway (if you know a better place, please let me know):
I just installed a new Eclipse with the Oomph-Installer (I'm using Mac) and when it got to the point of executing manual tasks after Eclipse opened for the first time, I got this error when computing the step Modular Target:

Performing Targlets Modular Target (eclipse-sdk-prereqs + ECF Dependency + EMF Dependency + Eclipse Platform + Platform + Platform Build Tools + Platform Images + Platform Releng Aggregator + Platform Root + Platform UI + Platform Website + SWT + SWT Binaries + SWT Website), activate ... Committing the provisioning operation. ERROR: org.eclipse.pde.core code=0 Problems occurred while resolving the target contents at org.eclipse.oomph.util.OomphPlugin.coreException(OomphPlugin.java:296) at org.eclipse.oomph.util.pde.TargetPlatformUtil.activateTargetDefinition(TargetPlatformUtil.java:151) at org.eclipse.oomph.targlets.internal.core.TargletContainer.forceUpdate(TargletContainer.java:905) at org.eclipse.oomph.setup.targlets.impl.TargletTaskImpl$4.run(TargletTaskImpl.java:1232) at org.eclipse.oomph.util.pde.TargetPlatformUtil.runWithTargetPlatformService(TargetPlatformUtil.java:120) at org.eclipse.oomph.setup.targlets.impl.TargletTaskImpl.perform(TargletTaskImpl.java:1092) at org.eclipse.oomph.setup.internal.core.SetupTaskPerformer.doPerformNeededSetupTasks(SetupTaskPerformer.java:3864) at org.eclipse.oomph.setup.internal.core.SetupTaskPerformer$WorkspaceUtil$1.run(SetupTaskPerformer.java:5200) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2453) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2478) at org.eclipse.oomph.setup.internal.core.SetupTaskPerformer$WorkspaceUtil.performNeededSetupTasks(SetupTaskPerformer.java:5193) at org.eclipse.oomph.setup.internal.core.SetupTaskPerformer.performNeededSetupTasks(SetupTaskPerformer.java:3798) at org.eclipse.oomph.setup.internal.core.SetupTaskPerformer.performTriggeredSetupTasks(SetupTaskPerformer.java:3773) at org.eclipse.oomph.setup.internal.core.SetupTaskPerformer.perform(SetupTaskPerformer.java:3651) at org.eclipse.oomph.setup.ui.wizards.ProgressPage$9.run(ProgressPage.java:592) at org.eclipse.oomph.setup.ui.wizards.ProgressPage$11$1.run(ProgressPage.java:721) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63) ERROR: org.eclipse.oomph.targlets.core code=0 Resolution problems ERROR: org.eclipse.oomph.targlets.core code=0 Problems resolving composed target 'eclipse-sdk-prereqs' ERROR: org.eclipse.pde.core code=0 Problems occurred while resolving the target contents ERROR: org.eclipse.pde.core code=0 Problems loading repositories ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit com.jcraft.jsch 0.1.55.v20230916-1400 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit com.jcraft.jsch.source 0.1.55.v20230916-1400 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.ant 1.10.14.v20230922-1200 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.ant.source 1.10.14.v20230922-1200 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.batik.css 1.17.0.v20231009-1000 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.batik.util 1.17.0.v20231009-1000 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.batik.i18n 1.17.0.v20231009-1000 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.batik.constants 1.17.0.v20231009-1000 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.batik.css.source 1.17.0.v20231009-1000 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.batik.util.source 1.17.0.v20231009-1000 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.batik.i18n.source 1.17.0.v20231009-1000 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.batik.constants.source 1.17.0.v20231009-1000 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.xmlgraphics 2.9.0.v20230916-1600 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.xmlgraphics.source 2.9.0.v20230916-1600 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.junit 4.13.2.v20230809-1000 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.junit.source 4.13.2.v20230809-1000 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.lucene.core 9.8.0.v20230929-1030 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.lucene.analysis-smartcn 9.8.0.v20230929-1030 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.lucene.analysis-common 9.8.0.v20230929-1030 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.lucene.core.source 9.8.0.v20230929-1030 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.lucene.analysis-smartcn.source 9.8.0.v20230929-1030 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.apache.lucene.analysis-common.source 9.8.0.v20230929-1030 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.hamcrest.core 2.2.0.v20230809-1000 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.hamcrest.core.source 2.2.0.v20230809-1000 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.jdom 1.1.3.v20230812-1600 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.jdom.source 1.1.3.v20230812-1600 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit com.sun.jna 5.13.0.v20230812-1000 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.eclipse.orbit.xml-apis-ext 1.0.0.v20230923-0644 ERROR: org.eclipse.pde.core code=0 Unable to locate installable unit org.eclipse.orbit.xml-apis-ext.source 1.0.0.v20230923-0644

So I opened the file eclipse-sdk-prereqs.target present in my WS and got this error instead:

Looking closer, this is the stack trace from the event in the Error Log

java.lang.NoSuchMethodError: 'org.eclipse.equinox.p2.query.IQueryable org.eclipse.equinox.internal.p2.director.PermissiveSlicer.slice(java.util.Collection, org.eclipse.core.runtime.IProgressMonitor)' at org.eclipse.pde.internal.core.target.P2TargetUtils.slice(P2TargetUtils.java:1364) at org.eclipse.pde.internal.core.target.P2TargetUtils.resolveWithSlicer(P2TargetUtils.java:1284) at org.eclipse.pde.internal.core.target.P2TargetUtils.synchronize(P2TargetUtils.java:831) at org.eclipse.pde.internal.core.target.TargetDefinition.resolve(TargetDefinition.java:404) at org.eclipse.pde.internal.ui.editor.targetdefinition.TargetEditor$TargetChangedListener$1.run(TargetEditor.java:651) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)

I noticed that the missing method is actually the new version of slice i.e. the one with the Collection in the parameters, which is odd.

Any idea how to fix this?

About the Oomph Installer

Version: 1.26.0 Build 5632

The missing new methods does looks odd indeed. At this point it looks like you would have to run the p2 task to update the installation. Another workaround is to delete this part of the task (the one that composed the actual *.target file) temporarily:

@HannesWell

I've lost count of how many times the resolution of the Platform's real *.target causes problem. It's certainly very good that one can get any target platform updates immediately rather than wait for the next I-Build, but very frustrating how often PDE fails to resolve it. Perhaps I need to do something in Oomph to be able to ignore failures...

Just for the record PDE was fixed with eclipse-pde/eclipse.pde#808.

Any idea how to fix this?

From the exception and the screenshot of the installed product I assume your installation has not the latest (I-Build) version of the org.eclipse.equinox.p2.director installed but somehow has the latest (I-build) version of o.e.pde.core?
Can you provide the exact version (including qualifier) of both bundles from the 'About' dialog?

I've lost count of how many times the resolution of the Platform's real *.target causes problem. It's certainly very good that one can get any target platform updates immediately rather than wait for the next I-Build, but very frustrating how often PDE fails to resolve it. Perhaps I need to do something in Oomph to be able to ignore failures...

Sadly I noticed that as well.
I could solve the resolution errors if I manually opended the prerequ-sdk target in the editor and after the resolution completed run the Oomph-Setup again.
Maybe the equivalent can be done in Oomph automatically? I can look-up the code or create a PR for Oomph if you like.

In general I already started to look into making the target-platform resolution more stable in PDE and preventing the quirks of unncessary re-resolved profiles, I already found a few things that are very likely causing problems but I'm currently busy with other things (i.e. the Windows-Defender auto-fix) so it might take a bit until I can complete these fixes.

The problem persist even after a 2nd and a 3rd attempt today, This is what I tried:

2nd attempt: I ran Help > Perform setup tasks today --> only the version of the o.e.pde.core got upgraded to 3.17.200.v20231023-0618 (o.e.equinox... remained the same)

3rd attempt: I downloaded the newest Oomph installer (1.31.0 Build 213) and installed from scratch --> same result: o.e.pde.core got upgraded to 3.17.200.v20231023-0618 and o.e.equinox... remained the same.

How inconvenient, this effectively keeps me from contributing from my Mac :-\

I ran the p2 director task yesterday and see this version installed:

The director task is installing from these sites:

There is definitely a new version of the p2 director bundle in that repository. Moreover, I have no idea which of those repositories could (does) provide an older version...

Can you show a screen grab of the p2 director task details?

I also see https://download.eclipse.org/eclipse/updates/4.30-I-builds in the P2 Director task when performing the setup tasks.

And here is the full log after I perform the setup tasks: fullLog.txt

Now it's clear what the problem is!

You did not really use the configuration:

https://github.com/eclipse-platform/.github/blob/main/CONTRIBUTING.md#creating-an-eclipse-development-environment

So you did not install the Eclipse SDK product as specified in the configuration:

Instead you installed the Eclipse Committers product. So you have a combination of content that's installed from the EPP/SimRel M1 along with content installed from the 4.30 I-Builds. That's because the products have some very specific ranges on things that are intended never to be uninstalled from such a product:

If you're going to do things a bit more manually, be sure to use the Eclipse SDK product:

🤯

This one is going directly to my favorites.

Thanks @merks, that solved my problem! 🥇

HannesWell commented Oct 9, 2023

View reviewed changes

HannesWell force-pushed the cleanUpSlicer branch from 3cf64ba to 149315f Compare October 10, 2023 22:14

HannesWell force-pushed the cleanUpSlicer branch 2 times, most recently from fbb8c10 to 571da71 Compare October 21, 2023 15:36

Clean-up and simplify Slicer, PermissiveSlicer ans SimplePlanner

012d53e

HannesWell force-pushed the cleanUpSlicer branch from 571da71 to 012d53e Compare October 21, 2023 18:35

HannesWell merged commit 38765a5 into eclipse-equinox:master Oct 21, 2023
9 checks passed

HannesWell deleted the cleanUpSlicer branch October 21, 2023 19:06

laeubi reviewed Oct 22, 2023

View reviewed changes

fedejeanne mentioned this pull request Nov 6, 2023

Test projects are being imported when using the "official" Oomph installation eclipse-jdt/eclipse.jdt.ui#903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean-up, simplify and modernize Slicer, PermissiveSlicer ans SimplePlanner #349

Clean-up, simplify and modernize Slicer, PermissiveSlicer ans SimplePlanner #349

HannesWell commented Oct 9, 2023

HannesWell Oct 9, 2023

akurtakov Oct 10, 2023

laeubi Oct 10, 2023 •

edited

Loading

merks Oct 10, 2023

HannesWell Oct 10, 2023

laeubi Oct 11, 2023

HannesWell Oct 21, 2023

merks Oct 21, 2023

github-actions bot commented Oct 9, 2023 •

edited

Loading

laeubi Oct 22, 2023

fedejeanne Oct 22, 2023

merks Oct 22, 2023

HannesWell Oct 22, 2023

HannesWell Oct 22, 2023

fedejeanne Oct 25, 2023

merks Oct 25, 2023

fedejeanne Oct 26, 2023

merks Oct 26, 2023

fedejeanne Oct 26, 2023

Clean-up, simplify and modernize Slicer, PermissiveSlicer ans SimplePlanner #349

Clean-up, simplify and modernize Slicer, PermissiveSlicer ans SimplePlanner #349

Conversation

HannesWell commented Oct 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laeubi Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 9, 2023 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

About the Oomph Installer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laeubi Oct 10, 2023 •

edited

Loading

github-actions bot commented Oct 9, 2023 •

edited

Loading