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

Fix API warnings in macosx fragments #1111

Merged
merged 1 commit into from Mar 15, 2024

Conversation

HannesWell
Copy link
Member

For cocoa specific implementations of

  • o.e.swt.accessibility.Accessible tag methods already tagged as @noreference also with @nooverride so that API tools considers them as internal and does not complain about non-API types being referenced in their signature.

  • o.e.swt.graphics.GCData tag all fields as @noreference so that API tools does not complain about non-API types being used in public fields of this actually internal class (tagged as @noreference).

Suppress issue about forbidden extension of Scrollable by Composite.

Copy link
Contributor

github-actions bot commented Mar 13, 2024

Test Results

   299 files  ±0     299 suites  ±0   6m 56s ⏱️ - 1m 14s
 4 099 tests ±0   4 091 ✅ ±0   8 💤 ±0  0 ❌ ±0 
12 209 runs  ±0  12 134 ✅ ±0  75 💤 ±0  0 ❌ ±0 

Results for commit 078581d. ± Comparison against base commit 9149b68.

♻️ This comment has been updated with latest results.

Comment on lines +251 to +344
<resource path="Eclipse SWT/cocoa/org/eclipse/swt/widgets/Composite.java" type="org.eclipse.swt.widgets.Composite">
<filter id="576778288">
<message_arguments>
<message_argument value="Scrollable"/>
<message_argument value="Composite"/>
</message_arguments>
</filter>
</resource>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think APItools are too strict here.
For classes in the same bundle I think it makes sense to permit extending classes tagged with @noextend by default.

@@ -51,19 +51,30 @@ public final class GCData {
public int fillRule = SWT.FILL_EVEN_ODD;
public Image image;

/** @noreference This field is not intended to be referenced by clients. */
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should IMHO also be implied by the @noreference of this class respectively public non-api types should not be an issue.

@HannesWell
Copy link
Member Author

Looks like even with this approach we have to add filters, but they are about previously API fields having become non-API.
I think the advantage in contrast to suppressing the leakage warning is that these filters are temporary and can be removed after the next release.

For cocoa specific implementations of
- o.e.swt.accessibility.Accessible tag methods already tagged as
@noreference also with @nooverride so that API tools considers them as
internal and does not complain about non-API types being referenced in
their signature.

- o.e.swt.graphics.GCData tag all fields as @noreference so that API
tools does not complain about non-API types being used in public fields
of this actually internal class (tagged as @noreference).

Suppress issue about forbidden extension of Scrollable by Composite.
@HannesWell
Copy link
Member Author

Thanks to @Phillipus who tested this PR on a real Mac and verified all API issues on Mac are really fixed with this:

@iloveeclipse
Copy link
Member

@HannesWell : beside the unneeded GCData filters the rest looks good.
Please double check that, remove if you also see the same & feel free to merge.

@HannesWell
Copy link
Member Author

@HannesWell : beside the unneeded GCData filters the rest looks good. Please double check that, remove if you also see the same & feel free to merge.

I used to get those warnings as well, but when I ensured the active workspace target and API-baseline target are properly resolved to one mac environment and the API-baseline is refreshed and after a clean+build they vanished.

Furthermore I have currently also included this change in #1110 , where the build is configured to fail on API warnings and the build passes.
And in #1109 (comment) @Phillipus said he does not have any warning with this.
So as far as I can tell this is all save and sound.

@Phillipus
Copy link
Contributor

And in #1109 (comment) @Phillipus said he does not have any warning with this.

I just double-checked with latest I-build and still no warnings or errors.

@HannesWell HannesWell merged commit d0a8af2 into eclipse-platform:master Mar 15, 2024
13 checks passed
@HannesWell HannesWell deleted the fix-mac-api-issues branch March 15, 2024 17:40
@Phillipus
Copy link
Contributor

@HannesWell Since 7479681 I am now getting errors for SWT such as AccessibleActionListener extends non-API type SWTEventListener. I'm using Eclipse 4.31 as IDE. Before this commit they were warnings. What should I do?

@merks
Copy link
Contributor

merks commented Mar 15, 2024

I think you must use Eclipse SDK 4.32 IDE because. Also, I had problem with missing links to the api filters folder that @iloveeclipse helped me resolve. I ended up having to delete the projects from my workspace an importing them again....

@Phillipus
Copy link
Contributor

Phillipus commented Mar 15, 2024

I think you must use Eclipse SDK 4.32 IDE

Hmmm...tricky. Before now I could use the current version of the IDE (4.31) and import the SWT projects and simply ignore any warnings. Now each time I get the SWT projects I have to set these errors to warnings for the projects settings.

@merks
Copy link
Contributor

merks commented Mar 15, 2024

yes, there was a problem that the API baseline details are missing from the 4.31 release of SWT so they are hard coded in PDE but of course only for 4.32. It's a tangled web we weave....

@Phillipus
Copy link
Contributor

My setup is I have Eclipse 4.31 installed on 6 machines (different OS and architectures) and pull and test the latest SWT project commits and ignore any API warnings. What I don't want to do is also install yet another version of the IDE on 6 machines. ;-) I guess I'll have to keep my own copy of org.eclipse.pde.api.tools.prefs for the SWT projects?

@Phillipus
Copy link
Contributor

I ended up having to delete the projects from my workspace an importing them again....

Strange. I did that and I no longer see the errors. False alarm?

@HannesWell
Copy link
Member Author

yes, there was a problem that the API baseline details are missing from the 4.31 release of SWT so they are hard coded in PDE but of course only for 4.32. It's a tangled web we weave....

Indeed. For the next release cycle this will hopefully be stable again.
But the error desribed should not depend on the baseline but only on the current workspace. So unless the error persists it really could be a false alarm.

My setup is I have Eclipse 4.31 installed on 6 machines (different OS and architectures) and pull and test the latest SWT project commits and ignore any API warnings.

That's a large setup 😮

What I don't want to do is also install yet another version of the IDE on 6 machines. ;-) I guess I'll have to keep my own copy of org.eclipse.pde.api.tools.prefs for the SWT projects?

That's relatable. Making the warnings errors was meant to help developers to notice them early and not only in the CI that fails on API warnings since #1110.
But if it continues to be a problem I can also make the leakage issues warnings again. As said, the CI should then catch new errors.

@Phillipus
Copy link
Contributor

I think it was a one-off. I don't have an API baseline set so I shouldn't see the errors, right?

@HannesWell
Copy link
Member Author

I think it was a one-off. I don't have an API baseline set so I shouldn't see the errors, right?

It dependents on the kind of error. If it said something changed in a wrong way, then a missing baseline might be a problem (but actually without a baseline such issues are actually, falsely, not raised).

errors for SWT such as AccessibleActionListener extends non-API type SWTEventListener.

That exact issues should not depend on the baseline, but who knows what API-tools is doing in its inner-most core. 😅

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.

None yet

4 participants