[Build] Enable API checks in CI builds

[HannesWell]

Fixes #1003

[github-actions]

Test Results

  200 files   -    99    200 suites   - 99   5m 47s ⏱️ - 1m 30s
4 099 tests ±    0  4 091 ✅ ±    0   8 💤 ± 0  0 ❌ ±0 
8 163 runs   - 4 046  8 105 ✅  - 4 029  58 💤  - 17  0 ❌ ±0 

Results for commit 51a079e. ± Comparison against base commit 0818fed.

♻️ This comment has been updated with latest results.

[HannesWell]

I have not investigated it in detail but maybe API checking fragments of other platforms fails because the fragments don't resolve in that case.
But since we build on all platforms we could just API check the fragment for the current platform for now.

But because in general all native fragments can have their Java code compiled on a platform (I can confirm this at least for Windows), it could work to API check all fragments on Jenkins only. But this probably requires adjustments in Tycho/PDE and it is of course the question if this special case is worth it.

[iloveeclipse]

since we build on all platforms we could just API check the fragment for the current platform for now

Yes. This would be enough for now.

[HannesWell]

Looks like we have the first API error catched:

Error:  Failed to execute goal org.eclipse.tycho:tycho-apitools-plugin:4.0.5:verify (verify) on project org.eclipse.swt.cocoa.macosx.x86_64: There are API errors:
Error:  Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java:106 The @noextend restriction have been added for type org.eclipse.swt.widgets.Display
Error:  META-INF/MANIFEST.MF:6 The major version should be incremented in version 3.125.0, since API breakage occurred since version 3.124.200

But when looking at git-blame it says that @noextend is there since ever (at least 2009). So I think it this is a false positive.

since we build on all platforms we could just API check the fragment for the current platform for now

Yes. This would be enough for now.

Fortunately the tycho-apitools-plugin already disables itself if the fragment cannot be resolved (not sure why there where problems before).

I have also tried to enable the api-validation for all platforms at the same time, by setting the os/ws/arch properties of the target-platform-configuration accordingly but this seem not to help.
@laeubi can you tell if the target-platform-configuration is considered when the tycho-apitools verification is executed?

[laeubi]

API tools does not know anything about target platform, only dependencies of the project and the baseline is resolved by Tycho and then passed to API-Tools, you can enable the debugging to see more details.

[laeubi]

@HannesWell see

• Resolve API baseline with all matching environments eclipse-tycho/tycho#3457
• Use 4.0.6-SNAPSHOT for latest fixes/enhancements eclipse.platform.releng.aggregator#1758

[HannesWell]

Awesome, thanks @laeubi.

Looks like we have the first API error catched:\n\nError: Failed to execute goal org.eclipse.tycho:tycho-apitools-plugin:4.0.5:verify (verify) on project org.eclipse.swt.cocoa.macosx.x86_64: There are API errors:\nError: Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java:106 The @noextend restriction have been added for type org.eclipse.swt.widgets.Display\nError: META-INF/MANIFEST.MF:6 The major version should be incremented in version 3.125.0, since API breakage occurred since version 3.124.200\nBut when looking at git-blame it says that @noextend is there since ever (at least 2009). So I think it this is a false positive.

@Phillipus I assume you don't see this kind of API error locally in the workspace on your Mac?

[HeikoKlare]

I see it on Mac since, I think, pulling #1008. I assumed it to be a local/temporary problem, but did not have time to investigate further yet.

[HannesWell]

I see it on Mac since, I think, pulling #1008. I assumed it to be a local/temporary problem, but did not have time to investigate further yet.

Thanks for checking this.
Looking at the .api_description files of the swt fragments for different platforms, I see <type name="Display" restrictions="2"> for Windows and Linux fragments and only <type name="Display" restrictions="0"> for both Mac platforms.
I assume 2 means no extend and 0 means no restrictions.
Bu then the questions remain: why is there a difference between platforms in I-builds (and probably also older releases) and why does the difference vanish in CI (and was Heiko reported) local API checks?

[laeubi]

The Nullpointer exception is fixed here:

• Print not mapped API messages to the log eclipse-tycho/tycho#3477

Bu then the questions remain: why is there a difference between platforms in I-builds (and probably also older releases) and why does the difference vanish in CI (and was Heiko reported) local API checks?

What do you mean by "vanish"? The IBuild uses ant and only compares the jars (maybe even ignoring swt in some way or handling it special), so this is "easier" but at the expense of not handling it necessarily the same as in the IDE.

Tycho executes API tools (mostly) as in the IDE so there you should see equal messages between IDE+Build.

[HannesWell]

I see it on Mac since, I think, pulling #1008. I assumed it to be a local/temporary problem, but did not have time to investigate further yet.

@HeikoKlare or @Phillipus could you please be so kind and post the content of the .api_filters file after you have asked APITools to add a filter on that error? It looks like the one I captured does not work.

[HeikoKlare]

I get this .api_filter and it resolves the warning in the IDE:

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<component id="org.eclipse.swt.cocoa.macosx.aarch64" version="2">
    <resource path="Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java" type="org.eclipse.swt.widgets.Display">
        <filter id="336744520">
            <message_arguments>
                <message_argument value="@noextend"/>
                <message_argument value="org.eclipse.swt.widgets.Display"/>
            </message_arguments>
        </filter>
    </resource>
</component>

[laeubi]

API filters are not correctly evaluated in the build if you use linked resources as mentioned here

• Restore CLASSPATH_ATTR_LIBRARY_PATH_ENTRYs in native .classpath files #1031 (comment)

I'll try to take a look but can't make a promise as the topic is rather complex to solve in general as it is very generic and support man indirection e.g. variables that probably can not be evaluated like done in a full ide...

[HannesWell]

Thanks Heiko, that looks different to mine. I'll try it this evening.

API filters are not correctly evaluated in the build if you use linked resources as mentioned here

• Restore CLASSPATH_ATTR_LIBRARY_PATH_ENTRYs in native .classpath files #1031 (comment)

I'll try to take a look but can't make a promise as the topic is rather complex to solve in general as it is very generic and support man indirection e.g. variables that probably can not be evaluated like done in a full ide...

Thank you for your help.
In the meantime I'll try out if placing the filters in the fragments directly works. Since filter is platform specific (and hopefully goes away after the next release anyway) I see no problem with that. It's just that the IDE then probably doesn't pick this up. In the worst case we have to duplicate that filter file temporarily.

[Phillipus]

I get this .api_filter and it resolves the warning in the IDE:

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<component id="org.eclipse.swt.cocoa.macosx.aarch64" version="2">
    <resource path="Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java" type="org.eclipse.swt.widgets.Display">
        <filter id="336744520">
            <message_arguments>
                <message_argument value="@noextend"/>
                <message_argument value="org.eclipse.swt.widgets.Display"/>
            </message_arguments>
        </filter>
    </resource>
</component>

Exactly the same here on Mac.

[iloveeclipse]

@HannesWell : 4.31 is almost released, we are in 4.32 development cycle already.

[HannesWell]

@HannesWell : 4.31 is almost released, we are in 4.32 development cycle already.

I know. I was just hoping that an update of the RC or a patch release was not yet completely off the table (although I haven't checked if this would have too far consequences due to version locking in features).

But with all your work in PDE and Tycho we are probably already prepared to handle this (for API tools users) big problem. Thank you already for this, I just didn't catch up on all of it (had an 8h flight and are waiting for the train to finally get home).

[iloveeclipse]

API checks are fine now. Anything else missing in this PR?

[laeubi]

API checks are fine now. Anything else missing in this PR?

I think existing API warnings should be suppressed (if we not plan to fix them), maybe wee need some more x-friends?

[laeubi]

This also looks strange:

ControlListener extends non-API type SWTEventListener

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/SWTEventListener.java

Lines 19 to 31 in d06d1a1

	/**
	* This interface is the cross-platform version of the
	* java.util.EventListener interface.
	* <p>
	* It is part of our effort to provide support for both J2SE
	* and J2ME platforms. Under this scheme, classes need to
	* implement SWTEventListener instead of java.util.EventListener.
	* </p>
	* <p>
	* Note: java.util.EventListener is not part of CDC and CLDC.
	* </p>
	* @noreference This interface is not intended to be referenced by clients.
	*/

maybe we should simply delete the interface, J2ME platforms are likely not a target anymore for SWT?

[HannesWell]

API checks are fine now. Anything else missing in this PR?

I think existing API warnings should be suppressed (if we not plan to fix them), maybe wee need some more x-friends?

I haven't checked all warnings, but as far as I can tell they are all about extending the non-API type SWTEventListener as you already mentioned.

maybe we should simply delete the interface, J2ME platforms are likely not a target anymore for SWT?

Either that or make it part of SWT's API or suppress all warnings (which would be a bit tedious since we would have to do it for all fragments) or just accept they exists.
But if J2ME isn't relevant anymore I would also vote for inlineing this interface, but I cannot tell if the premise is true.

[iloveeclipse]

Can we please first fix one issue (no API checks) and once we are confident it works, continue with any refactoring we might want in SWT (like removing SWTEventListener or whatever else)?

[laeubi]

Either that or make it part of SWT's API or suppress all warnings (which would be a bit tedious since we would have to do it for all fragments) or just accept they exists.

If we want to keep this interface, we should suppress them, that's one time work (with maybe can be copied a lot) but keeping them will just let us start bad... If we start clean you can even set the option failOnWarnings so we can ensure no new API problems are introduced (I did this for PDE already).

[laeubi]

Can we please first fix one issue (no API checks) and once we are confident it works, continue with any refactoring we might want in SWT (like removing SWTEventListener or whatever else)?

So then people can complain that they get "unrelated" warnings?

[HannesWell]

Either that or make it part of SWT's API or suppress all warnings (which would be a bit tedious since we would have to do it for all fragments) or just accept they exists.

If we want to keep this interface, we should suppress them, that's one time work (with maybe can be copied a lot) but keeping them will just let us start bad... If we start clean you can even set the option failOnWarnings so we can ensure no new API problems are introduced (I did this for PDE already).

If we want to keep it yes the warnings should be suppressed, but I agree with with Andrey that we can keep this for a follow-up/separate PR and focus here on enabling the API-tools per se.
I'll investigate a bit if J2ME is still relevant for SWT and then create a PR to either suppress the warnings or to inline the interface and we can discuss that topic there.

[akurtakov]

J2ME is dead and I tried to clean up in the past c66026a but obviously there are more leftovers. https://bugs.eclipse.org/bugs/show_bug.cgi?id=483638 has more content.

[HannesWell]

J2ME is dead and I tried to clean up in the past c66026a but obviously there are more leftovers. https://bugs.eclipse.org/bugs/show_bug.cgi?id=483638 has more content.

Created #1101 to inline it. Please have a look.

[HannesWell]

With #1101 we are down to one API error.
I'll follow-ups to resolve them and to resolve/suppress the remaining warnings so that we can even enforce no-warnings.
With that and given the build does not fail here (which it did before), I think this is ready.
Any objections?

[iloveeclipse]

No.