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

Update sbt-osgi to 0.10.0 #1063

Merged
merged 1 commit into from
Jan 29, 2024
Merged

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Jan 28, 2024

This PR brings in the latest version of sbt-osgi. Aside from the package name being changed, of note is that since the initialization of keys was properly fixed in sbt/sbt-osgi#121 we now have to correctly enable the SbtOsgi plugin on the projects that actually use sbt-osgi.

The reason why this wasn't needed previously is that keys was being inadvertently initialized without initialising the SbtOsgi plugin as documented because the incorrect keys was being put into projectSettings and we were referencing them in OSGi object.

@mdedetrich
Copy link
Contributor Author

@Roiocam If you want to have a look as well

@@ -467,6 +470,7 @@ lazy val testkit = pekkoModule("testkit")
.settings(AutomaticModuleName.settings("pekko.actor.testkit"))
.settings(OSGi.testkit)
.settings(initialCommands += "import org.apache.pekko.testkit._")
.enablePlugins(SbtOsgi)
Copy link
Member

Choose a reason for hiding this comment

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

Does the tests need osgi too?

Copy link
Contributor Author

@mdedetrich mdedetrich Jan 28, 2024

Choose a reason for hiding this comment

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

It's inherited from the parent project. Note that if it's not done correctly the sbt build won't even load.

The solution is quote simple, whoever OSGi settings were used we now need to enable the plugin explicitly, nothing more or less.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also the tests should not have OSGi enabled anyways, OSGi is about packaging the final jar and we don't package tests


lazy val streamTestkit = pekkoModule("stream-testkit")
.dependsOn(stream, testkit % "compile->compile;test->test")
.settings(Dependencies.streamTestkit)
.settings(AutomaticModuleName.settings("pekko.stream.testkit"))
.settings(OSGi.streamTestkit)
.enablePlugins(SbtOsgi)
Copy link
Member

Choose a reason for hiding this comment

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

So does the kit.

@He-Pin
Copy link
Member

He-Pin commented Jan 28, 2024

Before we can merge this, we should validate the Java 9 related classes is rightly been generated

@mdedetrich
Copy link
Contributor Author

Before we can merge this, we should validate the Java 9 related classes is rightly been generated

Agreed @Roiocam Do you want to do this since you did it last time.

There shouldn't be any issue since this specific issue is pretty black and white (i.e. either sbt loads or it doesn't) but its better to be safe than sorry (again).

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm

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

Copy link
Member

@Roiocam Roiocam left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks.

I have verified this PR with #1065 both (Compile / packageBin) and (OsgiKeys.bundle), it won't break the JDK9 plugin.

pekko > jdk9Check
Package check depend on ProjectRef(file:/Users/roiocam/IdeaProject/incubator-pekko/,actor-typed) / List(ScopedKey(Scope(Select(ProjectRef(file:/Users/roiocam/IdeaProject/incubator-pekko/,actor-typed)), Zero, Zero, Zero),osgiBundle))
Package check depend on ProjectRef(file:/Users/roiocam/IdeaProject/incubator-pekko/,cluster-sharding) / List(ScopedKey(Scope(Select(ProjectRef(file:/Users/roiocam/IdeaProject/incubator-pekko/,cluster-sharding)), Zero, Zero, Zero),osgiBundle))
Package check depend on ProjectRef(file:/Users/roiocam/IdeaProject/incubator-pekko/,remote) / List(ScopedKey(Scope(Select(ProjectRef(file:/Users/roiocam/IdeaProject/incubator-pekko/,remote)), Zero, Zero, Zero),osgiBundle))
Package check depend on ProjectRef(file:/Users/roiocam/IdeaProject/incubator-pekko/,stream) / List(ScopedKey(Scope(Select(ProjectRef(file:/Users/roiocam/IdeaProject/incubator-pekko/,stream)), Zero, Zero, Zero),osgiBundle))

@mdedetrich mdedetrich merged commit 613ccf6 into apache:main Jan 29, 2024
17 of 18 checks passed
@mdedetrich mdedetrich deleted the update-sbt-osgi branch January 29, 2024 02:35
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