#5346 Support for reading MRJs #5564

pkriens · 2023-02-21T15:12:33Z

There has been a long history how to support MRJs (Multi Release Jars). I kind of get it since it seems a giant magnet for complications, but alas, progress.

The main problem with the current approach is that the support was focused on the Jar file. Imho, (and I think BJ's) this is the wrong place. The Jar abstracts working with the ZIP file but should imho not contain any semantics of the class path. We already placed the Bundle-Classpath (which is a very similar problem) in the Analyzer.

This morning I had an interesting discussion with Christopher :-) He really helped me to see the problem more clear (I hope). I think we need to separate this problem strictly in 2 parts. The first is the MJR read support. This is this PR. Write will come later.

During the discussion it became clear to me that for our build (remember, I am ignoring writing MJRs) we need a view on the MJR that flattens the namespace of the resources. To be able to flatten, we need a max release for the view. I am a bit fuzzy on the use cases but I am convinced you need to be able specify this max release per JAR if you need to but for convenience, we need a setting per builder/project and default default. I've come up with the following rules:

In a -*path (e.g. -buildpath) you can set a MRJ.
You can add an attribute like -java.release.max=10 to the file in the buildpath. This will create a view up to and including release 10. I.e. what the VM would see if it was a Java 10 VM.
In absence of this attribute, a default can be set in the workspace or project with the same property -java.release.max=17
In absence of a default, the Jar is flattened for all the release it contains. This may mean that the compiler cannot read certain class files.

I am open to another strategy if someone has ideas. I am not sure I like the property name. We could use javac.target but I feel a tad uncomfortable with that. However, during the discussion it became crystal clear to me that this was not a Jar quality because the view that is used depends on the triplet of MRJ layout, max release, and the Jar.

So I've implemented this accordingly in this PR. Quick feedback is appreciated because Christopher has been hanging in there for a very long time.

Signed-off-by: Peter Kriens Peter.Kriens@aQute.biz

There has been a long history how to support MRJs (Multi Release Jars). I kind of get it since it seems a giant magnet for complications, but alas, progress. The main problem with the current approach is that the support was focused on the Jar file. Imho, (and I think BJ's) this is the wrong place. The Jar abstracts working with the ZIP file but should imho not contain any semantics of the class path. We already placed the Bundle-Classpath (which is a very similar problem) in the Analyzer. This morning I had an interesting discussion with Christopher :-) He really helped me to see the problem more clear (I hope). I think we need to separate this problem strictly in 2 parts. The first is the MJR read support. This is this PR. Write will come later. During the discussion it became clear to me that for our build (remember, I am ignoring writing MJRs) we need a _view_ on the MJR that flattens the namespace of the resources. To be able to flatten, we need a max release for the view. I am a bit fuzzy on the use cases but I am convinced you need to be able specify this max release per JAR if you need to but for convenience, we need a setting per builder/project and default default. I've come up with the following rules: * In a -*path (e.g. -buildpath) you can set a MRJ. * You can add an attribute like `-java.release.max=10` to the file in the buildpath. This will create a view up to and including release 10. I.e. what the VM would see if it was a Java 10 VM. * In absence of this attribute, a default can be set in the workspace or project with the same property `-java.release.max=17` * In absence of a default, the Jar is flattened for all the release it contains. This may mean that the compiler cannot read certain class files. I am open to another strategy if someone has ideas. I am not sure I like the property name. We could use javac.target but I feel a tad uncomfortable with that. However, during the discussion it became crystal clear to me that this was not a Jar quality because the view that is used depends on the triplet of MRJ layout, max release, and the Jar. So I've implemented this accordingly in this PR. Quick feedback is appreciated because Christopher has been hanging in there for a very long time. --- Signed-off-by: Peter Kriens <Peter.Kriens@aQute.biz> Signed-off-by: Peter Kriens <Peter.Kriens@aQute.biz>

kriegfrj

Minor things, I'm not in the space well enough to understand.

kriegfrj · 2023-02-22T01:53:49Z

biz.aQute.bndlib.tests/test/test/AnalyzerTest.java

 	/**
 	 * Verify that the manifest overrides a version in a package info
 	 */

 	@Test
 	public void testManifestOverridesPackageInfo() throws Exception {
-		try(Builder source = new Builder() ){
+		try (Builder source = new Builder()) {


It would make reviewing easier if these formatting changes were included separately. I know it's annoying to do so and I am often similarly lazy, so this is not a huge criticism...

kriegfrj · 2023-02-22T01:54:44Z

biz.aQute.bndlib/src/aQute/bnd/build/model/EE.java


-	UNKNOWN("<UNKNOWN>", "UNKNOWN", "0");
+	UNKNOWN("<UNKNOWN>", "UNKNOWN", "0", 0);


I realise that this is a legacy problem, but is there a problem with "unknown" and "1.0" having the same release number? Potential for confusion? Should Unknown be "-1" instead?

The lowest valid release target is 6 .... and there is already a method that handles this see my previous comment.

kriegfrj · 2023-02-22T01:55:14Z

biz.aQute.bndlib/src/aQute/bnd/osgi/Constants.java

@@ -24,6 +24,8 @@ public interface Constants {
 	String		IDENTITY_INITIAL_RESOURCE					= "<<INITIAL>>";
 	String		IDENTITY_SYSTEM_RESOURCE					= "<<SYSTEM>>";

+	String		MULTI_RElEASE								= "Multi-Release";


Looks like the "L" in "RELEASE" has snuck in as a lower case?

kriegfrj · 2023-02-22T01:57:02Z

biz.aQute.bndlib/src/aQute/bnd/osgi/MultiReleaseJars.java

+					return -1;
+			}
+		});
+		System.out.println(names);


timothyjward · 2023-02-23T09:11:18Z

It’s nice to see some progress on this issue. I tried to work with @laeubi some months ago on it but there was resistance at the time.

A flattened view seems like a good (and necessary) first step. When calculating manifests for multi-release bundles (a future PR) we will need to process the flattened contents at each version increment present in a multi release bundle.

laeubi · 2023-02-23T09:24:41Z

biz.aQute.bndlib/src/aQute/bnd/osgi/MultiReleaseJars.java

+import aQute.bnd.exceptions.Exceptions;
+
+/**
+ * Multi Release jars are another error magnet extension to Java. Instead of


BND would really benefit from describing actual implementation and usage than complains about other techniques ...

laeubi · 2023-02-23T09:26:20Z

A flattened view seems like a good (and necessary) first step.

I already tried that approach and actually it does not work out well and actually complicates things ... if one compares the necessary changes with my more generic approach that support read AND write, this clearly demonstrate that "flattening" it and handling it on multiple places don't work well. And it will get even more complicated when it comes to generation of MR jar.

Also the specification of any "settings" like -java.release.max=10 is completely superfluous and only adds unnecessary and unwanted configuration (again compare with my approach that fully works without any such additional options).

Putting this handling on the builder will require each and every user to take care of MR (instead of one central place the Jar) ...

So if someone wants a ZipFile this should be a class that Jar extends and put in everything "clean Zip" there... The MR-JAR is part of the Java Jar specification and therefore belongs there.

laeubi · 2023-02-23T09:31:39Z

biz.aQute.bndlib/src/aQute/bnd/build/model/EE.java

 	private transient EnumSet<EE>	compatibleSet;
 	private transient Parameters	packages	= null;
 	private transient Parameters	modules		= null;

 	/**
 	 * For use by JavaSE_9 and later.
 	 */
-	EE() {
+	EE(int release) {


EE class already has method for getting the release target:

bnd/biz.aQute.bndlib/src/aQute/bnd/build/model/EE.java

Lines 149 to 162 in f77b676

/**

* @return the java release target corresponding to this EE

*/

public OptionalInt getReleaseTarget() {

Version version = getCapabilityVersion();

int major = version.getMajor();

if (major > 1) {

return OptionalInt.of(major);

}

if (major == 1 && version.getMinor() > 5) {

return OptionalInt.of(version.getMinor());

}

return OptionalInt.empty();

}

laeubi · 2023-02-23T09:40:22Z

The Jar abstracts working with the ZIP file but should imho not contain any semantics of the class path.

MRJAR is not a semantic of the classpath it is an inherit semantic of the Jar specification see: https://docs.oracle.com/javase/9/docs/api/java/util/jar/JarFile.html

A multi-release jar file is a jar file that contains a manifest with a main attribute named "Multi-Release", a set of "base" entries, some of which are public classes with public or protected methods that comprise the public interface of the jar file, and a set of "versioned" entries contained in subdirectories of the "META-INF/versions" directory.

Also take a look specifically at the method:
https://docs.oracle.com/javase/9/docs/api/java/util/jar/JarFile.html#getJarEntry-java.lang.String-

That extends what a ZipFile does (https://docs.oracle.com/javase/9/docs/api/java/util/jar/JarFile.html#getEntry-java.lang.String-) with the definition of the Multi-Release-Jar specification.

Thee runtime handling is what I have actually extended the (bnd)Jar class with for build/analysis time semantic.

laeubi · 2023-02-23T09:44:03Z

To be able to flatten, we need a max release for the view.

The release for the build that must be used is the release target of the compiler, for analysis the minimum release is the minimum ee-target and the maximum of all dependencies where each distinct release between [min,max] needs to be analyzed and a corresponding supplemental manifest fragment must be generated.

laeubi · 2023-02-23T09:46:27Z

biz.aQute.bndlib/src/aQute/bnd/osgi/MultiReleaseJars.java

+		};
+
+		if (release < 9) {
+			release = Integer.MAX_VALUE;


if release is below 9, there is nothing to be flatten.. as the lowest possible release for a MJAR is 9.

Using MAX_VALUE is wrong here, as this will create a jar with the HIGHEST java version...

laeubi · 2023-02-23T09:51:42Z

biz.aQute.bndlib/src/aQute/bnd/osgi/MultiReleaseJars.java

+
+		List<String> names = new ArrayList<>(jar.getResources()
+			.keySet());
+		Collections.sort(names, (a,b)->{


This looks very confusing? Whats the purpose of this comparator? Why not work on the parsed version directly?

laeubi · 2023-02-23T09:52:20Z

biz.aQute.bndlib/src/aQute/bnd/osgi/MultiReleaseJars.java

+			} else
+				out.putResource(name, resource);
+		}
+		return out;


This does not merge the main manifest with the supplemental fragments

laeubi · 2023-02-23T09:53:28Z

biz.aQute.bndlib/src/aQute/bnd/osgi/MultiReleaseJars.java

+			Domain domain = Domain.domain(jar.getManifest());
+			return Processor.isTrue(domain.get(Constants.MULTI_RElEASE));
+		} catch (Exception e) {
+			throw Exceptions.duck(e);


In that case it is most probably not even a jar at all and one can return false.

laeubi · 2023-02-24T00:42:57Z

biz.aQute.bndlib/src/aQute/bnd/osgi/Analyzer.java

+		addClasspath(jar, -1);
+	}
+
+	public void addClasspath(Jar jar, int release) {


I think, if one want to make it work, that way, doing it at the time of adding classpath won't work well in the long term, but it could work doing it when one is getting the classpath!

So List<Jar> getClasspath(int release) or even Jar getClasspath(int release) (as one can actually even flatten all jars into a single linear jar view if we assume manifests do not matter here). That way one don't destroy the original added classpath jars and can later on analyze the classpath for different release levels.

pkriens · 2023-02-24T08:34:54Z

I am deleting this PR because I think I did not understand the specs. After reading the specs again and again and talking to @stbischof I think I have a better understanding.

A Multi Release JAR on the classpath has no impact. The compiler and bnd can treat this as a normal JAR. The spec explicitly forbids the versioned directories to make any changes to the public API. This means we can safely ignore the versioned directories. This removes a lot of my concerns.
The problem is then of generating a manifest. From @stbischof I understand there are bundles out there that use the multirelease jars and they need to get a manifest, this seems to be the primary problem?

I plan to push another PR later.

laeubi · 2023-02-24T09:37:15Z

I am deleting this PR because I think I did not understand the specs.

What spec and what part from it?

A Multi Release JAR on the classpath has no impact. The compiler and bnd can treat this as a normal JAR.

This is only true if BND completely ignores the classpath, so why the jars are added to the classpath if the are ignored? So if BND never uses the manifest of a classpath jar and never loads a resource from a classpath jar and never loads a class from the classpath jar, then one can ignore it for basic operations.

For plugins (e.g. JPMS) it is not true see #5327 (that was closed with unclear reference to this topic)

The problem is then of generating a manifest. From @stbischof I understand there are bundles out there that use the multirelease jars and they need to get a manifest, this seems to be the primary problem?

This is one part of the problem yes and maybe the most visible one, this problem and aspects where solved by #5359 that includes all codes and techniques to consume and produce jars according to both mentioned specs.

laeubi · 2023-02-24T09:41:08Z

I plan to push another PR later.

I can try to adjust my proposed PRs with the solution suggested above (only flatten then Jar for getClasspath() operations) and move all MR into a separate helper class that should satisfy most of the needs and concerns brought up, just let me know....

pkriens · 2023-02-24T10:24:32Z

@laeubi thanks for the offer but to do this right I think we need to some deep changes. As far as I can see, the JPMSModuleInfoPlugin runs in the verifier stage but adds a resource (module-inf). This will make it miss the name section.

I am adding a plugin that allows it to run at the right time: just after the main section is done, just before the name section. I also added a JPMSModule that abstracts the Jar that contains all the JPMS specific stuff. I also think I can use the doConditionalPackage() strategy to handle the few extra classes from the versioned directories efficiently. However, that requires copying the analyzer and its constituents.

I find that do it right, I need to touch a lot of places so I will make another attempt.

kriegfrj requested changes Feb 23, 2023

View reviewed changes

laeubi reviewed Feb 23, 2023

View reviewed changes

laeubi suggested changes Feb 24, 2023

View reviewed changes

pkriens closed this Feb 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#5346 Support for reading MRJs #5564

#5346 Support for reading MRJs #5564

pkriens commented Feb 21, 2023

kriegfrj left a comment

kriegfrj Feb 22, 2023

kriegfrj Feb 22, 2023

laeubi Feb 23, 2023

kriegfrj Feb 22, 2023

kriegfrj Feb 22, 2023

timothyjward commented Feb 23, 2023

laeubi Feb 23, 2023

laeubi commented Feb 23, 2023

laeubi Feb 23, 2023

laeubi commented Feb 23, 2023

laeubi commented Feb 23, 2023

laeubi Feb 23, 2023

laeubi Feb 23, 2023

laeubi Feb 23, 2023

laeubi Feb 23, 2023

laeubi Feb 24, 2023 •

edited

pkriens commented Feb 24, 2023

laeubi commented Feb 24, 2023

laeubi commented Feb 24, 2023

pkriens commented Feb 24, 2023


		UNKNOWN("<UNKNOWN>", "UNKNOWN", "0");
		UNKNOWN("<UNKNOWN>", "UNKNOWN", "0", 0);

	/**
	* @return the java release target corresponding to this EE
	*/
	public OptionalInt getReleaseTarget() {
	Version version = getCapabilityVersion();
	int major = version.getMajor();
	if (major > 1) {
	return OptionalInt.of(major);
	}
	if (major == 1 && version.getMinor() > 5) {
	return OptionalInt.of(version.getMinor());
	}
	return OptionalInt.empty();
	}

#5346 Support for reading MRJs #5564

#5346 Support for reading MRJs #5564

Conversation

pkriens commented Feb 21, 2023

kriegfrj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothyjward commented Feb 23, 2023

Choose a reason for hiding this comment

laeubi commented Feb 23, 2023

Choose a reason for hiding this comment

laeubi commented Feb 23, 2023

laeubi commented Feb 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laeubi Feb 24, 2023 • edited

Choose a reason for hiding this comment

pkriens commented Feb 24, 2023

laeubi commented Feb 24, 2023

laeubi commented Feb 24, 2023

pkriens commented Feb 24, 2023

laeubi Feb 24, 2023 •

edited