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

Add multi-release processing support #5350

Closed
wants to merge 1 commit into from

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Aug 19, 2022

Currently bnd is not a capable of processing multi-release jars, this
adds support of multi-release processing by a new directive -release
where one can select what release version should be processed by bnd.

Fixes #5346

@laeubi laeubi force-pushed the multi_release_light branch 3 times, most recently from 1b203a5 to 00f2d33 Compare August 19, 2022 12:41
Copy link
Member

@bjhargrave bjhargrave left a comment

Choose a reason for hiding this comment

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

Putting release state in the Jar object is not a good idea. The -release option seems also not proper since it causes a single manifest to be calculated based upon the value when you really need a supplement manifest for each release version and also the base manifest.

I think we need to think about the proper overall approach to building multi-release jars with Bnd before we add new code which may not be helpful.

@laeubi
Copy link
Contributor Author

laeubi commented Aug 19, 2022

Putting release state in the Jar object is not a good idea.

Why not? it is a property of the jar

The -release option seems also not proper since it causes a single manifest to be calculated based upon the value when you really need a supplement manifest for each release version and also the base manifest.

This is a different use-case, and it ist still possible to do so using multiple invocation of BND.

I think we need to think about the proper overall approach to building multi-release jars with Bnd before we add new code which may not be helpful.

This code is helpful as it enables BND to correctly process a multi-release jar. Currently BND simply fails and/or produces wrong results.

@timothyjward
Copy link
Contributor

I think we need to think about the proper overall approach to building multi-release jars with Bnd before we add new code which may not be helpful.

This code is helpful as it enables BND to correctly process a multi-release jar. Currently BND simply fails and/or produces wrong results.

Would it not make sense to start by teaching bnd to read a multi-release bundle? At the moment there's no way to get the "effective manifest" for a given Java version, or to know which versions have custom code. This is important for indexing/resolving and baselining, and will lay some basic API that can be applied to bundle creation.

We can also use the learning from this step to help guide the user usage pattern e.g.

  • is it one bnd file, or one bnd file per release folder?
  • is it a single process to generate the manifests or or multiple process steps?
  • I'm sure there are more...

The -release option seems also not proper since it causes a single manifest to be calculated based upon the value when you really need a supplement manifest for each release version and also the base manifest.

This is a different use-case, and it ist still possible to do so using multiple invocation of BND.

It's not totally clear what the use case for the -release option in the bnd file is. As BJ says the manifest generated using it doesn't seem to be useful as you can't use it in the bundle that you have just analysed. What are you using it for?

@laeubi
Copy link
Contributor Author

laeubi commented Aug 22, 2022

Would it not make sense to start by teaching bnd to read a multi-release bundle?

This PR actually do this, it "teaches" BND how to read resources for a given release, as I can "teach" javac to compile and process classpath entries for a given target release.

At the moment there's no way to get the "effective manifest" for a given Java version, or to know which versions have custom code. This is important for indexing/resolving and baselining, and will lay some basic API that can be applied to bundle creation.

All this can be implemented on top of this change as it provides everything one needs (I even implemented parts of this but jsut dropped this to get a minimal change that could be accepted by bnd ...)

We can also use the learning from this step to help guide the user usage pattern e.g.

Sure why not, at the moment, this change at least allows:

  • one or multiple bnd files per release
  • one process step for each manifest / release your target bundle should include

It's not totally clear what the use case for the -release option in the bnd file is.

Then maybe it would help to explain whats specifically unclear as I added some documentation it should be easy to ask more specific questions than "seems also not proper", for anyone compiling for different targets with the --target option in javac (either directly or indirectly via maven) it should be quite obvious here.

As BJ says the manifest generated using it doesn't seem to be useful as you can't use it in the bundle that you have just analysed.

Why should I'm not be able to use that?

I can use that to:

What are you using it for?

See ClickHouse/clickhouse-java#1016 but that's just one example as other libs are using Multi-Release as well.

@timothyjward
Copy link
Contributor

Again - I think that we're starting too big with this change. I think we should start by making the Analyzer able to correctly read a multi-release bundle, finding the correct resources and returning the correct manifest information (specifically handling Import-Package, Require-Bundle and Require-Capability. Once bnd can read the bundles correctly it will be easier to generate them correctly.

I can use that to:

Why is bnd looking at the content of your dependencies? Are you repackaging the Multi-Release dependency? If so your bundle is Multi-Release and the generated manifest would most likely be wrong. If you aren't repackaging the dependency then bnd should not need to read that jar file.

This is pre-judging the outcome of the discussions I outlined above. Is having multiple bnd files the right approach here? I'm not sure that it is. Is it necessary to add the -release flag? You could just set up your build to generate different folders with the right classpath layouts (i.e. copying the META-INF/versions/X into the main source). Finally, the multi-release manifests generated will be "wrong" as they will contain headers other than the legal Import-Package, Require-Bundle and Require-Capability.

  • If I think all this MR is just stupid stuff and I never need it I can simply not pass the option and get current BND behaviour

I'm all for a flag to enable the behaviour, I guess I'm not sure why the flag isn't Multi-Release: true.

Sorry if this sounds negative, as it absolutely isn't intended to be. I'm really excited that someone finally has the time and resource to get the ball rolling with Multi-Release. I've wanted to do it for quite a while. The main point that I'm trying to make is that Multi-Release has been a Java tooling disaster for quite a long time. I want what bnd does to be easy to use, and to make sense. That includes discussing and deciding stuff like:

  • How do I enable Multi-Release processing for my bundle
  • How do I Private-Package my multi-release content into my bundle
  • How do I customise my multi-release manifests
  • How do I represent a Multi-Release bundle in a repository
  • ...

It's a big important change and we need to make sure that we get the right result for users. That's why I'm keen for the non-contentious stuff (namely correctly reading a bundle that's already Multi-Release) to start first. If we can get the ball rolling in the right direction then I think the next steps will be a lot easier.

@laeubi
Copy link
Contributor Author

laeubi commented Aug 22, 2022

I think that we're starting too big with this change

This adds about 50 LOC (not counting documentation and test) so how "small" do you think such a change you describe here (with a much broader context) could become?

Why is bnd looking at the content of your dependencies?

Because it analyzes the classpath ...

private void analyzeContent() throws Exception {
// Parse all the classes in the
// the jar according to the OSGi Bundle-ClassPath
analyzeBundleClasspath();
//
// Get exported packages from the
// entries on the classpath and
// collect any contracts
//
for (Jar current : getClasspath()) {
getManifestInfoFromClasspath(current, classpathExports, contracts);
Manifest m = current.getManifest();
if (m == null) {
for (String dir : current.getDirectories()
.keySet()) {
learnPackage(current, "", getPackageRef(dir), classpathExports);
}
}
}
addDefinedContracts();
// Handle the bundle activator
String s = getProperty(BUNDLE_ACTIVATOR);
if (s != null && !s.isEmpty()) {
activator = getTypeRefFromFQN(s);
referTo(activator);
logger.debug("activator {} {}", s, activator);
}
// Conditional packages
doConditionalPackages();
}

.. and BND Analyzer plugins do this as well (e.g. JPMS)

then bnd should not need to read that jar file

BND seem to reads lot of context, e.g. packages, annotations present and so on from the classpath.

Is having multiple bnd files the right approach here?

BND support this already, so what should be "judged" here? If later on one comes up with an approach that allows to use a single one, why not? That do not limit us here in any way.... for sure we can "think" and "discuss" another five years it just seems no one really cares enough to do so...

If so your bundle is Multi-Release and the generated manifest would most likely be wrong

Why? I really would appreciate to discuss on real issues than on "feelings" as I have successfully be able to produce a result that looks correct. It produces a manifest that contains more than necessary, but I think we do not need to optimize for size at this stage ...

Is it necessary to add the -release flag?

in contrast to what? How should BND know my desired target release otherwise?

You could just set up your build to generate different folders with the right classpath layouts (i.e. copying the META-INF/versions/X into the main source)

This is done here and BND simply fails to understand this and instead complains that classes are on the wrong folder ;-)

Finally, the multi-release manifests generated will be "wrong" as they will contain headers other than the legal Import-Package, Require-Bundle and Require-Capability

The doc states that

Any other headers in the supplemental manifest file must be ignored.

So I can but any header in I want and it is still valid to do so, but OSGi will ignore them

I'm all for a flag to enable the behaviour, I guess I'm not sure why the flag isn't Multi-Release: true.

Because for the moment I wanted to make it as simple / small as possible, if anyone thinks to turn it on/of by this header I'm happy to enhance the PR ...

Sorry if this sounds negative, as it absolutely isn't intended to be.

Its fine as long as there is some path forward, but at the moment it feels a bit like we are searching reasons not to do it, in contrast to what do we need to make this happen ...

The main point that I'm trying to make is that Multi-Release has been a Java tooling disaster for quite a long time

Yes that's for sure an issue, but it seems there are quite a few (and with Java faster evolving even becoming more) projects that mastering this and its a shame to tell them they can't use BND because MR is not supported. That's why I tried to take the opportunity of a real world example (that is clickhouse) that is open source (so everyone can take a look that this not just a theoretical use-case) to actually try if this can work.

Of course this is not the solution that solves everything but at laest there is a way that could be used as a basis to improve MR support in BND that is:

  1. understanding MR manifests correctly (especially for dependencies!)
  2. maybe provide more comfortable setup e.g. auto detetection of version present in the jar ... maybe even in dependencies on the classpath because this could influence the outcome even if I'm not building for Java 17 in this case
  3. make bnd-plugins (especially JPMS) more aware
  4. improving the tooling (BNDtools, Eclipse, Maven, Gradle, ...)
  5. ...

Still I prefer smaller steps that can happen and do not solve *everything in contrast to something that solves *all but never happens...

That includes discussing and deciding stuff like

So important for me would then who decides when so it does not end like #2227 where many things are discussed but then nothing happens at all (or at laest: someone from the outside can not see any progress for a long time).

That's why I'm keen for the non-contentious stuff (namely correctly reading a bundle that's already Multi-Release) to start first

Well since there is no tool to create one its unlikely one will build one and because there is no such artifact build we not start to develop the tool for creating one ;-)

If we can get the ball rolling in the right direction then I think the next steps will be a lot easier.

This PR is the ball to get this rolling ...

@timothyjward
Copy link
Contributor

timothyjward commented Aug 22, 2022

I think that we're starting too big with this change

This adds about 50 LOC (not counting documentation and test) so how "small" do you think such a change you describe here (with a much broader context) could become?

There's a difference between code size and the impact of the change. A 500 LOC change that did the "reading" part would be a much "smaller" change to the usage of bnd.

Why is bnd looking at the content of your dependencies?

Because it analyzes the classpath ...

For this code to apply the jar is no longer a dependency, it's part of your bundle. This is why we need to teach bnd to read multi-release items first, then work out how users should generate multi-release manifests.

then bnd should not need to read that jar file

BND seem to reads lot of context, e.g. packages, annotations present and so on from the classpath.

This is true, annotations/classes referenced inside the bundle are searched for so that version ranges (and other metadata) can be found. Bnd doesn't need to fully process (i.e. generate anything for) those jars though, just load any existing bundle manifests.

Sorry if this sounds negative, as it absolutely isn't intended to be.
Its fine as long as there is some path forward, but at the moment it feels a bit like we are searching reasons not to do it, in contrast to what do we need to make this happen ...

I really do want this to happen. I'm trying to tell you that I think the best way to get started is to get the read case working. It doesn't require any bnd instructions and is much more likely to get past the maintainers who are naturally wary of new user facing API (there have been plenty of screw-ups in the past).

Once bnd can successfully read a MR bundle and tell you what the correct manifest is for a given Java version (and probably also the Java versions that have MR data) then we'll have a base to design the processing side in an issue, which is how this is normally done.

Still I prefer smaller steps that can happen and do not solve *everything in contrast to something that solves *all but never happens...
So important for me would then who decides when so it does not end like #2227 where many things are discussed but then nothing happens at all (or at laest: someone from the outside can not see any progress for a long time).

The reason that these issues stalled is simply that nobody has had the resource to make them happen. As a team the bnd maintainers are pretty good at responding to comments in issues, and suggesting improvements to things. What they normally lack is the time to implement stuff (unfortunately nobody is paid to maintain bnd).

I'm really happy that you're here to pick up the ball, and I'll be doing my best to make sure this stuff gets merged, but the design matters, and right now there's been no consensus on the design.

That's why I'm keen for the non-contentious stuff (namely correctly reading a bundle that's already Multi-Release) to start first

Well since there is no tool to create one its unlikely one will build one and because there is no such artifact build we not start to develop the tool for creating one ;-)

There are some, but not many. The test jars will need to be hand-created, but this has always been the case for this sort of new feature in bnd.

@laeubi
Copy link
Contributor Author

laeubi commented Aug 23, 2022

@bjhargrave I have now moved the release flag processing entirely into the Analyzer
@timothyjward I think you are correct about the classpath part and the jar content is only processed in some circumstances, so this is not really required for processing the analyzed jar but could be important for other issues, but this might better be addressed in another PR.

It doesn't require any bnd instructions and is much more likely to get past the maintainers who are naturally wary of new user facing API

My last approach was not introducing an instructions in the BND file, but then it was argued there must be one so it could be used out-of-the-box in all context, so this is rather confusing to me...

Once bnd can successfully read a MR bundle and tell you what the correct manifest is for a given Java version (and probably also the Java versions that have MR data) then we'll have a base to design the processing side in an issue, which is how this is normally done.

I'll add reading support as well ... but adding reading support without having a chance to use that information to create a correct jar seems a bit useless... That's why I came up with a -release flag so I can tell BND to use that release for reading manifest/resources and then one can use this for adding reading support writing support and even "bnd makes everything automatic" support :-)

@timothyjward
Copy link
Contributor

I'll add reading support as well ... but adding reading support without having a chance to use that information to create a correct jar seems a bit useless...

The reading support is actually quite an important first step. For example reading support (but not generation support) is necessary for indexing and resolving to work properly with MR jars. For example Aries JAX-RS Whiteboard currently resolves incorrectly on Java 9+ as it has a different set of imported packages in META-INF/versions/9/MANIFEST.MF which are ignored by bnd. When we resolve we know the target java version, so we could fix this problem easily using read support that allows us to pass a target EE.

This probably means enhancing the following enum members to know the appropriate version folder which applies to them.
https://github.com/bndtools/bnd/blob/60392b5a7e27b2012ac476204f1477a9c6e01f69/biz.aQute.bndlib/src/aQute/bnd/build/model/EE.java

That's why I came up with a -release flag so I can tell BND to use that release for reading manifest/resources

I think my main concern with the proposed release instruction (which I believe is shared by @bjhargrave) is that it is a very low-level thing to expose to the user, and the use-case for it isn't that well defined. I've seen your examples of it being used, but I'm still not 100% sure what the start point and the goal of those use cases are. It may well be that there is a different solution which better fits what a user is trying to achieve, or that we can correctly identify what to do without the release instruction using information that we already hold.

@laeubi
Copy link
Contributor Author

laeubi commented Aug 23, 2022

or example Aries JAX-RS Whiteboard currently resolves incorrectly on Java 9+ as it has a different set of imported packages in META-INF/versions/9/MANIFEST.MF which are ignored by bnd

Can you give a link to the exact problematic dependency version?

I've seen your examples of it being used, but I'm still not 100% sure what the start point and the goal of those use cases are.

It correctly processes the jar and produces one manifest for a particular release so I can pack it into a jar that then is recognized as a multi-release jar :-)

@timothyjward
Copy link
Contributor

timothyjward commented Aug 23, 2022

or example Aries JAX-RS Whiteboard currently resolves incorrectly on Java 9+ as it has a different set of imported packages in META-INF/versions/9/MANIFEST.MF which are ignored by bnd

Can you give a link to the exact problematic dependency version?

The change was added in apache/aries-jax-rs-whiteboard@ce3e7bc and is present in org.apache.aries.jax.rs:org.apache.aries.jax.rs.whiteboard:2.0.1

I've seen your examples of it being used, but I'm still not 100% sure what the start point and the goal of those use cases are.

It correctly processes the jar and produces one manifest for a particular release so I can pack it into a jar that then is recognized as a multi-release jar :-)

Ok, so my understanding of this is that your start point is:

  • There is a build project containing the sources (i.e. we aren't repackaging an existing jar file produced somewhere else)
  • the sources are multi-release (i.e. some sources designed to be compiled and put into a META-INF/versions/X folder)
  • the build project wants to generate its bundle manifest, rather than hard coding (code first approach)

And that your desired end goal is that you have a working multi-release OSGi bundle, with the relevant manifests in the relevant places.

If this interpretation is correct then my view is that the best user experience would be to set

Multi-Release: true

in the project's bnd configuration and have bnd work out the rest in a single processing pass. It is possible for bnd to automatically identify the target versions based on the META-INF/versions/X folders in the bundle's resources and then generate the appropriate manifests. This should be possible to do inside the Analyzer with repeated runs, even if the Analyzer needs to create "child" Analyzers for each of the target versions to avoid caching issues.

@laeubi
Copy link
Contributor Author

laeubi commented Aug 23, 2022

If this interpretation is correct then my view is that the best user experience would be to set

Yes this is correct, but ...

For example Aries JAX-RS Whiteboard currently resolves incorrectly on Java 9+ as it has a different set of imported packages in META-INF/versions/9/MANIFEST.MF which are ignored by bnd

I'm also aiming at this case to be supported.

This should be possible to do inside the Analyzer with repeated runs, even if the Analyzer needs to create "child" Analyzers for each of the target versions to avoid caching issues

Yes I already outlined that in another thread, but was not sure if it is appropriate to always do that thus I though it might be better to only enable this in demand, but If you think it is fine, I can add support for automatic retrieval as well...

@laeubi
Copy link
Contributor Author

laeubi commented Aug 23, 2022

For example reading support (but not generation support) is necessary for indexing and resolving to work properly with MR jars. For example Aries JAX-RS Whiteboard currently resolves incorrectly on Java 9+ as it has a different set of imported packages in META-INF/versions/9/MANIFEST.MF which are ignored by bnd.

I added read support for this but it seems the Analyzer actually don't care much about that as it only want to examine exports of its classpath items but the supplemental manifest only allows overriding Import-Package .. I can still ad the code but fear it must be used at some other place then ...

@timothyjward
Copy link
Contributor

For example Aries JAX-RS Whiteboard currently resolves incorrectly on Java 9+ as it has a different set of imported packages in META-INF/versions/9/MANIFEST.MF which are ignored by bnd

I'm also aiming at this case to be supported.

This should be possible to do inside the Analyzer with repeated runs, even if the Analyzer needs to create "child" Analyzers for each of the target versions to avoid caching issues

Yes I already outlined that in another thread, but was not sure if it is appropriate to always do that thus I though it might be better to only enable this in demand, but If you think it is fine, I can add support for automatic retrieval as well...

The Aries JAX-RS case is unfortunately non trivial and is one that will require some sort of new instruction or attribute on an existing instruction. This is because the Import-Package has to be customised differently for different versions and we can’t rely on default behaviour.

I would suggest that we start with the simpler (and more common) cases, where the Import-Package doesn’t need customising, or is customised the same way for all versions of the manifest. Once those are working we can work out the best way to configure “version specific” manifest generation instructions and also how to control pulling resources into version folders.

@timothyjward
Copy link
Contributor

timothyjward commented Aug 23, 2022

For example reading support (but not generation support) is necessary for indexing and resolving to work properly with MR jars. For example Aries JAX-RS Whiteboard currently resolves incorrectly on Java 9+ as it has a different set of imported packages in META-INF/versions/9/MANIFEST.MF which are ignored by bnd.

I added read support for this but it seems the Analyzer actually don't care much about that as it only want to examine exports of its classpath items but the supplemental manifest only allows overriding Import-Package .. I can still ad the code but fear it must be used at some other place then ...

This makes sense, most things shouldn’t care about multi-release. Those that do may want to ask for manifests and resources at specific versions.
Generating plugins (e.g. SCR) may need some level of the enhancement to process multi-release classes if they differ between versions. I’m thinking specifically that they have to output to the versioned folder not the normal location. This would be a separate PR though.

@laeubi
Copy link
Contributor Author

laeubi commented Aug 23, 2022

This makes sense, most things shouldn’t care about multi-release. Those that do may want to ask for manifests and resources at specific versions.

I think I can split the read-support in a separate PR, so if you think that is something that could be merged as a prerequisite, let me know and I'll open a PR for that.

Generating plugins (e.g. SCR) may need some level of the enhancement to process multi-release classes if they differ between versions

That's something I have also thought about, but I fear this is not covered by the spec or do I miss that? I think this would require a change in the spec.

@timothyjward
Copy link
Contributor

This makes sense, most things shouldn’t care about multi-release. Those that do may want to ask for manifests and resources at specific versions.

I think I can split the read-support in a separate PR, so if you think that is something that could be merged as a prerequisite, let me know and I'll open a PR for that.

I think that read support would be a good way to get this work started. Recognising a Multi-Release: true in the main manifest and being able to provide supplemented manifests/resources on request would be a great addition.

Generating plugins (e.g. SCR) may need some level of the enhancement to process multi-release classes if they differ between versions

That's something I have also thought about, but I fear this is not covered by the spec or do I miss that? I think this would require a change in the spec.

It might. The core spec talks about the classpath in
https://docs.osgi.org/specification/osgi.core/7.0.0/framework.module.html#framework.module-multireleasecontainer but SCR talks about getEntry/findEntries which ignore the classpath.

It may be as simple as saying that your DS components can’t be MR classes, at least until DS 1.next.

Currently bnd is not a capable of processing multi-release jars, this
adds support of multi-release processing by a new directive -release
where one can select what release version should be processed by bnd.

Fixes bndtools#5346

Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@laeubi
Copy link
Contributor Author

laeubi commented Aug 24, 2022

@timothyjward here we go:

@laeubi
Copy link
Contributor Author

laeubi commented Aug 25, 2022

If this interpretation is correct then my view is that the best user experience would be to set

Multi-Release: true

in the project's bnd configuration and have bnd work out the rest in a single processing pass.

Alright, I'll prepare a PR for this, so I think this one can be closed for now as low level release support seems not (yet) desired.

@laeubi laeubi closed this Aug 25, 2022
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.

[bnd] Support processing of Multi-Release-Jars
3 participants