Bundle manifest in right way

[Roiocam]

Motivation

Rollback to use (Compile / fullClasspath), import manifest via (Compile / internalDependencyAsJars)

Verifed

[info] [error] java.lang.RuntimeException: Expected 'Import-Package: ' and 'proj1;version="[1.2,2)"' in manifest! [info] [error] Import-Package: proj1,scala.reflect;version="[2.12,3)"

Conclusion

Only test-10-multi-project-dependsOn-includePackage-versions will complain, We need additional submissions to fix the packaging of this information. It seems that we have added an additional package name scala.reflect.

Waiting feedback from @mdedetrich

[lefou]

[info] [error] java.lang.RuntimeException: Expected 'Import-Package: ' and 'proj1;version="[1.2,2)"' in manifest! [info] [error] Import-Package: proj1,scala.reflect;version="[2.12,3)"

This does not mean, the scala.reflect import is unexpected! The test only expects the string proj1;version="[1.2,2)" in the manifest. Which means, we generate a properly version constraint on the proj1 import. I think it also depends on scala.reflect, but this is not tested here, as it is not relevant.

After you change, you can see that the import proj1 is unconstrained. It has no further version attribute. So your change is a regression.

[mdedetrich]

As @lefou mentioned, this is a legitimate regression. There is a reason we moved away from fullClassPath, its because at that point in time when sbt evaluates the task metadata for the version ranges is not available and so the sbt manifest headers are missing the version ranges.

I even tried to do alternate solutions like https://github.com/sbt/sbt-osgi/pull/103 but I then hit other problems.

[Roiocam]

After you change, you can see that the import proj1 is unconstrained. It has no further version attribute. So your change is a regression.

Thanks, it is very helpful for understanding how to verify this.

I am just thinking, why don't we just replace the fullClasspath define to (exportedProducts + dependencyClasspathAsJars), I have verified this doesn't break anything about the package version in the manifest (they pass test-10).

By using this approach, we could let osgiBundle continue to depend on fullClasspath, that's means it won't break other plugins that depend on fullClasspath too. e.g. sbt-assembly, pekko-jdk9

What do you think? @lefou

[mdedetrich]

I am just thinking, why don't we just replace the fullClasspath define to (exportedProducts + dependencyClasspathAsJars), I have verified this doesn't break anything about the package version in the manifest (they pass test-10).

If you mean replacing products with exportedProducts at https://github.com/sbt/sbt-osgi/blob/main/src/main/scala/com/github/sbt/osgi/SbtOsgi.scala#L51 then that can theoretically work, there is enough of a test suite that it should catch any regressions and it might mean in pekko's case we don't need the explodedJars and/or https://github.com/apache/incubator-pekko/blob/4968f0279609bcc4d51c421b952ed67b1a15e152/project/Jdk9.scala#L76-L83 workaround

[Roiocam]

If you mean replacing products with exportedProducts at https://github.com/sbt/sbt-osgi/blob/main/src/main/scala/com/github/sbt/osgi/SbtOsgi.scala#L51

Not just replace products, actually the most important part was replacing fullClasspath definition, I have to admit it will break another plugin that also redefine or enrich the fullClasspath. That means we can not change pekko to:

-  Compile / dependencyClasspathAsJars ++= notOnJdk8((CompileJdk9 / exportedProducts).value))
+  Compile / fullClasspath ++= notOnJdk8((CompileJdk9 / exportedProducts).value))

I have to say, as far as I know, this kind of redefining tasks does not have any solution that holds good for all time.

it might mean in pekko's case we don't need the explodedJars and/or https://github.com/apache/incubator-pekko/blob/4968f0279609bcc4d51c421b952ed67b1a15e152/project/Jdk9.scala#L76-L83 workaround

Yes, we need to these changes on Pekko.

-  Compile / dependencyClasspathAsJars ++= notOnJdk8((CompileJdk9 / exportedProducts).value))
+  Compile / exportedProducts ++= notOnJdk8((CompileJdk9 / exportedProducts).value))

And add these into the depend walker check, In case of any change from the future causes the regression of issues.

[Roiocam]

@mdedetrich I just found another way to still make test-10 pass.

      (Compile / exportJars) := false,
-     (Compile / fullClasspath) := concat(Compile / exportedProducts, Compile / dependencyClasspathAsJars).value,
+     (Compile / exportedProducts) ++= (Compile / dependencyClasspathAsJars).value,

That means the sbt-osgi needed to consider three things:

• Only Compile / dependencyClasspathAsJars will contain the dependency package version, how can we use it at the package task?
• We need exportedProducts and dependency, it could be implemented by using fullClasspath, are we on the right way?
• We change the jar by redefining the packageBin, Is there any better approach?

And something we already know:

• If we redefine packageBin, the pekko JDK9 plugin won't work, because it depends on the original packageBin(depend on packageBin / mappings)
• If we redefine packageBin, the assembly plugin won't work either, it uses the same way as sbt-osgi does.
• If we redefine packageBin, enable exportJars, depends fullClasspath, it will form a cycle.
• if OSGi uses the original fullClasspath, the dependency package version will be lost.
• if OSGi discards use fullClasspath as package sources, shouldn't break anything until other plugin depend on it.
• append to task(++=) better than redefine(:=) it, it won't break other plugins which depend on it.

One more bonus: sbt support key rank

[Roiocam]

I think the current implementation is correct, as like as https://github.com/sbt/sbt-osgi/issues/104 discussion.