API error on SWT on a 4.34 master

[iloveeclipse]

I see API error on SWT if using https://download.eclipse.org/eclipse/downloads/drops4/S-4.33RC2a-202409030240/ as API baseline and https://download.eclipse.org/eclipse/downloads/drops4/I20240904-0530/ as IDE.

SWT error is about changed execution environment.

I've rebuilt everything few times, no changes, errors do not disappear.

Note: originally reported API errors for JDT were gone after switching default workspace JDK from 21 to 17 and back, only SWT error remains.

[iloveeclipse]

Previous SWT related bug: #1283

[iloveeclipse]

Previous SWT related bug: #1283

@merks, @HannesWell, @ptziegler : If I see that right, the fix for that in #1292 was undone in #1302, see

https://github.com/ptziegler/eclipse.pde/blame/be7a44b30db25e212b915877567bd9a261808339/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/model/BundleComponent.java#L889

[merks]

I felt like this was a ground hog day issue that was fixed but now its back. 😱 That's what happens when there is no test. 😭

[iloveeclipse]

Interestingly, I can't reproduce JDT errors after switching default workspace JDK to Java 17 version (I had 21 before and tried to change that because I've assued that would "mute" SWT errors) and back. But instead, JDT errors disappeared but SWT still has errors ???

[iloveeclipse]

I see in debugger in org.eclipse.pde.api.tools.internal.model.BundleComponent.init(), that the binary fragment has empty array as fdeclaredRequiredEE, but workspace one gets JavaSE-17 (either current EE of the workspace or the one from host). So we have a mismatch... And yes, that looks like regression from #1302.

[merks]

FYI, I see both errors in my workspace:

[iloveeclipse]

Which JDK is set as default in the workspace? Could you try to change that?0

[merks]

I have 17 as the default:

[iloveeclipse]

have 17 as the default:

Could you just try to change it to 21, restart & rebuild JDT? That helped me, and after that I was able to switch back and still no errors.

[iloveeclipse]

I see contract violation in org.eclipse.osgi.internal.resolver.BundleDescriptionImpl.getRequirements(String)
It says:
The returned list will be empty if this resource declares no requirements in the specified namespace.
However it returns non empty list for SWT fragments because in org.eclipse.osgi.internal.resolver.BundleDescriptionImpl.getDeclaredRequirements(String) it calls org.eclipse.osgi.internal.resolver.BundleDescriptionImpl.getGenericRequires() that returns "Java 17".

Stack is

BundleDescriptionImpl.getDeclaredRequirements(String) line: 941	
BundleDescriptionImpl.getRequirements(String) line: 1275	
ManifestUtils.getRequiredExecutionEnvironments(Resource) line: 317	
ProjectComponent(BundleComponent).init() line: 317	
ProjectComponent(BundleComponent).getSymbolicName() line: 904	
WorkspaceBaseline(ApiBaseline).addComponent(IApiComponent) line: 326	

I can't say anything about that, it looks wrong, I will push a smaller workaround that fixes SWT error in a moment.

[iloveeclipse]

@merks : could you try #1387 for SWT error?

[merks]

have 17 as the default:

Could you just try to change it to 21, restart & rebuild JDT? That helped me, and after that I was able to switch back and still no errors.

The problem is there for 21 and when I switch back.

[merks]

@merks : could you try #1387 for SWT error?

Sorry, I have to get ready to go out for dinner. Maybe later when I'm back...

[HannesWell]

If I understand the issue correctly I think the new method in ManifestUtils should filter these artificial requirement out. That method is supposed to return what is declared in the manifest.

The test-case added in #1302 could also be extended to test the case if no EE is required.

I can look into both later this evening.

IIRC I checked the code for these artificial EE requirements when reviewing #1302, but it was probably only for the getExecutionEnvironments() method.

[ptziegler]

This issue is effectively the inverse of what I ran into, based on the two possible configurations where an error may be shown:

• The baseline component does specify an EE, but the workspace component doesn't.
• The baseline component doesn't specify an EE, but the workspace component does.

My issue is related to the first case, where the problem lied in the workspace component specifying the EE via the Require-Capability header, which wasn't supported at that time and was therefore ignored.

The issue @iloveeclipse is having is related to the second case, where the gtk fragment doesn't have an EE on the update site (and therefore in the baseline), but does have it in the SWT workspace.

[iloveeclipse]

gtk fragment doesn't have an EE on the update site (and therefore in the baseline), but does have it in the SWT workspace.

GTK fragment does not have EE in the manifest!

[ptziegler]

gtk fragment doesn't have an EE on the update site (and therefore in the baseline), but does have it in the SWT workspace.

GTK fragment does not have EE in the manifest!

Then I must've overlooked it... You are correct, of course. I've created #1388 to handle this regression and also tested it against your PR, which makes the test succeed.

Though that must mean the ManifestUtils method somehow still adds those artificial requirements, even if they are not in the manifest.

[iloveeclipse]

Though that must mean the ManifestUtils method somehow still adds those artificial requirements, even if they are not in the manifest.

See above: #1386 (comment)

[iloveeclipse]

I can look into both later this evening.

Thanks, would be good to have the fix merged soon

[HannesWell]

However it returns non empty list for SWT fragments because in org.eclipse.osgi.internal.resolver.BundleDescriptionImpl.getDeclaredRequirements(String) it calls org.eclipse.osgi.internal.resolver.BundleDescriptionImpl.getGenericRequires() that returns "Java 17".

I'm investigating why this is happening, but didn't found the cause yet.
I'll continue tomorrow so that this is resolved ASAP.

[HeikoKlare]

We would like to merge an SWT PR that will bump the minor version of the SWT bundles (eclipse-platform/eclipse.platform.swt#1409)

From my understanding, that will "implicitly" resolve the discussed API errors. I would like to make sure that merging it will not make analysis of this issue more complicated. So do you think we can merge that PR or should we wait for more progress or resolution of this issue? That would, of course, also apply to any further SWT PR that might require a minor version bump.

[iloveeclipse]

I would like to make sure that merging it will not make analysis of this issue more complicated.

I think the problem is understood and we have also a test case now via #1388.

So do you think we can merge that PR or should we wait for more progress or resolution of this issue?

Feel free to merge SWT code that can be merged.

[merks]

FYI, I did a completely from scratch platform SDK setup and the JDT problem recurs there:

But it doesn't occur from a debug launch. 😢

And trying to simplify the workspace before remote debugging, when the project is the only thing in the workspace and the rest is in the target platform, there are no API errors:

[merks]

In the debugger I see much more information about what the tool thinks has changed:

But note that what is says has changed has not really changed.

[merks]

It looks like things are going wrong here

The first description is processed and it has the correct information, but then the second one is processed for the mock thing and that one is inappropriate. I'm not sure if the order of these things is deterministic because the last one always wins.

And while exact has been set, but a wrapper object is returned and so nothing determines that the thing is exact.

How best to fix this...

[iloveeclipse]

Ed, do you mean, that analysis of the "mock" bundle makes the core bundle data corrupt, or the "mock" bundle analysis reports results to the wrong bundle? Why is "mock" thing analyzed at all?

[merks]

There are two descriptions and the last one wins. So the mock bundle's description wins. I assume everything in the workspace is analyzed. That mock thing has an NonAPIProjectDescription, whatever that is. Keep in mind I really don't know the design of the API tools. I'm considering either how to make use of exact information better how to prioritize a ProjectAPIDescription's annotation over those of a NonAPIProjectDecription. So I'm continuing to debug and try out solutions.

[iloveeclipse]

@merks : thanks to your great observation, I can reproduce too.
Once I open org.eclipse.jdt.core.tests.builder.mockcompiler project & rebuild org.eclipse.jdt.core I see API error.

The org.eclipse.jdt.core.tests.builder.mockcompiler project is a fragment of org.eclipse.jdt.core bundle.
But that is 1) a test and 2) why should it affect annotation visibility of the host types?

[merks]

I have prototyped what I believe is an appropriate fix so the method will be like this:

	public IApiAnnotations resolveAnnotations(IElementDescriptor element) {
		IApiAnnotations bestMatchAnnotation = null;
		for (IApiDescription fDescription : fDescriptions) {
			IApiAnnotations ann = fDescription.resolveAnnotations(element);
			if (ann != null) {
				bestMatchAnnotation = ann;
				if (ann.isExact()) {
					return ann; // if exact, return else keep looking for best match
				}
			}
		}
		return bestMatchAnnotation;
	}

I added a default isExact method to IApiAnnotations and make TypeAnnotations delegate to the IApiAnnotations that it wraps. This does fix the problem in debug mode.

But I have to go out now and will create a PR later and we can hope it causes no regressions.

[merks]

There was a "going out" delays so now there is a PR being built.

[HannesWell]

I see contract violation in org.eclipse.osgi.internal.resolver.BundleDescriptionImpl.getRequirements(String)
It says:
The returned list will be empty if this resource declares no requirements in the specified namespace.
However it returns non empty list for SWT fragments because in org.eclipse.osgi.internal.resolver.BundleDescriptionImpl.getDeclaredRequirements(String) it calls org.eclipse.osgi.internal.resolver.BundleDescriptionImpl.getGenericRequires() that returns "Java 17".

OK, I found the cause for this and PDE itself is actually to blame, BundleDescriptionImpl is totally fine.
And actually not surprisingly the cause close to the place that as duplicated and worked-around in #1292:

eclipse.pde/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/MinimalState.java

Lines 141 to 160 in 0953a33

	// inject BREE based on the project's JDK, otherwise packages from all
	// JREs are eligible for dependency resolution
	// e.g. a project compiled against Java 11 may get its java.xml
	// Import-Package resolved with a Java 8 profile
	IJavaProject javaProject = JavaCore.create(resource.getProject());
	if (!javaProject.exists()) {
	return manifest;
	}
	IVMInstall projectVmInstall = JavaRuntime.getVMInstall(javaProject);
	IExecutionEnvironment executionEnvironment = Arrays
	.stream(JavaRuntime.getExecutionEnvironmentsManager().getExecutionEnvironments())
	.filter(env -> env.isStrictlyCompatible(projectVmInstall)) //
	.findFirst().orElse(null);
	if (executionEnvironment != null) {
	manifest.put(Constants.BUNDLE_REQUIREDEXECUTIONENVIRONMENT, executionEnvironment.getId());
	}
	return manifest;

From the comments I believe we should really leave the injection of the synthetic EE in place. To fix this issue we should just restore the check for an explicit EE in BundleComponent, just like it was done in #1292 respectively restored in #1387.
I just created #1395, that restores this check but re-uses the method from MinimalState, which hopefully also creates a link in the code.

Alternatively we could also add another synthetic capability provided by a bundle with no EE and could check its existence.
This would also have the advantage that it could be checked in ManifestUtils.getRequiredExecutionEnvironments(). I wanted to use that method in more places in the future but I'm not yet sure if we actually always want only the declared EE requirements or sometimes also the 'effective' requirements. Probably the future has to show that.

eclipse.pde/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/ManifestUtils.java

Lines 316 to 321 in 0953a33

	public static Stream<String> getRequiredExecutionEnvironments(Resource resource) {
	List<Requirement> requirements = resource.getRequirements(EXECUTION_ENVIRONMENT_NAMESPACE);
	return requirements.stream()
	.map(requirement -> requirement.getDirectives().get(Namespace.REQUIREMENT_FILTER_DIRECTIVE))
	.mapMulti(ManifestUtils::parseRequiredEEsFromFilter);
	}