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

Add back exports to the org.eclipse.swt host #1032

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

tjwatson
Copy link
Contributor

@tjwatson tjwatson commented Feb 5, 2024

Some projects, like BND tools, will avoid using require-bundle at great lengths. This becomes problematic when the APIs they want to use do not version there Export-Package properly.

Many Eclipse projects, including SWT, do not version their API packages. They only version their individual bundles with respect to semantic versioning.

For BND they resorted to using Import-Package with the bundle-version and bundle-symbolic-name of the host. This "worked" because the SWT API packages were defined in the Export-Package header of the host bundle. But now with PR #1008 this changed and the host bundle no longer defines the exported packages. The end result is BND tools is now broken and has many unresolvable bundles.

Until this is sorted out, this PR simply adds back the Export-Package header of the SWT host. The longer term solution should be to properly version SWT API packages so that consumers of the API can properly use Import-Package to depend on the API.

Some projects, like BND tools, will avoid using require-bundle
at great lengths. This becomes problematic when the APIs they
want to use do not version there `Export-Package` properly.

Many Eclipse projects, including SWT, do not version their API
packages.  They only version their individual bundles with respect
to semantic versioning.

For BND they resorted to using `Import-Package` with the
`bundle-version` and `bundle-symbolic-name` of the host. This
"worked" because the SWT API packages were defined in the
`Export-Package` header of the host bundle. But now with
PR eclipse-platform#1008 this changed and the host bundle no longer defines
the exported packages. The end result is BND tools is now
broken and has many unresolvable bundles.

Until this is sorted out, this PR simply adds back the
`Export-Package` header of the SWT host. The longer term
solution should be to properly version SWT API packages
so that consumers of the API can properly use `Import-Package`
to depend on the API.
Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Looks good, thank you Tom.

Copy link
Contributor

github-actions bot commented Feb 5, 2024

Test Results

   299 files  ±0     299 suites  ±0   5m 53s ⏱️ -32s
 4 100 tests ±0   4 092 ✅ ±0   8 💤 ±0  0 ❌ ±0 
12 212 runs  ±0  12 137 ✅ ±0  75 💤 ±0  0 ❌ ±0 

Results for commit d068bc0. ± Comparison against base commit c8d8944.

@HannesWell
Copy link
Member

The quality-gate errors in Jenkins are somewhat expected, since it was setup newly for SWT and has to settle first.

@tjwatson tjwatson merged commit 2ca0b37 into eclipse-platform:master Feb 5, 2024
10 of 13 checks passed
@laeubi
Copy link
Contributor

laeubi commented Feb 6, 2024

For BND they resorted to using Import-Package with the bundle-version and bundle-symbolic-name of the host.

So effectively they using Require-Bundle but just giving it another name ;-)

(yes I know that there are still slight differences)

The longer term solution should be to properly version SWT API packages so that consumers of the API can properly use Import-Package to depend on the API.

an immediate solution would be to simply always export all packages with the bundle version.

@iloveeclipse
Copy link
Member

This change adds now 19 API errors on org.eclipse.swt.gtk.linux.x86_64, saying basically that all workspace types from org.eclipse.swt.internal new and should be tagged with since 3.125 version.

Changing export to org.eclipse.swt.internal;x-internal:=true, fixes that.
Note, originally it was - org.eclipse.swt.internal;x-friends:="org.eclipse.ui,org.eclipse.swt.tools.spies",

I'm alone who sees the problem? Or is this Linux specific?

@merks
Copy link
Contributor

merks commented Feb 6, 2024

I see this now:

image

@HannesWell
Copy link
Member

HannesWell commented Feb 6, 2024

Changing export to org.eclipse.swt.internal;x-internal:=true, fixes that.
Note, originally it was - org.eclipse.swt.internal;x-friends:="org.eclipse.ui,org.eclipse.swt.tools.spies",

Yes adding back org.eclipse.swt.internal as public package was wrong, I oversaw that in the review.

Changing it to org.eclipse.swt.internal;x-internal:=true, should be fine. org.eclipse.swt.tools.spies already suppresses the restriction warnings and org.eclipse.ui seems not to use swt.internal (anymore):
https://github.com/search?q=repo%3Aeclipse-platform%2Feclipse.platform.ui%20path%3A%2F%5Ebundles%5C%2Forg%5C.eclipse%5C.ui%5C%2F%2F%20org.eclipse.swt.internal&type=code

If non one of you adds the x-internal directive back before, I can do it this evening.

@iloveeclipse
Copy link
Member

I see this now

Exact. So it should be fixed => #1038

@tjwatson
Copy link
Contributor Author

tjwatson commented Feb 6, 2024

This change adds now 19 API errors on org.eclipse.swt.gtk.linux.x86_64, saying basically that all workspace types from org.eclipse.swt.internal new and should be tagged with since 3.125 version.

Changing export to org.eclipse.swt.internal;x-internal:=true, fixes that. Note, originally it was - org.eclipse.swt.internal;x-friends:="org.eclipse.ui,org.eclipse.swt.tools.spies",

I'm alone who sees the problem? Or is this Linux specific?

Sorry about that. I was told the x-friends was no longer needed for spies. I didn't notice that it was still needed for org.eclipse.ui. I mistakenly removed all of x-friends. It should still make org.eclipse.ui a friend.

@iloveeclipse
Copy link
Member

Most likely this change also causes following issue: eclipse-platform/eclipse.platform.ui#1654

@laeubi
Copy link
Contributor

laeubi commented Feb 22, 2024

The end result is BND tools is now broken and has many unresolvable bundles.
Until this is sorted out, this PR simply adds back the Export-Package header of the SWT host.

@tjwatson can you explain what component complains here? As I'm currently looking into the resolver I'm also looking what the spec says and there one finds:

The Framework will automatically associate each package export definition with the following attributes:

  • bundle-symbolic-name - The bundle symbolic name of the exporting bundle. In the case of a fragment bundle, this is the host bundle's symbolic name.
  • bundle-version - The bundle version of the exporting bundle. In the case of a fragment bundle, this is the host bundle's version.

For me it looks like the "complainer" here is not respecting that rule and only looks at packages from the host but not consider those contributed by fragments.

@tjwatson
Copy link
Contributor Author

I remember that part of the specification, but I don't believe it to be implementable. At one point I thought I removed that from the core specification. No framework implements that behavior, because I believe it to be unimplementable. And clearly there is no TCK for it.

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.

5 participants