search

sbt / sbt-osgi

sbt plugin for creating OSGi bundles
Apache License 2.0
47 stars 43 forks source link

Fix regression when using Jar's to force proper manifest files #102

Closed mdedetrich closed 7 months ago

mdedetrich commented 8 months ago

So I did some digging to figure out the root cause of https://github.com/apache/incubator-pekko/issues/757 and I managed to bisect the change which caused the regression which happens to be https://github.com/sbt/sbt-osgi/pull/64 . When using

log.info("CONTENT IS")
log.info(content.mkString("\n"))

at https://github.com/sbt/sbt-osgi/blob/main/src/main/scala/com/typesafe/sbt/osgi/Osgi.scala#L186 and sbt protobuf-v3/osgiBundle within pekko to diagnose the content that is being packaged, without https://github.com/sbt/sbt-osgi/pull/64 you get

[info] CONTENT IS
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/BufferAllocator.class,org/apache/pekko/protobufv3/internal/BufferAllocator.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/ApiOrBuilder.class,org/apache/pekko/protobufv3/internal/ApiOrBuilder.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/RepeatedFieldBuilderV3$MessageExternalList.class,org/apache/pekko/protobufv3/internal/RepeatedFieldBuilderV3$MessageExternalList.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/Protobuf.class,org/apache/pekko/protobufv3/internal/Protobuf.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache,org/apache)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/DescriptorProtos$EnumOptions.class,org/apache/pekko/protobufv3/internal/DescriptorProtos$EnumOptions.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/NewInstanceSchemaFull.class,org/apache/pekko/protobufv3/internal/NewInstanceSchemaFull.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/CanIgnoreReturnValue.class,org/apache/pekko/protobufv3/internal/CanIgnoreReturnValue.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/SourceContextOrBuilder.class,org/apache/pekko/protobufv3/internal/SourceContextOrBuilder.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/StringValue$1.class,org/apache/pekko/protobufv3/internal/StringValue$1.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/CodedInputStream$1.class,org/apache/pekko/protobufv3/internal/CodedInputStream$1.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/EmptyOrBuilder.class,org/apache/pekko/protobufv3/internal/EmptyOrBuilder.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/DescriptorProtos$MessageOptionsOrBuilder.class,org/apache/pekko/protobufv3/internal/DescriptorProtos$MessageOptionsOrBuilder.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/Method.class,org/apache/pekko/protobufv3/internal/Method.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/RpcController.class,org/apache/pekko/protobufv3/internal/RpcController.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/UninitializedMessageException.class,org/apache/pekko/protobufv3/internal/UninitializedMessageException.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/DescriptorProtos$SourceCodeInfo$Location$1.class,org/apache/pekko/protobufv3/internal/DescriptorProtos$SourceCodeInfo$Location$1.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/CheckReturnValue.class,org/apache/pekko/protobufv3/internal/CheckReturnValue.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/UInt32ValueOrBuilder.class,org/apache/pekko/protobufv3/internal/UInt32ValueOrBuilder.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/BinaryWriter$UnsafeHeapWriter.class,org/apache/pekko/protobufv3/internal/BinaryWriter$UnsafeHeapWriter.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/DescriptorProtos$EnumValueOptionsOrBuilder.class,org/apache/pekko/protobufv3/internal/DescriptorProtos$EnumValueOptionsOrBuilder.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/Syntax$1.class,org/apache/pekko/protobufv3/internal/Syntax$1.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/ByteString$2.class,org/apache/pekko/protobufv3/internal/ByteString$2.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/Int32Value$Builder.class,org/apache/pekko/protobufv3/internal/Int32Value$Builder.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/Option.class,org/apache/pekko/protobufv3/internal/Option.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/DynamicMessage.class,org/apache/pekko/protobufv3/internal/DynamicMessage.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/Type.class,org/apache/pekko/protobufv3/internal/Type.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/DescriptorProtos$MessageOptions$1.class,org/apache/pekko/protobufv3/internal/DescriptorProtos$MessageOptions$1.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/ListFieldSchema.class,org/apache/pekko/protobufv3/internal/ListFieldSchema.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/MessageInfoFactory.class,org/apache/pekko/protobufv3/internal/MessageInfoFactory.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/MapEntryLite$Metadata.class,org/apache/pekko/protobufv3/internal/MapEntryLite$Metadata.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google/protobuf/type.proto,google/protobuf/type.proto)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/Int32Value.class,org/apache/pekko/protobufv3/internal/Int32Value.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/TimestampProto.class,org/apache/pekko/protobufv3/internal/TimestampProto.class)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/org/apache/pekko/protobufv3/internal/Duration$1.class,org/apache/pekko/protobufv3/internal/Duration$1.class)
... (truncated)

where as with https://github.com/sbt/sbt-osgi/pull/64 you get 

[info] CONTENT IS
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir,)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google,google)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google/protobuf,google/protobuf)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google/protobuf/timestamp.proto,google/protobuf/timestamp.proto)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google/protobuf/field_mask.proto,google/protobuf/field_mask.proto)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google/protobuf/api.proto,google/protobuf/api.proto)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google/protobuf/duration.proto,google/protobuf/duration.proto)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google/protobuf/struct.proto,google/protobuf/struct.proto)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google/protobuf/wrappers.proto,google/protobuf/wrappers.proto)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google/protobuf/source_context.proto,google/protobuf/source_context.proto)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google/protobuf/any.proto,google/protobuf/any.proto)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google/protobuf/type.proto,google/protobuf/type.proto)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google/protobuf/empty.proto,google/protobuf/empty.proto)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google/protobuf/compiler,google/protobuf/compiler)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google/protobuf/compiler/plugin.proto,google/protobuf/compiler/plugin.proto)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/google/protobuf/descriptor.proto,google/protobuf/descriptor.proto)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/META-INF,META-INF)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/META-INF/LICENSE,META-INF/LICENSE)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/META-INF/DISCLAIMER,META-INF/DISCLAIMER)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/META-INF/COPYING.protobuf,META-INF/COPYING.protobuf)
[info] (/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/pekko-protobuf-v3_2.13-1.1.0-M0-1-SNAPSHOT.jar_tmpdir/META-INF/NOTICE,META-INF/NOTICE)

As you can see the change means that all of the .class files end up getting stripped out of the osgi-bundle.

If we do some more digging we can see what is causing the regression, if I you do show protobuf-v3/Compile/fullClasspath which is what was used without https://github.com/sbt/sbt-osgi/pull/64 you get

[info] * Attributed(/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/stripped/stripped/pekko-protobuf-v3-assembly-1.1.0-M0-1-SNAPSHOT.jar)
[info] * Attributed(/Users/mdedetrich/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/google/protobuf/protobuf-java/3.20.3/protobuf-java-3.20.3.jar)

where as if you do show protobuf-v3/Compile/dependencyClasspathAsJars (which is with https://github.com/sbt/sbt-osgi/pull/64) you get

[info] * Attributed(/Users/mdedetrich/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/google/protobuf/protobuf-java/3.20.3/protobuf-java-3.20.3.jar)

The Attributed(/Users/mdedetrich/github/incubator-pekko/protobuf-v3/target/scala-2.13/stripped/stripped/pekko-protobuf-v3-assembly-1.1.0-M0-1-SNAPSHOT.jar) happens to be missing.

This is very likely a result of the bespoke way that we generate the jar for protobuf-v3 (see https://github.com/apache/incubator-pekko/blob/main/build.sbt#L359-L387)

@lefou Since you implemented https://github.com/sbt/sbt-osgi/pull/64 you know what the underlying motivation for the change. At least on a glance to me it seems like a quick way to fix this is to combine both (dependencyClasspathAsJars in Compile).value.map(_.data) ++ (products in Compile).value and (fullClasspath in Compile) but use (dependencyClasspathAsJars in Compile).value.map(_.data) ++ (products in Compile).value as a preference which should still solve the underlying problem described in https://github.com/sbt/sbt-osgi/issues/60 .

@raboof Maybe you can provide some underlying context here regarding the implementation of the protobuf-v3 sbt module.

@romainreuillon If you have time to look into this that would also be great.

lefou commented 8 months ago

I only use sbt occasionally and am not familiar with it's internals any longer. At the time I send the PRs, I was in need for a solution for #60 but lacked the detailed understanding (I think I made that clear in my communications at that time and was even searching for help in the chats). I ended up with #61 and #64, which worked for us but, as it turned out, was not the best solution. I tried to come up with a executable integration test case, which is IMHO the key to a proper plugin implementation. Try to reproduce any bugs as test cases and make them succeed.

That being said, I'm sorry that I can't help with any sbt specifics. I can tell that bnd (the OSGI bundle creator tool) needs to see the OSGi Bundles of the dependencies. Just feeding it the classfiles without the proper OSGi Manifests is not enough. Only the manifests contain the exported package versions, which are needed to calculate proper import package version constraints. The essential change I tried to make in PR #64 was to replace some directories with classfiles (the dependencies) with their JARs containing proper OSGi manifests.

TBH, my confidence, that this issue can be solved in a clean way is rather small. My solution failed in a very race-y way. There seems to be no API in sbt, that ensures that the result of tasks, once they are done, stays stable. I have no idea (and also did not receive any helpful idea from others) how this can be fixed. My advive for any dev who targets OSGi, don't do it with sbt. My OSGi projects went a lot smother since they switched over to Mill. Don't get me wrong. I don't want to discredit sbt or sbt-osgi. It's a very capable build tool driving many successful projects. But there is no intersection with OSGi knowledge anymore. Given how long the issue is known and there is no clear idea how to fix it, it's unlikely that a fix is easy and prompt.

And to leave with a more constructive note. Be prepared to deep dive into sbt internals yourself. Maybe a working solution needs a bit out-of-the-box thinking, trading performance for correctness. The essential task is pretty easy. Just get the jars of your dependencies and feed it to bnd. TBH, I don't know why it is so damn hard to get a grab on these artifacts trough the sbt dependency graph. There must be a simple solution.

mdedetrich commented 8 months ago

So I think I have a little bit of a better understanding of whats going on. Specifically in the case of pekko, we are dealing with a farjar from sbt-assembly which by definition doesn't really have dependencies (hence why its not included in dependencyClasspathAsJars). I have resolved this by adding the jar to OsgiKeys.explodedJars (see https://github.com/mdedetrich/incubator-pekko/blob/24ada111620b6a482f0003e8a31504830fa75f03/project/OSGi.scala#L85-L90). So in this case Import-Package is just * but I believe that is correct.

This means that at least for this issue specifically, its not to do with the racey issues with sbt as you describe but rather the distinction between jars that have dependencies vs jars that don't (which does raise other questions regarding how should treat sbt-osgi when used by other plugins like sbt-assembly). With that being said this does raise issues regarding knowing if the result of an sbt task is "dirty" or not but there isn't much that I can do here (maybe @eed3si9n can comment), moving to mill at this point for Pekko is out of the question.