eclipse-pde / eclipse.pde

Eclipse Public License 2.0
25 stars 64 forks source link

Fix: API tool reports fictitious changes to execution environment #1302

Closed ptziegler closed 2 months ago

ptziegler commented 3 months ago

The API tool only checks the Bundle-RequiredExecutionEnvironment header when comparing the baseline with the workspace bundle. If the EE of the baseline bundle is described via the Require-Capability header, an empty string is compared to the workspace EE, which is treated as an incompatibility.

This also adds a simple test case where the execution environment of two bundles is checked. One bundle uses the Require-Capability, the other one the Bundle-RequiredExecutionEnvironment header.

Resolves https://github.com/eclipse-pde/eclipse.pde/issues/1301

ptziegler commented 3 months ago

Argh... this test setup is way too confusing.

The new bundle doesn't seem to be registered properly, though the issue still shows up when changing the manifest file of "bundle.a".

ptziegler commented 3 months ago

There we go... the expected error is now:

Unable to find BREE for bundle using 'Require-Capability': array lengths differed, expected.length=1 actual.length=0; arrays first differed at element [0]; expected:<JavaSE-17> but was:<end of array>
    at org.junit.internal.ComparisonCriteria.arrayEquals(ComparisonCriteria.java:89)
    at org.junit.internal.ComparisonCriteria.arrayEquals(ComparisonCriteria.java:28)
    at org.junit.Assert.internalArrayEquals(Assert.java:534)
    at org.junit.Assert.assertArrayEquals(Assert.java:285)
    at org.eclipse.pde.api.tools.builder.tests.compatibility.ProjectTypeContainerTests.testExecutionEnvironment(ProjectTypeContainerTests.java:170)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at junit.framework.TestCase.runTest(TestCase.java:177)
    at org.eclipse.jdt.core.tests.junit.extension.TestCase.runTest(TestCase.java:972)
    at junit.framework.TestCase.runBare(TestCase.java:142)
    at junit.framework.TestResult$1.protect(TestResult.java:122)
    at junit.framework.TestResult.runProtected(TestResult.java:142)
    at junit.framework.TestResult.run(TestResult.java:125)
    at junit.framework.TestCase.run(TestCase.java:130)
    at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:128)
    at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:40)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:529)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:757)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:452)
    at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner.main(RemotePluginTestRunner.java:83)
    at org.eclipse.pde.internal.junit.runtime.PlatformUITestHarness.lambda$0(PlatformUITestHarness.java:45)
    at org.eclipse.e4.ui.internal.workbench.swt.E4Testable.lambda$1(E4Testable.java:127)
    at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40)
    at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:132)
    at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:5040)
    at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4520)
    at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1151)
    at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
    at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1042)
    at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:152)
    at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:639)
    at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
    at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:546)
    at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:173)
    at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:152)
    at org.eclipse.pde.internal.junit.runtime.NonUIThreadTestApplication.start(NonUIThreadTestApplication.java:58)
    at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:208)
    at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:143)
    at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:109)
    at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:439)
    at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:271)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:668)
    at org.eclipse.equinox.launcher.Main.basicRun(Main.java:605)
    at org.eclipse.equinox.launcher.Main.run(Main.java:1481)
    at org.eclipse.equinox.launcher.Main.main(Main.java:1454)
Caused by: java.lang.AssertionError: expected:<JavaSE-17> but was:<end of array>
    at org.junit.Assert.fail(Assert.java:89)
    at org.junit.Assert.failNotEquals(Assert.java:835)
    at org.junit.Assert.assertEquals(Assert.java:120)
    at org.junit.Assert.assertEquals(Assert.java:146)
    at org.junit.internal.ComparisonCriteria.arrayEquals(ComparisonCriteria.java:87)
    ... 50 more
github-actions[bot] commented 3 months ago

Test Results

   291 files  ±0     291 suites  ±0   55m 52s :stopwatch: - 1m 13s  3 580 tests +1   3 504 :white_check_mark: +1   76 :zzz: ±0  0 :x: ±0  11 037 runs  +3  10 806 :white_check_mark: +3  231 :zzz: ±0  0 :x: ±0 

Results for commit be7a44b3. ± Comparison against base commit 1b1b92a3.

:recycle: This comment has been updated with latest results.

laeubi commented 3 months ago

@ptziegler great work in reproducing the issue!

ptziegler commented 3 months ago

Let's just hope that this time, less than all the tests crash... I forgot to push the .project and .classpath files, which is why it worked locally.

ptziegler commented 3 months ago

@HannesWell This is effectively the same fix I applied in the Equinox PR. The important part is to not use getExecutionEnvironments() as this method only supports the Bundle-RequireExecutionEnvironment header.

HannesWell commented 3 months ago

@HannesWell This is effectively the same fix I applied in the Equinox PR. The important part is to not use getExecutionEnvironments() as this method only supports the Bundle-RequireExecutionEnvironment header.

Thanks for that. I'll try my best to have a look at this tomorrow (Sorry was a busy weekend).

laeubi commented 3 months ago

Still other cases should be supported too.

As I have written elsewhere this is a tough case and probably can only be solved by matching the filter against a list of available EEs. I would just postpone that to the time where it is really needed.

ptziegler commented 3 months ago

I also changed your commit to use the new method and force pushed the result to this PR's branch, I hope that's fine for you. If you want to keep the old code, you should make sure to make a local of the branch.

That's more than fine by me. I'm not overly familiar with all possible combinations with which one could describe the EE, so I appreciate you taking the time and cleaning up those edge cases.

HannesWell commented 3 months ago

This change itself is ready, but I'm currently investigating other use-cases for the new method, e.g. in https://github.com/eclipse-pde/eclipse.pde/pull/1318. I would therefore like to keep this open to bring that in a more general shape, but I'll complete this in the next days and then submit this. @ptziegler from your issue assume this is not urgent for you and a few days more or less do not matter?

ptziegler commented 2 months ago

@HannesWell That is correct. The problem only shows up at the start of a release cycle, so it'll only come up again at the start of 2024-09.