unitsofmeasurement / indriya

JSR 385 - Reference Implementation
Other
115 stars 40 forks source link

The project's generated MR JAR is not spec-compliant #346

Closed anthonyvdotbe closed 3 years ago

anthonyvdotbe commented 3 years ago

The JAR spec says:

The public API exported by the classes in a multi-release JAR file must be exactly the same across versions

However, this is currently not the case, since the Java SE 12 version adds additional API. This leads to issues such as the one described at https://github.com/unitsofmeasurement/uom-demos/tree/master/console/java13 (where it is incorrectly qualified as a JDK issue).

keilw commented 3 years ago

@anthonyvdotbe Sorry but which "The JAR spec" do you mean by that? The example is correctly called a JDK bug because a feature that was introduced in Java 12 can only be used in Java 13, The JDK trackers and contributors even admitted that, see closed issues #306 or more significantly #301. The problem is, that JDK-8207162 hides the correct Java 12 multi-release type definition that does not make sense prior to Java 12 (so it would be useless to declare the enum earlier it would just be a waste) so any JDK before 13 falls back to the default where again this type is USELESS and a properly working MP functionality (from Java 13 onward) "magically" enables it. So the bug was admitted before Java 12 but it wasn't actually fixed until 13.

So this is a no-issue and I hereby close it. Wherever you found that sentence it is either outdated or misleading. You find all sorts of cases where e.g. Java 9, 10 or above introduced new methods to the JDK and a method that takes Module or something else makes no sense in earlier JDK versions they won't even compile, but they are usable after a particular Java version. Same here, this does not make sense prior to Java 12 to define NumberFormatStyle so it would be technically possible but absolute nonsense to declare it in the default (Java 8+) codebase but throw a UnsupportedOperationException prior to Java 12. It works but only after Java 13 which is one version later than bugs like JDK-8207162 also admit.

anthonyvdotbe commented 3 years ago

@anthonyvdotbe Sorry but which "The JAR spec" do you mean by that?

@keilw this one, which is listed as one of the Java SE 11 specifications here

Again, the spec states:

The public API exported by the classes in a multi-release JAR file must be exactly the same across versions

So the MR JAR of this project is not spec-compliant. And in case you're wondering: the one in Java SE 16 has the same text, so it's not outdated.

Please reopen this issue.

andi-huber commented 3 years ago

I'm not an expert on this topic, though it seems to me, the observation is correct, stating that Indriya's MR jar is not spec [1] compliant. I think there is little room on how to interpret

The public API exported by the classes in a multi-release JAR file must be exactly the same across versions

Which leads to the question, what to do about that. I'm also curious, can we ignore the spec? (That would be the easy way out.)

[1] https://docs.oracle.com/en/java/javase/16/docs/specs/jar/jar.html

nipafx commented 3 years ago

I'm pretty sure that's also the reason for the behavior observed in this uom demo, which depends on indriya and where the JAR leads to compilation errors on JDK 12 class/module path and JDK 13-15 module path. (Which indicates that contrary to what the demo claims, there's no bug in the JDK tooling for multi-release JARs.)

anthonyvdotbe commented 3 years ago

@andi-huber thanks for your consideration

Which leads to the question, what to do about that. I'm also curious, can we ignore the spec? (That would be the easy way out.)

The spec also states:

It is difficult and costly to perform extensive API verification checks as such tooling, such as the jar tool, is not required to perform extensive verification and a Java runtime is not required to perform any verification.

In other words: javac/jar/java are allowed to assume MR JARs are well-formed, without being required to verify that their assumptions hold. So the spec can't be ignored, because malformed MR JARs will lead to issues such as the one in https://github.com/unitsofmeasurement/uom-demos/tree/master/console/java13 (e.g. during compilation, javac is allowed not to consider the versioned directories, because the public API must be the same across versions anyway).

andi-huber commented 3 years ago

I'm pretty sure that's also the reason for the behavior observed in this uom demo, which depends on indriya and where the JAR leads to compilation errors on JDK 12 class/module path and JDK 13-15 module path. (Which indicates that contrary to what the demo claims, there's no bug in the JDK tooling for multi-release JARs.)

I'm not involved with that particular demo, but that might be a valid observation, no objections to that.

andi-huber commented 3 years ago

Hi @keilw - seems to me, this is more serious than we might have thought at first glance. But again I'm by no means the expert here.

keilw commented 3 years ago

@andi-huber Thanks for the input. The only other expert here (you are at least when it comes to the inner workings of Indriya but if you haven't dealt with JPMS or Multi-Release-JARs you are not alone;-) seems @nipafx because I not only saw, reviewed and approved his DWX talk his GitHub profile shows decent commits (just like that other GeoAPI case believe me, it is a bit like Football or other training and if someone lacks that kind of excersise they are usually either a total newcomer or someone who stopped coding ;-) while @anthonyvdotbe's profile is almost blank with 79 commits compared to our 1k+ to 2.5k+ in your case.

First and most importantly Indriya is NOT the API, so the whole talking about APIs does not apply here. The RI is not the API and it may expose additional features (think of NumberSystem, MixedQuantity or others) to its users but they are an implementation detail. With some SPI features like

301 is independent of the demo case and was raised for an affected Java version like 11 so there clearly are JDK bugs and they were not fixed till some later version.

Hypothetically 12, it should even have been backported to 11, @nipafx could you shed some light on that, because I tested the "uom-demo" case with the latest available jdk-11.0.8.10 and it still fails, also fails with jdk-12.0.2.10 but passes with the first jdk 13 or higher?

As for the question about the spec: https://docs.oracle.com/en/java/javase/11/docs/specs/jar/jar.html and https://docs.oracle.com/en/java/javase/16/docs/specs/jar/jar.html are exactly identical. I was in the EC voting down JSR 376 before it passed the Reconsideration Ballot (something the pre-precursors to this JSR also once faced but for different reasons and the quality of the spec was not so much the issue then) by the EC a while later. Some concessions were made by Mark Reinhold, he did take it almost as an insult by the EC and did not communicate (e.g. block some of us on Twitter) and we still don't hug each other (like with Brian Goetz at the EC F2F in Austin that very May 2017) but I last met him personally in Stockholm at JFokus with Ed Burns and we spoke a bit after their talk. Enough JCP history, fact is, that JSR 376 spec never changed since at least 2017 and Java SE 9, but the JDK changed and "A future release of this specification may relax the exact same API constraint to support careful evolution." already hinted in a very vague fashion this should and will change. And it did, the --enable-preview feature wasn't introduced until jep12 that went into effect around Java 12, but the old JPMS part of the spec never changed, it just had that ambiguity and possible option to allow API evolution ever since. Which at the very least with the preview becomes necessary. Or are you really going to tell me, that to satisfy the old and outdated assumption from JSR 376 that was never changed later a class that would only work from Java 14 or 15 onwards and also just if you --enable-preview should be forced into the root path (outside META-INF/versions) be accessible only to throw let's say an UnsupportedOperationException??? I don't think so and if that was a necessity, this was dropped after Java 13 without any mention in the spec (that has not changted there after Java 9) also most likely because the preview feature became so widely used after that.

There is no need to reopen this and those who at least read, review or even co-author specifications themselves will understand.

andi-huber commented 3 years ago

I was under the impression that API in the context of a jar, does mean something very specific. But fair enough, I'm happy to learn.

keilw commented 3 years ago

Glad it allowed you (and hopefully a few others, too) to learn a bit about the sometimes tedious and often very "political" way some of these specs are created rather than just bore you. I guess if @desruisseaux has the time he could also tell you a little more about how this works in the OGC although you may have got a little "taste" of that helping those GeoTools guys with their bugs and just as much, there have been far more ugly and "colorful" discussions between Spec Leads of e.g. what's now "Jakarta Inject" (when it was still under the JCP lead by other people) and CDI over the roles and responsibilities, than what we heard in the GeoTools issue or from some of its contributors ;-O

nipafx commented 3 years ago

There is no need to reopen this

Yes, there is. The JAR's violation of the spec is the reason for the weird behavior you observe elsewhere and misattributed to existing but unrelated bugs in the JDK.

The spec is clear and - as you pointed out - unchanged between 11 and 16. The sentence "A future release of this specification may relax the exact same API constraint to support careful evolution." is exactly what it appears to be: A description of the possibility of this constraint changing in the future. But it's not changed yet and you're observing the effects of that.

Which at the very least with the preview becomes necessary.

No, it doesn't. --enable-preview is unrelated to multi-release JARs.The former is to experiment (not ship!) language features, the latter is to use more powerful/stable/performant/etc APIs on JVMs that support them.

keilw commented 3 years ago

@nipafx No it isn't, there is no point in making an enum that's only used by the implementation (see below) available in a Java version <12, sorry but that would be a waste of our resources and we have more important things to help our users with than this.

It works in the Java versions from 13 on where it needs to, there is no value making the same enum available in earlier Java versions down to 8 because it would there be a no-op and useless. Therefore no action is needed here. While --enable-preview may not be directly related to multi-release JARs the latter are often used and necessary to allow testing those features from a particular version onwards. The particular enum type assisting the use of CompactNumberFormat is exactly such case. It has no value prior to that. CompactNumberFormat does not exist before Java 12 and I am not aware, the JDK team intends to backport it to Java 11 or before either ;-) JPMS and multi-release JARs are also not directly related, yet they have unforeseen and unwanted side-effects like the bugs mentioned earlier.

What makes the whole ticket even more ridiculous is, that NumberFormatStyle is not even a public API, it was accidentially left public but if any of you guys especially @anthonyvdotbe ever studied what it really does, you would have easily comprehended, the only purpose is an internal switch for NumberDelimiterQuantityFormat.getCompactInstance() a method that only makes sense after Java 12 because it relies on the CompactNumberFormat. Therefore throwing some exception would technically be possible but it would be a nuisance, therefore it only exists in the Java 12+ MRJ implementation for a good reason.

That getCompactInstance() follows the pattern of NumberFormat and the enum is nothing more than INTEGERSTYLE in

    public static final NumberFormat getIntegerInstance() {
        return getInstance(Locale.getDefault(Locale.Category.FORMAT), INTEGERSTYLE);
    }

That constant is completely private as the mentioned getInstance() method with those extra arguments, and we will probably make NumberFormatStyle private as well, for now the next version of Indriya makes it package-private.

nipafx commented 3 years ago

You keep failing to understand the issue. This is not about whether it makes sense for your API or not, what NumberFormatStyle does, or on what version that makes sense. That is 100% beside the point.

The problem is that this JAR violates the spec and that causes actual problems that you yourself observed but fail to attribute to the correct cause and instead insist have something to do with nebulous JDK bugs that you keep mentioning...

JPMS and multi-release JARs are also not directly related, yet they have unforeseen and unwanted side-effects like the bugs mentioned earlier.

... yet don't describe or link to.

What makes the whole ticket even more ridiculous is...

... that you keep discussing what this API does while it's its mere presence violates the spec. Your argument is akin to insisting that compile errors after a method's return aren't actual errors because the code never gets executed anyway.

keilw commented 3 years ago

You keep failing to understand this is and never WAS an API in the first place. It was merely an implementation detail yet you keep talking about "API".

don't link to

Please see earlier, https://bugs.openjdk.java.net/browse/JDK-8207162 was linked here numerous times in this ticket and it's not the only one (also https://bugs.openjdk.java.net/browse/JDK-8210502 referenced there)

The issue is between JPMS and multi-release JARs. Without module-info.java the internal package-local element is found and everything compiles fine, as soon as you activate JPMS with a module-info.java the same fails. If the enum violated the specs it would not work withiout JPMS either, hence the precense of module-info.java not anything else violates the build.

desruisseaux commented 3 years ago

Hello Werner.

I have not examined this issue closely. But my reading of this report is that the Indriya JAR file does not provide the exact same set of public classes, methods and fields (we can ignore private things) for all JDK versions targeted in the JAR. If this is the case, this is indeed a risk of unpredictable behaviour at runtime. In order to analyse this issue, maybe a first step would be to list which classes/methods/fields are different?

keilw commented 3 years ago

Martin, Thanks for your input. The problem is not Indriya, it's the JDK. I completely eliminated that enum but the enum is not the problem. The problem is, that as soon as you add a module-info in your consuming application the JVM and compiler behave totally different. There is nothing missing the only difference is that NumberDelimiterQuantityFormat exists in a slightly enhanced version that only makes sense from Java 12 on, hence there is a META-INF\versions\12 variant overriding the default one, a bit like the ones you also added e.g. in the API. Without JPMS enabled (no module-info in the application, see and try in https://github.com/unitsofmeasurement/uom-demos/tree/master/console/java16) the multi-release-JAR version for Java 12 or higher is properly used. As soon as your application contains a module-info the default class not the Java 12 one is picked even on a higher JDK version. That is a JDK bug, you remove module-info and it works you add it and it fails, no other changes to anything else including Indriya and that internal enum was never the problem, it's failure of the multi-release JAR mechanism if JPMS is used together with it (I think it should work with an Automatic-Module-Name but a module-info breaks it)

keilw commented 3 years ago

@desruisseaux, @nipafx These JDK bug(s) exist all the way to Java 15. I don't really have time or care which exact bug numbers they have in the JDK tracker, feel free to investigate yourself but fact is they were not addressed until Java 16, see console/java16, a merge of prior "java13" and "java14" demos also containing the format demo. It now works by default with module-info.java, but that requires a minimum release of 16. If you keep the module-info.java and only change <jdkVersion>16</jdkVersion> to 15 or below, running it with the matching Java version, they all fail.

It's good to know for Jakarta EE, that there is a version that allows combining JPMS via module-info and multi-release JARs. Although there are still Java 11 updates I highly doubt, the one that's out now really got any of that fixed if Java 15 could not even fix that yet. @nipafx maybe if you play with Java 11 to 16 migration for your talks you can try that out yourself, but here there are no Java 11 features providing any value so the only multi-release variations above 9 are 12 and 14 and you would not access them from Java 11 yet.

keilw commented 3 years ago

Unfortunately the JDK contributors often have a slightly different focus and priorities on the next Garbage Collector, Multi-Core in the Cloud, 20 different variations of java.util.Set.of() (to gain what, nanoseconds?) or things like Constable, and they often forget to improve or grant the stability of existing features that especially with Java 9 were pretty rushed and not all of them tested properly in combination with each other. Modular multi-release JAR files are maybe not the most common use case either and that seems why some of it slipped through the QA.

Not sure if either of you even read the full spec including @nipafx because

A module descriptor is generally treated no differently to any other class or resource file. A module descriptor may be present under a versioned area but not present under the top-level directory. This ensures, for example, only Java 8 versioned classes can be present under the top-level directory while Java 9 versioned classes (including, or perhaps only, the module descriptor) can be present under the 9 versioned directory

Explicitly states the module-info could as the only class be present in the 9 versioned directory.

For example, a multi-release JAR file can depend on both the Java 8 and Java 9 major platform releases, where some class files depend on APIs in Java 8 and other class files depend on APIs in Java 9.

Earlier also states, there can be classes depending on Java 8 or Java 9 so if you have a class that deals with Module or CompactNumberFormat, there is neither point nor way to put this into the Java 8 base directory because it just would not compile. And this is also the case with the two variations of NumberDelimiterQuantityFormat. The enum was merely for debug purposes and internal switch statements and does not even matter anymore, but still if a class only makes sense on the Java 9 or Java 12 platform it must not exist in other directories where it may not even compile.

So Indriya never violated anything, it works perfectly as the spec intends to at least on recent enough Java versions. The demo unveiled at least 2 main categories of JDK bugs:

  1. Before Java 13 it was impossible to consume any multi-release JAR at least if that JAR itself (like Indriya) was also modular, even without the consuming app declaring any module-info. This was allegedly backported to Java 11 maybe it even was, try if you are curious.
  2. Before Java 16 if the consuming app declares a module-info then the multi-release JAR still wasn't properly accessed and instead (if present) you got the lowest available version class e.g. for Java 8. This problem exists until Java 15 so I doubt it was backported to Java 11. And it is unknown, if it ever can due to the distance between the two and with Java 17 out as the next LTS this fall, I also doubt this very special case gets that much attention to be solved in Java 11.
nipafx commented 3 years ago

You keep failing to understand this is and never WAS an API in the first place. It was merely an implementation detail yet you keep talking about "API".

It doesn't matter what you think of as an API vs an implementation detail. "The public API exported by the classes" clearly refers to the set of all public classes and their public/protected members. As far as the JDK is concerned, that's the JAR's API and in this JAR, there's a difference between the various versions. That's it - very simple. You can't argue that away.

Please see earlier, https://bugs.openjdk.java.net/browse/JDK-8207162 was linked here numerous times in this ticket and it's not the only one (also https://bugs.openjdk.java.net/browse/JDK-8210502 referenced there)

A) One is the duplicate of another. The issue concerns jdeps (not the compiler) and it was fixed in Java 12, so it can't explain the behavior you observed up to and including Java 15. Hence, it's obviously irrelevant in this context. B) That's just one. Where's "not the only one"? If you don't have any, it's very questionable (to say the least) if you keep telling people there are more.

The issue is between JPMS and multi-release JARs. Without module-info.java the internal package-local element is found and everything compiles fine, as soon as you activate JPMS with a module-info.java the same fails.

Once again, your JAR does not comply with the spec. From that point on, all bets are off and unpredictable and inconsistent behavior is to be expected. Everything that you're observing is very likely the consequence of this JAR being broken. Instead of writing lengthy replies, you could've just fixed the JAR with exception-throwing implementations of the concerned classes in the JAR's root (just to check) and then watch the problems disappear. Why not give that a try?

So Indriya never violated anything, it works perfectly as the spec intends

According to your highly dubious interpretation of what the spec "intends". However, it violates what the spec says.

All of this would be of no consequence and I wouldn't care very much if your stubborn misconception of how the module system behaves wouldn't impact one of the largest technologies in the Java ecosystem (JEE). Because you're not only wrong, you're loudly so.

desruisseaux commented 3 years ago

Werner, would it be possible to run the following command:

javap -public path/to/an/indriya.class

where path/to/an/indriya.class is the path to (said) the class for JDK 8, run again the command with the path to the same class for JDK 11 (or whatever is in the JAR file) and post a diff of the two outputs?

As a side question for my information: is Indriya embedded with JEE?

keilw commented 3 years ago

@nipafx As mentioned there are at least two different problems and most of these exist up to Java 15.

Once again, your JAR does not comply with the spec. From that point on, all bets are off and unpredictable and inconsistent behavior is to be expected. Everything that you're observing is very likely the consequence of this JAR being broken. Instead of writing lengthy replies, you could've just fixed the JAR with exception-throwing implementations of the concerned classes in the JAR's root (just to check) and then watch the problems disappear. Why not give that a try?

There is no need to "try" that because you probably never looked at these two classes. Again you keep babbling about the enum but the enum does not matter!!! Java 8 NumberDelimiterQuantityFormat was always there and a Java 12 NumberDelimiterQuantityFormat which uses the Java 12+ java.text.CompactNumberFormat.

It does not matter that this has an additional factory method getCompactInstance() because even if it was there, those are static methods and not part of an "API" which also the implementation doesn't even expose, it is not the API, I know the JDK sometimes does not make a clear difference be There is no need to create a static factory method only specific to Java 12 in the Java 8/root class and it would not fix this problem of the JDK which is an erroneous call to the "root" class instead of the proper Java 12 class. Plus again if a Java 12 specific class has additional methods taking e.g. a CompactNumberFormat or Module or any other argument that does not exist in the older platforms like Java 8 you could not even declare such "dummy" method only do have it there, it would not make the problem go away because the problem is caused by the wrong class called by the JDK.

Unless you remove the module-info below Java 16, so they fixed some bug there. Having fixed maybe 2 others while this one still exists does not help, it is all about Working Software.

@desruisseaux This should also answer your question. There is no relationship to Jakarta EE other than the API and RI also able to use new Jakarta annotations, but the guy who opened this ticket based on midled interpretation of some JEPs or the spec also did so in some Jakarta specifications. These problems may and will occur to Jakarta EE applications if ANY (I assume at least some other popular frameworks use multi-release JARs eventually) dependency of the application also makes use of JPMS, it will face one of these problems if the minimal Java version currently discussed is Java 11 and not higher.

desruisseaux commented 3 years ago

I just did the following test on a clone of Indriya project:

git pull
mvn clean package
cd target/classes
javap -protected tech/units/indriya/format/NumberDelimiterQuantityFormat.class | sort > base.txt
javap -protected META-INF/versions/12/tech/units/indriya/format/NumberDelimiterQuantityFormat.class | sort > v12.txt
diff base.txt v12.txt 

I got the following output:

>   public static tech.units.indriya.format.NumberDelimiterQuantityFormat getCompactInstance(tech.units.indriya.format.FormatBehavior);

So it it seems that Indriya has a public method for Java 12 which is not present on older Java version. Is it intended?

keilw commented 3 years ago

Yes, because compactInstance() relies on java.text.CompactNumberFormat which did not exist before. Although the arguments would theoretically be compatible, what would you do there, throw an exception maybe, but you would only get another error instead of method not found. And again if the Java 12 method took an argument like CompactNumberFormat then what would you do in the Java 8 version? So the argument that all methods must be the same are utter nonsens when new features in a higher platform come into play.

So if a method like getInstance() with various arguments existed before they shall be the same, but if you had either getCompactInstance() or say getInstance(CompactNumberFormatnumberFormat, UnitFormat unitFormat) there is no way to make this available in an older (Java 8-11) version of that static factory method. If someone fails to understand that they may be in the wrong business.

desruisseaux commented 3 years ago

If Indriya had a foo(CompactNumberFormat) method, it should be impossible to invoke that foo method in a code compiled for Java 8 because that code would be unable to access to the java.text.CompactNumberFormat type in the first place. So maybe that case would be less risky.

In the case where a method signature is invokable by all Java versions, we are at risk of NoSuchMethodError. An alternative would be to provide an implementation of that method for Java 8 with:

throw new UnsupportedOperationException("This method requires Java 12.");

Admittedly we still have an exception, but it is a RuntimeException instead than an Error. It is quite significant since Error usually stop the Java application, while Exception are intended to be handled. Furthermore is allow to provide a clear error message.

keilw commented 3 years ago

Either way that does not solve the issue that the JDK just gets it wrong if a module-info is there in the consuming app. If JPMS works as intended there is no error like this. And what would you do for cases with arguments that clearly must be allowed before raising the minimal JDK version to say 12 or 16 with arguments that don't exist in a prior Java version like Module or the mentioned CompactNumberFormat?

keilw commented 3 years ago

If Indriya had a foo(CompactNumberFormat) method, it should be impossible to invoke that foo method in a code compiled for Java 8 because that code would be unable to access to the java.text.CompactNumberFormat type in the first place. So maybe that case would be less risky.

@desruisseaux Until Java 16 that case only worked without a JPMS module-info declared in the consuming app. Even the Java 16 Spec as discussed earlier does not mention anything more than "A future release of this specification may relax the exact same API constraint to support careful evolution." but with Java 16 it clearly WAS relaxed, whether any of the documents mention that or not. So it still feels a bit like a mix between a new feature (after Java 16) and a bug, because if the JDK enforced that API consistently it would have done so for both modular and non-modular multi-release JAR files which we know it didn't. I keep the Java 12-specific method there but deprecated in the snapshot mainly for demonstration purposes. Other methods like parseCompound() are also deprecated across all Java versions and will be removed in favor of parseMixed() soon. A Java 12 Console demo has the call commented-out because it only works eithe without module-info or starting with Java 16.

Not sure @nipafx if you also plan to talk about those things in your talk, but the JDK itself is most sketchy and inconsistent in applying and enforcing its own specs otherwise the behavior of modular and non-modular multi-release JARs would have been the same ever since they got introduced. Whether actual bug tickets were filed for any of that or not. It's good to see, there is a way to allow more Java versions to compile this even with the burden of a runtime exception as @desruisseaux suggested. The other suggestion does not work at least not before Java 16. And most likely the JDK team relaxed this restriction with the only lapse of not so clearly stating there yet. Will ask some folks there, but this is not the first time and especially in MicroProfile this happens a lot where e.g. individual specs are outdated and some even break backward-compatibility while this does not break backward compatibility, it adds a little more flexibility in future Java versions. So this is resolved once and for all, we keep the stub to make it easier in different JDK versions before 16 and will explore platform specific versions if Indriya itself should Java 16 or 17 version classes. E.g. a variation that would have made sense with Java 12 already would be passing the NumberFormat.Style enum to control the creation of the CompactNumberFormat, inside that QuantityFormat, but that has the same side-effects prior to Java 16, so we probably just keep it beyond 16.

nipafx commented 3 years ago

Version 2.1.2 of tech.units:indriya contains the public class NumberFormatStyle (which has a few public methods) only in the Java 12 source branch. That means, in violation of the JAR spec, the JAR's public API differs across versions. The negative effect of this can be seen in this uom-demo from three weeks ago - the compilations fails (if a module declaration is present) exactly because of that class:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project uom-console-demos-java13: Compilation failure: Compilation failure: 
[ERROR] /home/nipa/code/tmp/uom-demos/console/java13/src/main/java/tech/uom/demo/java13/format/UnitFormatDemo.java:[34,33] cannot find symbol
[ERROR]   symbol:   class NumberFormatStyle
[ERROR]   location: package tech.units.indriya.format
[ERROR] /home/nipa/code/tmp/uom-demos/console/java13/src/main/java/tech/uom/demo/java13/format/UnitFormatDemo.java:[58,27] cannot find symbol
[ERROR]   symbol:   variable NumberFormatStyle
[ERROR]   location: class tech.uom.demo.java13.format.UnitFormatDemo
[ERROR] /home/nipa/code/tmp/uom-demos/console/java13/src/main/java/tech/uom/demo/java13/format/UnitFormatDemo.java:[59,56] cannot find symbol
[ERROR]   symbol:   method getCompactInstance(tech.units.indriya.format.FormatBehavior)
[ERROR]   location: class tech.units.indriya.format.NumberDelimiterQuantityFormat

Since then, you have:

Accordingly, the current version of the uom-demo no longer fails because of NumberFormatStyle. It still fails, though (at least with snapshot 2.1.3-20210510.011854-35 on JDK 15's module path):

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project uom-console-demos-java16: Compilation failure
[ERROR] /home/nipa/code/tmp/uom-demos/console/java16/src/main/java/tech/uom/demo/java16/format/QuantityFormatDemo.java:[58,57] method getCompactInstance in class tech.units.indriya.format.NumberDelimiterQuantityFormat cannot be applied to given types;
[ERROR]   required: tech.units.indriya.format.FormatBehavior
[ERROR]   found:    java.text.CompactNumberFormat,tech.units.indriya.format.SimpleUnitFormat
[ERROR]   reason: actual and formal argument lists differ in length

Looking into NumberDelimiterQuantityFormat::getCompactInstance, we can see that the JDK 12 version contains an overload that the original doesn't. You already deprecated that overload for removal in a9c52394f (with the message "Migrate java10 demos to java12 demos, java14 to java16" 🤔), so I thought I'll check what happens if it's actually gone.

Unfortunately, I can't build indriya because the maven-bundle-plugin gets a ConcurrentModificationException. Instead I ran mvn compile and replaced the JDK 12 variant of the NumberDelimiterQuantityFormat.class that comes with the official snaphot JAR mentioned above with the one I just compiled to target/classes/META-INF/versions/12.

Then I ran the uom-demo java16 again on JDK 14, 15, and 16 both with and without module declaration (after removing the call to the removed method from QuantityFormatDemo and enabling preview for records). I also checked out the old java13 demo and ran it with and without module declaration (after removing calls to the now removed NumberFormatStyle).

And guess what? They were all successful!

TL;DR

Once I made indriya spec-compliant by aligning the signatures of all public and protected methods of all public classes (aka "the JAR's API") across all versioned source trees, all problems went away. Contrary to your many claims to the opposite. Really, instead of keeping yourself and everybody else busy, you should've just tried that when @anthonyvdotbe opened this issue.

keilw commented 3 years ago

@nipafx Thanks for replicating at least some of what we already did earlier.

Unfortunately, I can't build indriya because the maven-bundle-plugin gets a ConcurrentModificationException. Instead I ran mvn compile and replaced the JDK 12 variant of the NumberDelimiterQuantityFormat.class that comes with the official snaphot JAR mentioned above with the one I just compiled to target/classes/META-INF/versions/12.

That is a problem/bug of a Maven plugin (bnd or Felix), I guess it's documented somewhere, but they have not fixed that yet, you should build it with Java 14 and not 16, because the highest toolchain is also 14, see the CI build it works without problems.

Of course the demo is succesful in Java 16 even with the overloaded methods that are Java 12 specific like foo(CompactNumberFormat) but I guess you did not bother trying that or did not want to, but no worries, it runs on the CI server java16/format/QuantityFormatDemo.java

So the spec from Java 16 is applied differently and that is the most important learning from this, beside the fact (which you also did not bother to replicate but we did it earlier) that the JVM behaved for non-modular multi-release JARs like that ever since at least Java 13 while only for modular multi-release JARs it behaved differently. After 16 this is no longer the case.

If you or @anthonyvdotbe chose to ignore that or still have blind faith in what some spec that Oracle was just too lazy or busy to update says there is nothing to prevent you but as he even filed that ticket from some forgotten Jakarta EE ticket the important takeaway for Jakarta EE is what you can do with multi-release JARs after Java 16 and what you cannot do before that at least not without side-effects. However, while Indriya is supposed to run from Java 8 at least for another 5-10 years the JVM ranges for Jakarta EE 10, 11 or higher and also those of other implementations like Seshat by @desruisseaux are much different. The minimum Java version even for the APIs are expected to go up to at least 11 and soon it may be 17, so by then these issues will be irrelevant to Jakarta EE, in the meantime while 11-15 are still supported it is good to mention it somewhere in the Jakarta EE specs or documentation.

nipafx commented 3 years ago

Of course the demo is succesful in Java 16

Not "of course". It works coincidentally in 16, likely because some unrelated change to the compiler altered its behavior in unspec'ed and thus untested situations.

So the spec from Java 16 is applied differently

No. If the JDK checked whether a JAR was built to spec (which it doesn't have to - "a Java runtime is not required to perform any verification" says the JAR spec), all versions would reject indriya. But it doesn't, so they don't and so the project keeps flying under the radar.

that the JVM behaved for non-modular multi-release JARs like that ever since at least Java 13

I assume you've fed it the same broken JARs, so that's no surprise at all.

some spec that Oracle was just too lazy or busy to update

I wonder how much more broken an argument can be than "My JAR doesn't comply with the spec and I observe erratic behavior. Clearly, it's because the spec wasn't updated to what I imagine it should be and because bugs all over the JDK prevent it from behaving according to the imagined spec." Ouch.

keilw commented 3 years ago

Not "of course". It works coincidentally in 16, likely because some unrelated change to the compiler altered its behavior in unspec'ed and thus untested situations.

Please point to the exact changes in Java 16 you believe are responsible for the different behavior Until Java 15

[ERROR]   required: tech.units.indriya.format.FormatBehavior
[ERROR]   found:    java.text.CompactNumberFormat,tech.units.indriya.format.SimpleUnitFormat
[ERROR]   reason: actual and formal argument lists differ in length
[ERROR]

After Java 16:

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  5.009 s
[INFO] Finished at: 2021-05-10T13:01:16+02:00
[INFO] ------------------------------------------------------------------------

With exact identical code, so Java 16 tolerates the overloading with new arguments (or even a new method that did not exist in the prior versions of the class), thus what "magical change" would you say is responsible for that?

that the JVM behaved for non-modular multi-release JARs like that ever since at least Java 13

Again remove the module-info and see the "broken" JAR was always accepted by JDK 13-15 or even below 13.

How many specs do you review and ask the authors to change them (if they missed something) on a daily basis?

How many have you written at Oracle????

You talk at conferences about what your colleagues like Brian Goetz, Mark Reinhold or others define, so please ask them before making assumptions.

nipafx commented 3 years ago

Please point to the exact changes in Java 16 you believe are responsible for the different behavior

Why don't you? You're the one claiming this was a bug that got fixed, so it should be easy to point to it.

Again remove the module-info and see the "broken" JAR was always accepted by JDK 13-15 or even below 13.

This is no counterargument. Nobody ever said the JDK would reject a broken JAR, so it not rejecting it, can not be an argument for it being up to spec. In unspec'ed situations, the behavior is not determined by the spec (obviously) and non-deterministic behavior is to be expected. Your position is akin to somebody whose code has race condition but who denies that because "it works sometimes".

How many specs do you review and ask the authors to change them (if they missed something) on a daily basis?

How many have you written at Oracle????

None and none. And if this thread reflects your understanding of how specifications work, then clearly this should be the same for you. Also, argument from authority.

keilw commented 3 years ago

Well I do, and I also reviewed the very JPMS, see https://jcp.org/en/jsr/results?id=5959. There were people including Mark Reinhold and others who tried to pressure us to vote "Yes" on that ballot, as it also happened before when some (not always as many) voted against specs around Java SE. So I understand how these specs work not only those I contribute to.

While you just admitted yourself at Oracle you're merely a foot-soldier and if you can't find anybody in your company who would answer the question of changed behavior between Java 15 and 16, no worries I can.

nipafx commented 3 years ago

Waffling, ab auctoritate, ad hominem, trying to hide relevant changes with unrelated commit messages, etc. None of that changes two facts:

Everything else is just noise and distraction.

keilw commented 3 years ago

Thanks but your services are no longer needed, I'm going to ask the people responsible e.g. the Spec Leads about the obvious change of behavior (allowing an overloaded method in a multi-release-JAR where that was different before) You cannot answer that so please don't bother.

desruisseaux commented 3 years ago

I think that @nipafx has a good point about having the same API for all versions. I'm not familiar with what changed in JDK 16, but it seems to me that making the public API identical will be safer in any cases, isn't it?

I also feel uncomfortable about authority argument. They often depend on a particular context, and can distract from the technical discussion. Do we have an agreement on the following technical points?

I did not tested the experiment that @nipafx did above, where he got the demo working. But if this approach work, what would be the drawback of fixing the issue reported here?

Can we unblock him and instead gives this issue a rest for a few days before to reconsider?

keilw commented 3 years ago

No he has nothing to add to this anymore just repeated noise I'm afraid. The problem is solved for Indriya as we provide the same signature in NumberDelimiterQuantityFormat at least until maybe a Java 16 or 17 class (this one or another) can do differently based upon credible sources and evidence. @nipafx is not a credible source for this, the Spec Leads like Brian or Iris (and maybe others in the JSR 391 EG) are and not for Indriya but for the long-term evolution of Jakarta EE based on different Java versions I plan to ask them but he has nothing to add here, I don't know if he ever met them in person.

The only method for study purposes is deprecated, so are others, we don't just have the Java SE spec, there are others especially Unicode which is the reason why parseCompound() is deprecated and will be removed from Indriya soon, or you with OGC and its own specifications. These provide real value for the users, e.g. new units or some that may behave differently, so I don't want to spend more time with this non-functional requirement, let's go back to all the functional requirements we also have here and in other modules.

AlanBateman commented 3 years ago

@nipafx The JBS issue with the fix to javac in JDK 16 is JDK-8235229. You are correct that the classes in a multi-release JAR file should be exactly the same across versions.

desruisseaux commented 3 years ago

I'm not familiar with Indriya API so I can only have superficial opinion on this issue; I let you manage. But while the discussion has wandered in different directions, I did not saw a reason to doubt about @nipafx technical competence. I feel that his contributions in other parts of "unitsofmeasurement" (if he wanted to) would be appreciable. I find unfortunate to loose his possible inputs in Seshat for example…

keilw commented 3 years ago

Just see @AlanBateman's input, thanks a lot, that was useful and pretty close to the Spec Leads ;-) And if the Spec Leads and experts in the Java SE JSRs confirm, that something like overriding foo() in the base version of a multi-release JAR with say foo(Module) or foo(Record) (just to give some examples) that are only available in later Java versions are not allowed or advisable in a multi-release JAR file then we will stick to that advice and hope any change is communicated properly if it came some day.

Thanks @AlanBateman and others for pointing @nipafx in the right direction so we got the answer asked pretty much at the beginning of this thread without having to read coffee grounds or make assumptions. It is probably even more useful for other specs like Jakarta EE and its user guides, etc. where in the past we already pointed to some of the Java SE specs but were asked to elaborate a little more about modularity and/or multi-release JARs.

We'll keep him silenced here because everyone else is welcome and able to provide input in a more compact and precise manner like Alan did. And maybe after a few days or so cooling period I guess we may revert it.

desruisseaux commented 3 years ago

Well, I would like to have him welcomed at least in unitsofmeasurement/seshat. Even if he probably has no interest in that specific sub-project, I would appreciate to have this possibility open. Is it possible?

keilw commented 3 years ago

No it does not work in a particular sub-project but what kinds of contributions do you mean, code?` He's blocked for 3 days now, that is the only option, you cannot block for some repositories only.

desruisseaux commented 3 years ago

Not necessarily code. Bug reports are appreciated as well, or hints such as pointing to some specifications.

keilw commented 3 years ago

You'll probably get more constructive input by the others involved including Alan, but after the 3 days he's welcome to participate again, hopefully a little more efficient. The main reason for the temporary blocking was the cheeky "Go f* yourself and search that rather than me giving the correct answer" a little earlier, only for others in his company to give that correct answer for him soon after. That was inappropriate and wasteful, hope we can avoid this in the future.

So even with the term "foot-soldier" may have been a little harsh, it still hits the point. And before BEA and the JRockit team (much of it including Flight Recorder came from there, we were asked to sell its predecessors like "BEA Guardian" while I worked there) was taken over by Oracle, I worked there as a "DRE" ("Developer Relationship Expert" I believe) in 2006, the only external contractor BEA accepted in the EMEA region at least back then. Our team lead Richard who at least indirectly reported to Adam Messinger who later was the Oracle Rep in the JCP I think even the EC alternate for some time, asked me into job interviews for people just like @nipafx now (also in a "Developer Relations" kind of role) and in 90% or more of these interviews he later said "He was good, but he wasn't Werner". So I was in his shoes having to help customers with their problems or bugs of BEA products like WebLogic, JRockit, AquaLogic for some time and also know a bit about proper behavior towards those customers... If I responded in a similar way to any customer, especially the "Crit Sit" ones or high profile customers with a big budget, they would probably have fired me.

pron commented 3 years ago

After we discussed this matter amongst ourselves and agreed that, as a courtesy to others who might read this issue, it’s important to point out you had misunderstood the MR JAR spec, @nipafx volunteered to communicate the answer to you because he had brought this issue to our attention and, frankly, no one who had interacted with you in the past would. @AlanBateman then bravely agreed to chime in briefly, but that’s it.

keilw commented 3 years ago

The spec had alway been a little ambiguous especially

Modular multi-release JAR files:

A public or protected class in a non-exported package (that is not declared as exported in the module descriptor) need not preside over a class of the same fully qualified name and access modifier whose class file is present under the top-level directory.

A module descriptor is generally treated no differently to any other class or resource file. A module descriptor may be present under a versioned area but not present under the top-level directory. This ensures, for example, only Java 8 versioned classes can be present under the top-level directory while Java 9 versioned classes (including, or perhaps only, the module descriptor) can be present under the 9 versioned directory.

And while @nipafx kept insisting that the module-info may also be in the top-level (Java 8) folder under the right circumstances (e.g. we do this in the JSR 354 RI Moneta) placing that in Java 9+ folders is perfectly spec-compliant, see above.

A multi-release JAR file allows for a single JAR file to support multiple major versions of Java platform releases. For example, a multi-release JAR file can depend on both the Java 8 and Java 9 major platform releases, where some class files depend on APIs in Java 8 and other class files depend on APIs in Java 9. This enables library and framework developers to decouple the use of APIs in a specific major version of a Java platform release from the requirement that all their users migrate to that major version. Library and framework developers can gradually migrate to and support new Java features while still supporting the old features.

Explicitly states "Library and framework developers can gradually migrate to and support new Java features while still supporting the old features." so one would think that e.g. the same as adding a new method like public Module getModule() would be legitimate for a class like Class in a multi-release JAR with the Java 8 base version without such method. Just as the JDK does itself all over the place. It turns out, that is currently not supported which is unfortunate, but "A future release of this specification may relax the exact same API constraint to support careful evolution." always left that open and whether the side-effect or not, Java 16 compiles without problems both modular and non-modular multi-release JAR files or rather the apps consuming at least one of them.

We had an earlier bug report #301 precisely caused by JDK-8235229 as it looks like. This was also referenced from the currennt one, but nobody ever mentioned that OpenJDK ticket until @AlanBateman did today. The only other tickets mentioned were unrelated and @nipafx seems to have mistaken them or not been aware of JDK-8235229.

You @pron also wrote in jakarta-platform#329 but only about support for Java 8 or 11, never mentioning JDK-8235229 while that's pretty essential to a working module-info vs. Automatic-Module-Name or the need to have both at least prior to Java 16.

Of course there are worse specs at least some that have hundreds or more optional aspects, just take JWT as one example, which is why I cautioned @m-reza-rahman and others about using it directly in Jakarta EE as opposed to a more "fluent" spec and API like MP-JWT.

pron commented 3 years ago

Just to be perfectly clear, JDK-8235229 is incidental to this issue, even though it may have caused some confusion about its origin as it's caused the underlying problem to manifest differently on different JDK versions. As @nipafx and others repeatedly tried to explain, the JAR specification currently requires that the public/protected signature of exported classes be identical for all versions for the JAR file to be well-formed and behave correctly in all situations; that a specific ill-formed JAR happens to incidentally behave correctly in some situations on some JDK versions is irrelevant. The jar tool will partially validate some aspects of this requirement but may fail to detect others.

keilw commented 3 years ago

While it may not be directly related to this one (which was just created without an actual problem based on discussions in Jakarta EE) it most certainly was for #301. I also pointed to it in jakartaee-platform#329 only briefly mentioning the examples (which reproduce BOTH at least until Java 16) but instead of getting to the root cause of #301 @anthonyvdotbe thought to help by creating this one. An issue nobody even reported of faced out there unlike #301 which covered a real need of users.

@anthonyvdotbe also quoted Alex Buckley from some JDK mailing list:

Every library including its own copy of a standard API is a bug, not a feature.

But not only is that precisely what the whole JDK does, there is no separate API JAR (and for some specs like JSR 310 there isn't even a clear separation of the API and API elements reference their own implementation while e.g. the Collections API was much better at this) even with smaller modules API and implementation are molded closer together than in Spring, there are also several Jakarta EE specs where the implementations cannot really separate the API, especially Faces.

While in theory and in the academic world many things may sound great the reality of software projects and libraries is often different. This was more of an academic discurse or discussions in different parts of the project or I remember we also had with Brian or Mark Reinhold along the lines of JSRs like the JPMS (one of his professors was Stallman btw ;-D) so there is no more need here because the only theorecial problem (nobody ever hit it or filed an issue here) was preemptively addressed, but there is more actual work in Jakarta EE.

And instead of beating this dead horse even further, what about #348? Can those with similar or even more insight into JPMS and multi-release JARs than @sormuras comment there so we don't get things mixed together again like some of the JDK bugs or problems.

desruisseaux commented 3 years ago

Thanks @pron for the clarification about JDK-8235229 and the links to tracking system.

keilw commented 3 years ago

@desruisseaux please also have a look at #348 because you know (since you contributed a lot there) unit-api has one way to create a "dual-release JAR" (for Java 8+9) using maven-antrun, but for a true "multi-release JAR" (more than 2 Java versions, currently 4) this was not feasible, so I evaluated several other options with the maven-toolchain plugin. Especially for Maven there are not so many that really work, and most plugins were experimental and soon abandoned, like https://github.com/metlos/multi-release-jar-maven-plugin.

So is there tool-support and if using "jar" instead of the maven-jar-plugin where is this done succesfully at the moment? Again please follow-up in #348 and not here to avoid fragmenting the discussion across different tickets.

keilw commented 3 years ago

This #349 is how that ticket here should have looked like. Unfortunately @anthonyvdotbe splashed something here without knowing enough or anything about the particular JSR as he also did in Jakarta EE before creating this only from a ticket there which the maintainers of Jakarta EE had pretty much ignored and nobody in the Spec Committee (myself included) ever heard about or took notice. The special variations of NumberDelimiterQuantityFormat never made sense without proper customization that could happen by overloading if the Java/JAR spec would already lift the restriction, but until it does this makes no sense. And that alone is the only real business value, support for CompactNumberFormat which also seems to come from Unicode (where we get other changes like Compound->Mixed because the Unicode specs now have a strong usage of both) but unless someone really used CompactNumberFormat and may end up with something like "1K kg" now or "1 thousand kg" having to handle both potentially even in more than one locale.

It gave us answers to the much more crucial ticket #301 where there was a workaround until Java 16 and we still need it as fallback, but CompactNumberFormat and the related method or implementation details may never have been used by anybody hence the whold discussion here was about something less than 0.5% or fewer users ever hit, and so far none have.

It seems like an excuse for both @anthonyvdotbe or @nipafx that they just read or talk about specs, but next time please also try to read THIS spec before you even create such a ticket in the first place or spend much time on it.

It's up to the maintainers of the project or spec to prioritize, @AlanBateman or @pron may know why e.g. JDK-8235229 took between Java 11 and 16 to resolve, but "Created: 2019-12-01 08:10" sounds like it was only spotted more than a year after 11 came out. I guess there are no plans to backport that into Java 11 btw, are there? Because only real value of these findings here is for Jakarta EE, for Indriya once #349 is executed there won't be any signature differences (at least while the Java platform or language does not like them) and it may take 10 or 20 years before anybody even came across a problem with it in real life using a version like 2.0.4-2.1.2 (all released within less than 12 months, we have a slightly higher version cadence than even the JDK does with a much smaller team ;-)