Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Build] Enable API checks in CI builds #1011

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

HannesWell
Copy link
Member

Fixes #1003

Copy link
Contributor

github-actions bot commented Jan 28, 2024

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
Copy link
Member Author

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
Copy link
Member

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 HannesWell force-pushed the enable-api-checks branch 3 times, most recently from 5f792ba to 5de57cb Compare February 4, 2024 13:16
@HannesWell
Copy link
Member Author

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
Copy link
Contributor

laeubi commented Feb 4, 2024

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
Copy link
Contributor

laeubi commented Feb 4, 2024

@HannesWell
Copy link
Member Author

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
Copy link
Contributor

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.

image

@HannesWell HannesWell force-pushed the enable-api-checks branch 4 times, most recently from 1733793 to 1a6ef25 Compare February 5, 2024 23:26
@HannesWell
Copy link
Member Author

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
Copy link
Contributor

laeubi commented Feb 6, 2024

The Nullpointer exception is fixed here:

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.

binaries/pom.xml Outdated Show resolved Hide resolved
@HannesWell
Copy link
Member Author

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.

image

@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
Copy link
Contributor

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
Copy link
Contributor

laeubi commented Feb 7, 2024

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

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
Copy link
Member Author

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

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
Copy link
Contributor

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
Copy link
Member

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

@HannesWell
Copy link
Member Author

@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
Copy link
Member

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

@laeubi
Copy link
Contributor

laeubi commented Mar 12, 2024

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?

Jenkinsfile Outdated Show resolved Hide resolved
@laeubi
Copy link
Contributor

laeubi commented Mar 12, 2024

This also looks strange:

ControlListener extends non-API type SWTEventListener

/**
* 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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Contributor

laeubi commented Mar 12, 2024

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
Copy link
Contributor

laeubi commented Mar 12, 2024

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
Copy link
Member Author

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
Copy link
Member

akurtakov commented Mar 12, 2024

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

No.

@HannesWell HannesWell merged commit c116be3 into eclipse-platform:master Mar 12, 2024
9 of 13 checks passed
@HannesWell HannesWell deleted the enable-api-checks branch March 12, 2024 17:20
iloveeclipse added a commit to iloveeclipse/eclipse.platform.swt that referenced this pull request Mar 13, 2024
@iloveeclipse iloveeclipse mentioned this pull request Mar 13, 2024
iloveeclipse added a commit to iloveeclipse/eclipse.platform.swt that referenced this pull request Mar 13, 2024
iloveeclipse added a commit that referenced this pull request Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API errors not reported for PR's
6 participants