Use PDE API-Tools java-annotations instead of javadoc-annotations

[HannesWell]

Using the java-annotations is more save/stable since the compiler can check them and would allow the SWT to get rid of the .api_description file (but at the moment it looks like apitools/the tycho apitools mojo still generates the entries in the description).

Furthermore this helps to fix (at least in my local build) the inconsistency in the generated .api_description for macos regarding the Display class as found in https://github.com/eclipse-platform/eclipse.platform.swt/pull/1011#issuecomment-1928492712. On Macos the entry is currently <type name="Display" restrictions="0">, while it is <type name="Display" restrictions="2"> on all other platforms. This probably leads to the error described in https://github.com/eclipse-platform/eclipse.platform.swt/pull/1011#issuecomment-1926412377. I assume somehow the existing @noextend javadoc annotation is currently not discovered, at least when running the build on Linux and Windows. On a mac it seems to be discovered. For other combinations of running platform and Display class platform-variant it works as expected. But I cannot tell why this is currently done wrong.

At the moment one cannot specify a informational message for these annotations, but I have created https://github.com/eclipse-pde/eclipse.pde/pull/1133 to add that possibility.

If there are not objections I'll apply this change to the remaining classes too.

Fixes https://github.com/eclipse-platform/eclipse.platform.swt/issues/1043

[github-actions[bot]]

Test Results

0 files   -    299  0 suites   - 299   0s :stopwatch: - 7m 1s 0 tests  -  4 099  0 :white_check_mark:  -  4 091  0 :zzz:  -  8  0 :x: ±0  0 runs   - 12 209  0 :white_check_mark:  - 12 134  0 :zzz:  - 75  0 :x: ±0 

Results for commit e3360c12. ± Comparison against base commit bd306acb.

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

[HannesWell]

Furthermore this helps to fix (at least in my local build) the inconsistency in the generated .api_description for macos regarding the Display class as found in #1011 (comment). On Macos the entry is currently <type name="Display" restrictions="0">, while it is <type name="Display" restrictions="2"> on all other platforms. This probably leads to the error described in #1011 (comment). I assume somehow the existing @noextend javadoc annotation is currently not discovered, at least when running the build on Linux and Windows. On a mac it seems to be discovered. For other combinations of running platform and Display class platform-variant it works as expected. But I cannot tell why this is currently done wrong.

Created https://github.com/eclipse-platform/eclipse.platform.swt/issues/1043 to track this further.

[HeikoKlare]

I agree that having annotations instead of javadoc comments is good, in particular to ease static analysis. Only concern I see is that this introduces a kind of further dependency to PDE. But if I understand correctly, it is kind of optional in the sense that if you want to analyze the PDE annotation, you can do that, otherwise it will be present in the class file but without having any further effect on SWT consumers, right? In that case, I vote for this change.

[laeubi]

Only concern I see is that this introduces a kind of further dependency to PDE

The (javadoc) annotations are only understood by PDE API Tools anyways ;-)

But if I understand correctly, it is kind of optional in the sense that if you want to analyze the PDE annotation, you can do that, otherwise it will be present in the class file but without having any further effect on SWT consumers, right?

The annotation are class retention annotations and therefore not preserved at runtime and only required at build time.

[HannesWell]

Only concern I see is that this introduces a kind of further dependency to PDE. But if I understand correctly, it is kind of optional in the sense that if you want to analyze the PDE annotation, you can do that, otherwise it will be present in the class file but without having any further effect on SWT consumers, right? In that case, I vote for this change.

No it doesn't. The neither the annotation providing package nor the bundle is added to the Manifest (as you can verify in the changes of this PR). In the IDE PDE and in the build Tycho automatically adds the apitools.annotation bundle to the project classpath so it is not necessary to define any dependency.

And besides what Christoph already mentioned, even if they would have retention policy runtime it would not be a problem. If an annotation's class cannot be loaded at runtime that annotation is simply ignored and no NoClassDefFoundError or alike is thrown.

So in that regard this change is safe :) And the generated .api_description can also be compared with an existing one.

[Phillipus]

How do you resolve the dependency on org.eclipse.pde.api.tools.annotations.NoExtend when testing in the workspace? I imported the org.eclipse.jface plug-in to my workspace but get several "The import org.eclipse.pde cannot be resolved" errors.

[Phillipus]

@HannesWell As long as additional.bundles = org.eclipse.pde.api.tools.annotations is present in the build.properties file all is OK in my workspace with the SWT bundles imported. However, if this is removed there are many errors as the dependency cannot be found (as I found when importing org.eclipse.jface into my workspace because it is not present in the build.properties file. See here).

1. Do you intend to remove additional.bundles = org.eclipse.pde.api.tools.annotations? There is a comment in that file - "Can be removed when built into Tycho"
2. If so, how will the dependency be resolved in the workspace?

[laeubi]

@Phillipus please use latest Platform SDK IDE: https://github.com/eclipse-platform/.github/blob/main/CONTRIBUTING.md#creating-an-eclipse-development-environment

it will then resolve everything automatically, and Tycho is doing it already since a few releases, so yes remove additional.bundles = org.eclipse.pde.api.tools.annotations please, it is misused already enough on other places...

[Phillipus]

@laeubi I'm using Eclipse 4.30 as IDE and a target platform of 4.31 (latest I build). I guess i need 4.31 as IDE. Thanks for the heads up.

@HannesWell Please ignore comments above.

@Phillipus Note to self - use latest IDE build as well as target platform when testing new features. ;-)

[HannesWell]

it will then resolve everything automatically, and Tycho is doing it already since a few releases, so yes remove additional.bundles = org.eclipse.pde.api.tools.annotations please, it is misused already enough on other places...

In general this is right, but currently not for this PR because it uses the new annotation message values, which a.t.m. are only available in I-build. It looks like Tycho uses the latest release of the annotations at the time of tycho's release, when adding them automatically to the classpath.

But I'm currently starting a three week vacation and when I'm back the next SimRel will happen soon.

Furthermore at least I get a lot of API errors in my workspace when using the annotations. It locks like they are not discovered in some cases. Actually it should work since the generated .api_descriptions looked good. I didn't have the time to investigate it yet, but will do it when I get back. But it's good to know that it works for you.

Another part in this story is Oomph. If there is no dependency in the bundle Oomph does not add the apitools-annotations to the TP and I had to add it explicitly. Maybe Oomph could do that implicitly (as optional requirement) if a API-baseline task is defined. @merks if you think this would be a good enhancement, I can try to contribute that when back.

[laeubi]

Tycho uses what is available from target platform and if nothing is found uses the latest from central.

[HannesWell]

Tycho uses what is available from target platform and if nothing is found uses the latest from central.

Then it was maybe a not updated cache or similar why I got compile errors in local builds. But I'm glad if the extra entries can be removed.

[akurtakov]

There are many conflicts with master now. Do you plan to finish this one or it should be abandoned?

[HannesWell]

I plan to finish this one, altough it has currently not the highest priority.