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 JDK9 plugin when using OSGi #1047

Merged
merged 3 commits into from
Jan 26, 2024

Conversation

Roiocam
Copy link
Member

@Roiocam Roiocam commented Jan 26, 2024

Continue my digging: #1040 (comment)

And found a solution that can easily fix the JDK9 plugin, which changes the JDK9 product's inject place, the original inject place is fullClasspath, but sbt-osgi doesn't use them anymore, so a simple change could fix our problem and without dependency upstream update.

Compile result

image

Unzip Jar

image

@Roiocam
Copy link
Member Author

Roiocam commented Jan 26, 2024

cc @He-Pin @mdedetrich @pjfanning

@He-Pin
Copy link
Member

He-Pin commented Jan 26, 2024

just checked on jdk 21
image
The result is ok, but osgibundle took 115s+ :)

and in osgi plugin
image

@He-Pin He-Pin requested a review from mdedetrich January 26, 2024 03:01
@He-Pin He-Pin added the build sbt or build of the project label Jan 26, 2024
@He-Pin He-Pin added this to the 1.1.0 milestone Jan 26, 2024
Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

lgtm, and thank you. the code match the one in sbt-osgi, will let @mdedetrich take a look

@Roiocam
Copy link
Member Author

Roiocam commented Jan 26, 2024

But looks like it can not package on JDK8 anymore:

截屏2024-01-26 11 15 00

@Roiocam
Copy link
Member Author

Roiocam commented Jan 26, 2024

But looks like it can not package on JDK8 anymore:

Good news, I just fixed this via a simple filter.

Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

Nice find I missed this part, I was trying to solve it another way.

This indeed seems to be correct location where the sbt-osgi workaround was historically done and I wasn't aware of it's existence. In general we need to look at how to solve this problem in a more sane way since the current implementation is really brittle, i.e. whenever sbt-osgi changes how something is packaged any other plugin which modifies the classpath in some way can also be inadvertently effected.

Pinging @eed3si9n since you asked before about this problem specifically incase you are still curious. There might be a case of re-thinking how to solve this problem in a more principled way, possibly for SBT 2.x.x?

@He-Pin
Copy link
Member

He-Pin commented Jan 26, 2024

@Roiocam @mdedetrich I think we can merge this first, any changes can come up after. and I can send a PR to test if the paradox fail with wrong java 9 related doc.

@mdedetrich
Copy link
Contributor

@Roiocam @mdedetrich I think we can merge this first, any changes can come up after. and I can send a PR to test if the paradox fail with wrong java 9 related doc.

I will defer to @Roiocam w/e he thinks is best. @Roiocam If you want us to merge the PR as is so you can make a new one just let me know.

@Roiocam
Copy link
Member Author

Roiocam commented Jan 26, 2024

I will defer to @Roiocam w/e he thinks is best. @Roiocam If you want us to merge the PR as is so you can make a new one just let me know.

I have investigated the problem of protobuf-v3 for a while, and I don't think there is any problem with the existing methods.

Since the sbt-osgi upgrade, there dependency has big change, but sbt-assembly only depends on Compile / fullClasspath, we could make osgiBundle depends on sbt-assembly, but they assembly result won't compact to osgi jar. I think we could make this PR merged, and then use another issue to find out if it has a better solution for sbt-assembly or not.

10a11,12
> [info] 	protobuf-v3 / Compile / dependencyClasspathAsJars
> [info] 	protobuf-v3 / osgiCacheBundle
13d14
< [info] 	protobuf-v3 / Compile / fullClasspath
20a22
> [info] 	protobuf-v3 / Compile / products

@mdedetrich
Copy link
Contributor

Fair enough, I will go ahead and merge this PR and we can discuss this point later if needed.

Thanks again for fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build sbt or build of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants