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

Enforce encapsulation of equinox-launcher Constants and JNIBridge #565

Merged

Conversation

HannesWell
Copy link
Member

@HannesWell HannesWell commented Mar 24, 2024

In the Eclipse SDK there are neither references to org.eclipse.equinox.internal.launcher.Constants nor to org.eclipse.equinox.launcher.JNIBridge.
@tjwatson or anybody else are you aware of external users that need those two classes public at the places where they are?

@HannesWell
Copy link
Member Author

HannesWell commented Mar 24, 2024

Looks like at least the org.eclipse.launcher.tests introduced with #401, needs some adjustments.
But this could maybe be solved by making the test a fragment of the launcher and renaming the TestLauncherApp's package to org.eclipse.equinox.launcher.

Copy link

github-actions bot commented Mar 24, 2024

Test Results

   28 files  ±0     28 suites  ±0   11m 25s ⏱️ -38s
2 170 tests ±0  2 124 ✅ ±0  46 💤 ±0  0 ❌ ±0 
2 242 runs  ±0  2 196 ✅ ±0  46 💤 ±0  0 ❌ ±0 

Results for commit d490910. ± Comparison against base commit a90c944.

♻️ This comment has been updated with latest results.

@HannesWell HannesWell force-pushed the equinox-launcher-encapsulation branch from 55c5963 to 4504ab2 Compare March 25, 2024 07:04
Copy link
Contributor

@tjwatson tjwatson left a comment

Choose a reason for hiding this comment

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

I can't say if I am confident that JNIBridge is not needed outside of the launcher by anyone. Regardless, I don't think it was ever intended to be API. We can move forward here I think.

@HannesWell HannesWell force-pushed the equinox-launcher-encapsulation branch from 4504ab2 to 0b00fc8 Compare March 25, 2024 20:02
@HannesWell HannesWell force-pushed the equinox-launcher-encapsulation branch from 0b00fc8 to d490910 Compare March 25, 2024 20:05
@HannesWell
Copy link
Member Author

I can't say if I am confident that JNIBridge is not needed outside of the launcher by anyone. Regardless, I don't think it was ever intended to be API. We can move forward here I think.

Ok, good.
Since the build passes now, should we submit this or are there concerns by anybody?

@tjwatson
Copy link
Contributor

Ok, good.
Since the build passes now, should we submit this or are there concerns by anybody?

Not from me.

@HannesWell HannesWell merged commit 8975fd3 into eclipse-equinox:master Mar 26, 2024
27 of 28 checks passed
@HannesWell HannesWell deleted the equinox-launcher-encapsulation branch March 26, 2024 20:56
HannesWell added a commit to HannesWell/equinox that referenced this pull request Mar 30, 2024
Because JNIBridge is a package-private class since [1] it cannot be
referenced from external bundles anyway. So there is no need for
API-tags to prohibit that.

[1] - eclipse-equinox#565
HannesWell added a commit to HannesWell/equinox that referenced this pull request Mar 30, 2024
Because JNIBridge is now a package-private class it cannot be referenced
from other bundles anyway. So there is no need for API-tags that
prohibit it.

Follow-up of eclipse-equinox#565
HannesWell added a commit that referenced this pull request Mar 30, 2024
Because JNIBridge is now a package-private class it cannot be referenced
from other bundles anyway. So there is no need for API-tags that
prohibit it.

Follow-up of #565
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

2 participants