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

Use Import-Package instead of Require-Bundle for o.e.c.runtime to prevent importing unnecessary classes #3375

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

testforstephen
Copy link
Contributor

What it does

Based on the discussion in eclipse-equinox/equinox#697 (comment), using Require-Bundle for org.eclipse.core.runtime will pull in all of org.eclipse.osgi which in-turn exposes you to all the JDK packages because this horrible re-export:

https://github.com/eclipse-platform/eclipse.platform/blob/6dd67323474b1b49d541bcd1e4ea8007ab2a36a4/runtime/bundles/org.eclipse.core.runtime/META-INF/MANIFEST.MF#L12C17-L12C87.

In our downstream project, we try to override some part of JRE packages within our fragment of org.eclipse.jdt.core. However, the default JDK packages always take precedence because of this Require-Bundle thing. The equinox team's recommendation is to use Import-Package instead of Require-Bundle for finer control over the imported classes.

How to test

This PR just minimizes the imported packages from the bundle o.e.c.runtime, without affecting the overall functionality of the feature.

@@ -6,6 +6,16 @@ Bundle-Version: 3.40.100.qualifier
Bundle-Activator: org.eclipse.jdt.core.JavaCore
Bundle-Vendor: %providerName
Bundle-Localization: plugin
Import-Package: org.eclipse.core.internal.runtime,
Copy link
Contributor

Choose a reason for hiding this comment

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

why importing an internal package?

Copy link
Contributor

@szarnekow szarnekow Dec 4, 2024

Choose a reason for hiding this comment

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

Also why are these imported without a version constraint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why importing an internal package?

The use of internal packages was not introduced by this PR; it originates from existing code in jdt.core, which already references these internal packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also why are these imported without a version constraint?

A version constraint was added in last revision of this PR.

@jukzi
Copy link
Contributor

jukzi commented Dec 4, 2024

By arguing this PR affect your project you imply that the require-bundle silently is some public API. APIs should not be changed. I do not understand what exactly is going on there, but i have doubts that this is the right way to fix it.

@testforstephen
Copy link
Contributor Author

By arguing this PR affect your project you imply that the require-bundle silently is some public API. APIs should not be changed. I do not understand what exactly is going on there, but i have doubts that this is the right way to fix it.

Both Import-Package and Require-Bundle are standard mechanisms for referencing packages from other bundles. And following https://docs.osgi.org/specification/osgi.core/7.0.0/framework.module.html#i3198951, Import-Package is even the preferred way for wiring bundles. Please clarify what issue do you see with this PR change.

@@ -6,6 +6,16 @@ Bundle-Version: 3.40.100.qualifier
Bundle-Activator: org.eclipse.jdt.core.JavaCore
Bundle-Vendor: %providerName
Bundle-Localization: plugin
Import-Package: org.eclipse.core.internal.runtime,
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on discussion at eclipse-equinox/equinox#697 , it looks like we can add ;bundle-symbolic-name="org.eclipse.core.runtime";bundle-version="[3.29.0, 4.0)" to each Import-Package to keep some versioning.

@testforstephen testforstephen force-pushed the jinbo_eclipsecoreimport branch from f04b14f to 5787754 Compare January 5, 2025 09:07
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.

4 participants