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

FISH-7880 Update Bundle Plugin #6439

Merged
merged 17 commits into from
Nov 6, 2023

Conversation

Pandrex247
Copy link
Member

@Pandrex247 Pandrex247 commented Oct 10, 2023

Description

Updates the maven bundle plugin to 5.1.9.
This requires reconfiguring a lot of OSGi since it introduces changes to the default package import behaviour.

While creating this I realised that for quite a while now the <Export-Package /> configuration to not export anything by default was actually not doing anything, meaning by default we were exporting everything. I've kept this as-is now, but probably worth reviewing if this is correct and why GlassFish originally didn't do so (they do now if that's relevant).

Important Info

Blockers

Finding an OSGi oneiromancer who can properly advise.

Testing

New tests

None

Testing Performed

Built server, started admin console - no OSGi errors.

Testing Environment

Windows 11, Zulu 11.0.20.1 & 21.0.0

Documentation

N/A?

Notes for Reviewers

I've dotted some optional imports around, but I need to review if some of them should be dynamic (or some other thing) instead.

No idea yet why updating bundle plugin means extra non-JDK packages seem to be getting imported now ¯\_(ツ)_/¯

  • org.apache.jasper.compiler - Qué?
  • com.sun.messaging.jmq.admin.jmsspi - I've set this as optional for now, as this comes from OpenMQ. Eclipse GlassFish have removed this "legacy" handler so might also be relevant for us to do the same?
  • org.apache.xerces.impl - Just excluded for now, in the same way GlassFish do. Not sure if this should be optional resolution since presumably something in web-core wants it.
  • opentelemetry-repackaged - I've just excluded dalvik and com.android since they don't seem relevant, but I've left io.opentelemetry.exporter.prometheus as optional since that seems like it might be.
  • org.glassfish.admin.rest.resources.generatedASM - no clue. Generated class lookup from somewhere. GlassFish exclude it so thought I should too.

@Pandrex247 Pandrex247 requested a review from pdudits October 10, 2023 12:18
@Pandrex247 Pandrex247 changed the title FISH-7880 Update Mundle Plugin FISH-7880 Update Bundle Plugin Oct 10, 2023
@Pandrex247 Pandrex247 force-pushed the Update-Bundle-Plugin branch 2 times, most recently from b0ac777 to 5880169 Compare October 11, 2023 15:41
@Pandrex247
Copy link
Member Author

Jenkins test please

@Pandrex247 Pandrex247 force-pushed the Update-Bundle-Plugin branch from 35a7d70 to 105dcbb Compare October 19, 2023 12:23
@Pandrex247
Copy link
Member Author

Jenkins test please

Copy link
Contributor

@pdudits pdudits left a comment

Choose a reason for hiding this comment

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

While creating this I realised that for quite a while now the configuration to not export anything by default was actually not doing anything, meaning by default we were exporting everything. I've kept this as-is now, but probably worth reviewing if this is correct and why GlassFish originally didn't do so (they do now if that's relevant).

If they do it would be probably good to follow suite, as that has been the intent all along. There was of course massive PR from David that replaced the osgi.bundle magic with direct configuration of the plugin, so it's hard to see how exactly that changed.

I'll yet run this through bunana to get full picture

appserver/packager/external/ant/pom.xml Outdated Show resolved Hide resolved
@pdudits
Copy link
Contributor

pdudits commented Oct 19, 2023

Apparently the microprofile update is even newer than 2 hours, as I see quite some differences due to new MP6.1 stuff in the diff. But there are few more:

I fail to create a proper table so just the raw lines of some interesting imports/exports

Bundle	Version	Package	Direction	Min Version	Max Version	Qualifier	Extras
----
fish.payara.server.core.nucleus.kernel	6.9.0.SNAPSHOT	com.sun.tracing	import	<any>	<any> []	false null
fish.payara.server.core.grizzly.nucleus-grizzly-all	6.9.0.SNAPSHOT	sun.security.ssl	import	<any>	<any>	[]	false	null
fish.payara.server.appclient.gf-client-module	6.2023.11.SNAPSHOT	org.glassfish.appclient.client.acc.agent	import	<any>	<any>	[]	false	null
fish.payara.server.core.packager.opentelemetry-repackaged	6.9.0.SNAPSHOT	sun.security.ssl	import	<any>	<any>	[]	false	null
fish.payara.server.core.security.ee	6.9.0.SNAPSHOT	com.sun.enterprise.security.acl	import	6.9.0	7.0.0	[]	false	null

Those sun imports probably need to be all optional.
Strange is that security.ee import that suddenly appears that wasn't there.

And actually, no Java packages appeared, at least from what bunana sees

@Pandrex247
Copy link
Member Author

Jenkins test please

@Pandrex247 Pandrex247 marked this pull request as ready for review October 20, 2023 10:04
@Pandrex247
Copy link
Member Author

Jenkins test please

Copy link
Contributor

@breakponchito breakponchito left a comment

Choose a reason for hiding this comment

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

LGTM

@Pandrex247 Pandrex247 merged commit e38406a into payara:master Nov 6, 2023
@Pandrex247 Pandrex247 deleted the Update-Bundle-Plugin branch November 6, 2023 09:13
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.

3 participants