Protobuf v3 osgi #29172

barthorre · 2020-06-03T08:41:09Z

I noticed OSGi header information was missing from the Manifest when installing akka-protobuf-v3 (to be used with akka-cluster) in apache karaf.

I managed to fix my issue with the following changes, but I am quiet new to writing code in SBT, meaning any pointers, concerns, best practices etc. are most welcome

akka-ci · 2020-06-03T08:41:22Z

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

akka-ci · 2020-06-03T08:46:57Z

Test FAILed.

raboof · 2020-06-03T10:05:50Z

/cc @atooni @lefou @briantopping @oheger @Falmarri @cschneider

akka-ci · 2020-06-03T11:46:23Z

Test PASSed.

lefou · 2020-06-03T13:48:50Z

project/OSGi.scala

+      configImport(),
+      "*"),
+    OsgiKeys.exportPackage := Seq("akka.protobufv3.internal.*"),
+    OsgiKeys.privatePackage := Seq("com.google.protobuf")


Looks like we're embedding the com.google.protobuf package here. Is this intentional? Shouldn't we import these instead?

This was indeed intentional, but it should be google.protobuf.* instead of com.google.protobuf, to include the *.proto files instead, no? At least that reflects the current jar's contents.

I think the question is rather if it is intentional to include the classes from (com.)google.protobuf in the akka protobuf jar . With your instruction you are saying, please include that package in my own jar file, but do not export it to the out side world. If there are bundles that require the google protobuf API, they won't see it from the akka bundle (again, which might be was you are intending to do).

There is another thing that strikes me, but it might be ok: You are exporting a package that is marked as "internal". It might be required to make bundles that depend on the protobuf bundle work, but it looks strange. In our own project we never export "internal" packages as they are not for public consumption.

Perhaps you can validate that your bundle works in Karaf - then the suggested changes are ok from my side as well.

Quick scan here, it looks like there's JAR shading going on in the build, in combination with the OSGi embedding, has anyone confirmed that com.google.protobuf is not being included twice at separate package roots? EDIT: I see that the packaging was changed to only include the protobuf descriptors in b765835.

@atooni It was indeed intended to include the protobuf descriptors in the jar, since that was the existing behaviour. As of why they are in there to begin with, I'm not sure. My guess would be the included compiler needs them for adhoc stuff, so I don't see a problem such keeping them in the private package.

I did a small test of creating an akka cluster with 2 nodes each running in a different bundle inside karaf and that works just fine. If needed I can provide a features.xml I assembled to make it run locally.

Not sure if this was for me, my original comment was to make sure there was not two copies of the dependency contents, one shaded and one not. Since it's just the descriptors, an oversight is less of a big deal IMO. Correctness is best tho and it would be good to dump the jar and make sure nothing unexpected is duplicated.

So, why do we export the *.proto files again? Aren't these only useful at compile time? If we only need them to be present in the jar file they can also be marked as Private-Package. I don't see the need for exporting them at runtime.

atooni · 2020-06-03T17:38:15Z

@barthorre You might be aware of #28304. I would be curious about your use case of using akka in OSGi. If you have a minute or two, perhaps you could leave a comment in the issue quoted as we are currently investigating how we can continue with Akka under OSGi in our own project.

akka-ci · 2020-06-03T17:52:34Z

Test PASSed.

oheger · 2020-06-04T20:13:26Z

I checked the protobuf-v3 bundle produced with these changes with my setup based on Apache Felix and can confirm that it is working as expected. So the content of the bundle and the meta data seem to be fine.

atooni

After it has been confirmed that the suggested change works in an OSGi container I am happy with the change

raboof

thanks for checking!

patriknw · 2020-06-15T11:43:05Z

I compared the contents of the jar from this PR with corresponding jar in master. Looking good. Only difference is MANIFEST.MF

before (master):

Manifest-Version: 1.0
Automatic-Module-Name: akka.protobuf.v3
Implementation-Title: akka-protobuf-v3
Implementation-URL: https://akka.io/
Implementation-Vendor-Id: com.typesafe.akka
Implementation-Vendor: Lightbend Inc.
Implementation-Version: 2.6.6+25-17f7f110
Specification-Title: akka-protobuf-v3
Specification-Vendor: Lightbend Inc.
Specification-Version: 2.6.6+25-17f7f110

after (this PR):

Manifest-Version: 1.0
Automatic-Module-Name: akka.protobuf.v3
Bnd-LastModified: 1592220269523
Bundle-Description: Akka Protobuf V3 is a shaded version of the protob
 uf runtime. Original POM: https://github.com/protocolbuffers/protobuf
 /blob/v3.9.0/java/pom.xml
Bundle-DocURL: https://doc.akka.io/api/akka/2.6.5+112-b7658350
Bundle-License: https://www.apache.org/licenses/LICENSE-2.0.html;descr
 iption=Apache-2.0
Bundle-ManifestVersion: 2
Bundle-Name: akka-protobuf-v3
Bundle-SymbolicName: com.typesafe.akka.protobuf.v3
Bundle-Vendor: Lightbend Inc.
Bundle-Version: 2.6.5.b7658350
Created-By: 11.0.5 (AdoptOpenJDK)
Export-Package: akka.protobufv3.internal;version="2.6.5.b7658350",akka
 .protobufv3.internal.compiler;uses:="akka.protobufv3.internal";versio
 n="2.6.5.b7658350"
Implementation-Title: akka-protobuf-v3
Implementation-URL: https://akka.io/
Implementation-Vendor: Lightbend Inc.
Implementation-Vendor-Id: com.typesafe.akka
Implementation-Version: 2.6.5+112-b7658350
Import-Package: akka.protobufv3.internal
Private-Package: google.protobuf.compiler,google.protobuf
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version>=1.8))
 "
Specification-Title: akka-protobuf-v3
Specification-Vendor: Lightbend Inc.
Specification-Version: 2.6.5+112-b7658350
Tool: Bnd-4.0.0.201805111645

The google/protobuf contains some .proto files, and those are probably not important.

patriknw

LGTM

barthorre added 3 commits March 18, 2020 15:58

register ActorSystem reference under ActorFactoryRef interface name

9c3f2e8

reformat code

39dde15

add OSGi headers for akka-protobuf-v3

c62d365

akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Jun 3, 2020

barthorre added 2 commits June 3, 2020 12:27

Merge remote-tracking branch 'upstream/master'

1dd808a

Merge branch 'master' into protobuf-v3-osgi

5127074

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Jun 3, 2020

lefou reviewed Jun 3, 2020

View reviewed changes

change private package to only include proto files

b765835

akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Jun 3, 2020

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Jun 3, 2020

patriknw added 2 - pick next Used to mark issues which are next up in the queue to be worked on. The tag is non-binding and removed 2 - pick next Used to mark issues which are next up in the queue to be worked on. The tag is non-binding labels Jun 5, 2020

atooni reviewed Jun 9, 2020

View reviewed changes

atooni approved these changes Jun 9, 2020

View reviewed changes

raboof approved these changes Jun 10, 2020

View reviewed changes

patriknw approved these changes Jun 15, 2020

View reviewed changes

patriknw merged commit 2c4e114 into akka:master Jun 15, 2020

patriknw mentioned this pull request Jun 22, 2020

Remove OSGi support #28304

Closed

patriknw added this to the 2.6.7 milestone Jun 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protobuf v3 osgi #29172

Protobuf v3 osgi #29172

barthorre commented Jun 3, 2020

akka-ci commented Jun 3, 2020

akka-ci commented Jun 3, 2020

raboof commented Jun 3, 2020

akka-ci commented Jun 3, 2020

lefou Jun 3, 2020

barthorre Jun 3, 2020 •

edited

Loading

atooni Jun 3, 2020

briantopping Jun 3, 2020 •

edited

Loading

barthorre Jun 3, 2020 •

edited

Loading

briantopping Jun 3, 2020 •

edited

Loading

lefou Jun 10, 2020

atooni commented Jun 3, 2020

akka-ci commented Jun 3, 2020

oheger commented Jun 4, 2020

atooni left a comment

raboof left a comment

patriknw commented Jun 15, 2020

patriknw left a comment

Protobuf v3 osgi #29172

Protobuf v3 osgi #29172

Conversation

barthorre commented Jun 3, 2020

akka-ci commented Jun 3, 2020

akka-ci commented Jun 3, 2020

raboof commented Jun 3, 2020

akka-ci commented Jun 3, 2020

lefou Jun 3, 2020

Choose a reason for hiding this comment

barthorre Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

atooni Jun 3, 2020

Choose a reason for hiding this comment

briantopping Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

barthorre Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

briantopping Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

lefou Jun 10, 2020

Choose a reason for hiding this comment

atooni commented Jun 3, 2020

akka-ci commented Jun 3, 2020

oheger commented Jun 4, 2020

atooni left a comment

Choose a reason for hiding this comment

raboof left a comment

Choose a reason for hiding this comment

patriknw commented Jun 15, 2020

patriknw left a comment

Choose a reason for hiding this comment

barthorre Jun 3, 2020 •

edited

Loading

briantopping Jun 3, 2020 •

edited

Loading

barthorre Jun 3, 2020 •

edited

Loading

briantopping Jun 3, 2020 •

edited

Loading