You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "mbeckerle (via GitHub)" <gi...@apache.org> on 2023/09/23 18:05:06 UTC
[GitHub] [daffodil] mbeckerle opened a new pull request, #1090: WIP: Changes for Java 21 (LTS)
mbeckerle opened a new pull request, #1090:
URL: https://github.com/apache/daffodil/pull/1090
Mostly works with tiny changes.
Remaining issues:
2 deprecated calls (hence fatal warnings turned off)
5 tests fail. One each in these 3 test classes
org.apache.daffodil.TresysTests.test_BG000
org.apache.daffodil.section13.text_number_props.TestTextNumberProps.test_textNumberPattern_exponent01 org.apache.daffodil.section05.simple_types.TestSimpleTypes.test_double_binary_06
All 3 of those failures are floating point related like this:
[error] Expected (attributes stripped)
[error] <d_02>9.8765432109876544E16</d_02>
[error] Actual (attributes ignored for diff)
[error] <ex:d_02>9.876543210987654E16</ex:d_02>
The Expected has one more digit 4 at the end.
1 other test failure is for reasons unknown. Possible change in regex behavior?
org.apache.daffodil.io.layers.TestJavaIOStreams.testBase64ScanningForDelimiter1
One CLI test failure:
org.apache.daffodil.cli.cliTest.TestEXIEncodeDecode.test_CLI_Encode_Decode_EXI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes for Java 21 (LTS) [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1090:
URL: https://github.com/apache/daffodil/pull/1090#discussion_r1345071970
##########
build.sbt:
##########
@@ -268,28 +272,30 @@ def buildScalacOptions(scalaVersion: String) = {
case _ => Seq.empty
}
- val javaVersionSpecificOptions =
- if (scala.util.Properties.isJavaAtLeast("9"))
- Seq("-release", "8") // ensure Java backwards compatibility (DAFFODIL-2579)
- else
- Seq.empty
+ commonOptions ++ scalaVersionSpecificOptions
Review Comment:
Added per comment.
But... It's a bit trickier. Scala 2.12 can't specify a release other than 8. So we can't just default to specifying java 11 class files when we are using Java 21, because scala warns about that, and we're fatal warnings.
So I did the minSupportedJavaVersion thing, but it's only used by java compilation for now. We have very small amounts of java code today, but those would complain about java 8, so those will use Java 11 classes IF the JVM is Java 21 only. Scala compilation does not specify any "-target" option.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -850,6 +886,14 @@ Differences were (path, expected, actual):
arrayCounters
}
+ private def getXSIType(a: Elem) = {
+ val res = a.attribute(XSI_NAMESPACE.toString, "type").map(_.head.text).orElse {
+ val a2 = a.attributes.asAttrMap.get("xsi:type")
Review Comment:
Removed. Requires an xmlns:xsi definition now.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -1113,6 +1189,29 @@ Differences were (path, expected, actual):
val b = DFDLDateTimeConversion.fromXMLString(dataB)
a == b
}
+ case Some("xs:double") => {
+ val a = strToDouble(dataA)
+ val b = strToDouble(dataB)
Review Comment:
Same resolution. NodeInfo.Double.fromString calls this.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/Misc.scala:
##########
@@ -103,38 +102,99 @@ object Misc {
// more time is wasted by people forgetting that the initial "/" is needed
// to get classpath relative behavior... Let's make sure there is a leading "/"
val resPath = if (resourcePath.startsWith("/")) resourcePath else "/" + resourcePath
- val res = this.getClass().getResource(resPath)
- if (res == null) {
- (None, resPath)
- } else (Some(res.toURI), resPath)
+ val res = Option(this.getClass().getResource(resPath))
+ (res.map.(_.toURI), resPath)
Review Comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes for Java 21 (LTS) [daffodil]
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
tuxji commented on code in PR #1090:
URL: https://github.com/apache/daffodil/pull/1090#discussion_r1345225847
##########
build.sbt:
##########
@@ -245,19 +251,19 @@ lazy val commonSettings = Seq(
def buildScalacOptions(scalaVersion: String) = {
val commonOptions = Seq(
- "-target:jvm-1.8",
+ // s"-target:$minSupportedJavaVersion", // scala 2.12 can only target Java 8.
"-feature",
"-deprecation",
"-language:experimental.macros",
"-unchecked",
- "-Xfatal-warnings",
"-Xxml:-coalescing",
"-Xfuture",
"-Ywarn-infer-any",
"-Ywarn-inaccessible",
// "-Ywarn-nullary-unit", // we cannot use this. It interferes with the Uniform Access Principle.
// See https://stackoverflow.com/questions/7600910/difference-between-function-with-parentheses-and-without.
"-Ywarn-unused-import",
+ "-Xfatal-warnings",
Review Comment:
Up to you (I am fine with ignoring my comment), but "-Xfatal-warnings" has been removed at one line number and added back at a different line number (an unnecessary change).
##########
build.sbt:
##########
@@ -268,29 +274,29 @@ def buildScalacOptions(scalaVersion: String) = {
case _ => Seq.empty
}
- val javaVersionSpecificOptions =
- if (scala.util.Properties.isJavaAtLeast("9"))
- Seq("-release", "8") // ensure Java backwards compatibility (DAFFODIL-2579)
- else
- Seq.empty
+ commonOptions ++ scalaVersionSpecificOptions
+}
+
+val javaVersionSpecificOptions = {
+ val releaseOption = // as of Java 11, they no longer accept "-release". Must use "--release".
+ if (scala.util.Properties.isJavaAtLeast("11")) "--release" else "-release"
- commonOptions ++ scalaVersionSpecificOptions ++ javaVersionSpecificOptions
+ // Java 21 deprecates Java 8 and warns about it.
+ // So if you are using Java 21, Java code compilation will specify a newer Java version
+ // to avoid warnings.
+ if (scala.util.Properties.isJavaAtLeast("11")) Seq(releaseOption, minSupportedJavaVersion)
+ else if (scala.util.Properties.isJavaAtLeast("9")) Seq(releaseOption, "8")
+ else Nil // for Java 8 compilation
}
// Workaround issue that some options are valid for javac, not javadoc.
// These javacOptions are for code compilation only. (Issue sbt/sbt#355)
def buildJavacOptions() = {
val commonOptions = Seq(
- "-Werror",
"-Xlint:deprecation",
+ "-Werror",
Review Comment:
Ditto here too, unnecessary change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes for Java 21 (LTS) [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1090:
URL: https://github.com/apache/daffodil/pull/1090#discussion_r1345017904
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -70,6 +70,34 @@ object XMLUtils {
val NegativeInfinityString = "-INF"
val NaNString = "NaN"
+ /**
+ * Converts a string to a float, including handling our INF, -INF, and NaN notations.
+ * @param s
+ * @return
+ */
+ def strToFloat(s: String): Float = {
+ s match {
+ case PositiveInfinityString => Float.PositiveInfinity
+ case NegativeInfinityString => Float.NegativeInfinity
+ case NaNString => Float.NaN
+ case _ => s.toFloat
+ }
+ }
+
+ /**
+ * Converts a string to a double, including handling our INF, -INF, and NaN notations.
+ * @param s
+ * @return
+ */
+ def strToDouble(s: String): Double = {
+ s match {
+ case PositiveInfinityString => Double.PositiveInfinity
+ case NegativeInfinityString => Double.NegativeInfinity
+ case NaNString => Double.NaN
+ case _ => s.toDouble
+ }
+ }
+
Review Comment:
Actually NodeInfo is in daffodil-runtime1, and this is in daffodil-lib, so to avoid code duplication I can convert NodeInfo.Double.fromString to call this. That's the right thing because the NodeInfo things construct JDouble boxed things anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes for Java 21 (LTS) [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1090:
URL: https://github.com/apache/daffodil/pull/1090#discussion_r1345843918
##########
build.sbt:
##########
@@ -268,28 +272,30 @@ def buildScalacOptions(scalaVersion: String) = {
case _ => Seq.empty
}
- val javaVersionSpecificOptions =
- if (scala.util.Properties.isJavaAtLeast("9"))
- Seq("-release", "8") // ensure Java backwards compatibility (DAFFODIL-2579)
- else
- Seq.empty
+ commonOptions ++ scalaVersionSpecificOptions
Review Comment:
It loosk like `-target` is deprecated in scala 2.13 in favor of `-release`. Maybe we should add `-release $minSupportedJavaVersion`, which works on both scala 2.12 and 2.13?
With scala 2.12.x, `-release` defines minimum JDK compatibility and works for more than just Java 8, but always creates Java 8 class files.
With scala 2.13.x `-release` defines the same minimum JDK compatibility, but also creates the same .class version.
So I think `-release` does a reasonable thing for both versions of Scala?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes for Java 21 (LTS) [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1090:
URL: https://github.com/apache/daffodil/pull/1090#discussion_r1343947069
##########
build.sbt:
##########
@@ -250,14 +250,18 @@ def buildScalacOptions(scalaVersion: String) = {
"-deprecation",
"-language:experimental.macros",
"-unchecked",
- "-Xfatal-warnings",
"-Xxml:-coalescing",
"-Xfuture",
"-Ywarn-infer-any",
"-Ywarn-inaccessible",
// "-Ywarn-nullary-unit", // we cannot use this. It interferes with the Uniform Access Principle.
// See https://stackoverflow.com/questions/7600910/difference-between-function-with-parentheses-and-without.
- "-Ywarn-unused-import",
+ "-Ywarn-unused-import"
+ ) ++ (
+ if (!scala.util.Properties.isJavaAtLeast("21"))
+ Seq("-Xfatal-warnings") // Java21 warns about java 8 target being deprecated
Review Comment:
Instead of removing xfatal warnings for java 21, should we change `javaVersionSpecificOptions` to something like this:
```scala
val sup = scala.util.Properties
if (sup.isJavaAtLeast("21")) Seq()
else if (sup.isJavaAtLeast("11")) Seq("--release", "8")
else if (sup.isJavaAtLeast("9") Seq("-release", "8")
else Seq()
```
So that on Java 21, you get Java 21 .class files? Or Maybe make it `--release 9` or `--release 11` or something, so even when building on Java 21 you get some level of backwards compatibility?
##########
build.sbt:
##########
@@ -268,28 +272,30 @@ def buildScalacOptions(scalaVersion: String) = {
case _ => Seq.empty
}
- val javaVersionSpecificOptions =
- if (scala.util.Properties.isJavaAtLeast("9"))
- Seq("-release", "8") // ensure Java backwards compatibility (DAFFODIL-2579)
- else
- Seq.empty
+ commonOptions ++ scalaVersionSpecificOptions
Review Comment:
This removes the `-release` option from scalacOptions. I'm not sure it actually matters since it looks like scalac defaults to generating Java 8 class files regardless of the JVM version. I assume Java 21 will still support running Java 8 files, just not creating them, so this should be fine?
Or maybe we add a new `val minSupportedJavaVersion` that defaults to 8 but is 9 or 11 on Java 21, e.g.
```scala
val minSupportedJavaVersion = if (scala.util.Properties.isJavaAtLeast("21") 11 else 8
```
And then we add `Seq("--release", minSupportedJavaVersion)` to both `scalacOptions` and `javacOptions`? So every class, regardless if it comes from a .java or .scala file, is the same class file version?
And when we decide to drop Java 8 support, we can just bump this one variable?
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -850,6 +886,14 @@ Differences were (path, expected, actual):
arrayCounters
}
+ private def getXSIType(a: Elem) = {
+ val res = a.attribute(XSI_NAMESPACE.toString, "type").map(_.head.text).orElse {
+ val a2 = a.attributes.asAttrMap.get("xsi:type")
Review Comment:
This `a2` is new. Is this because `xmlns:xsi` isn't define in some infosets? Should we just define them so they are valid XML?
##########
build.sbt:
##########
@@ -268,28 +272,30 @@ def buildScalacOptions(scalaVersion: String) = {
case _ => Seq.empty
}
- val javaVersionSpecificOptions =
- if (scala.util.Properties.isJavaAtLeast("9"))
- Seq("-release", "8") // ensure Java backwards compatibility (DAFFODIL-2579)
- else
- Seq.empty
+ commonOptions ++ scalaVersionSpecificOptions
+}
- commonOptions ++ scalaVersionSpecificOptions ++ javaVersionSpecificOptions
+val javaVersionSpecificOptions = {
+ val sup = scala.util.Properties
+ if (sup.isJavaAtLeast("11"))
+ Seq("--release", "8")
+ else if (sup.isJavaAtLeast("9"))
+ // this -release option has been removed in recent Java versions
+ Seq("-release", "8") // ensure Java backwards compatibility (DAFFODIL-2579)
+ else // java 8
+ Seq.empty
}
// Workaround issue that some options are valid for javac, not javadoc.
// These javacOptions are for code compilation only. (Issue sbt/sbt#355)
def buildJavacOptions() = {
val commonOptions = Seq(
- "-Werror",
- "-Xlint:deprecation",
- )
-
- val javaVersionSpecificOptions =
- if (scala.util.Properties.isJavaAtLeast("9"))
- Seq("--release", "8") // ensure Java backwards compatibility (DAFFODIL-2579)
+ "-Xlint:deprecation") ++ (
+ if (!scala.util.Properties.isJavaAtLeast("21"))
+ Seq("-Werror") // Java21 warns about java 8 target being deprecated
else
- Seq.empty
+ Nil
+ )
Review Comment:
Is this change still needed now that ``--release`` is not used when building with Java 21?
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -1113,6 +1189,29 @@ Differences were (path, expected, actual):
val b = DFDLDateTimeConversion.fromXMLString(dataB)
a == b
}
+ case Some("xs:double") => {
+ val a = strToDouble(dataA)
+ val b = strToDouble(dataB)
Review Comment:
Mentioned above, but this could be `NodeInfo.Double.fromXMLString(...)`. Same with xs:float below.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/Misc.scala:
##########
@@ -103,38 +102,99 @@ object Misc {
// more time is wasted by people forgetting that the initial "/" is needed
// to get classpath relative behavior... Let's make sure there is a leading "/"
val resPath = if (resourcePath.startsWith("/")) resourcePath else "/" + resourcePath
- val res = this.getClass().getResource(resPath)
- if (res == null) {
- (None, resPath)
- } else (Some(res.toURI), resPath)
+ val res = Option(this.getClass().getResource(resPath))
+ (res.map.(_.toURI), resPath)
Review Comment:
Typo `.` here.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -70,6 +70,34 @@ object XMLUtils {
val NegativeInfinityString = "-INF"
val NaNString = "NaN"
+ /**
+ * Converts a string to a float, including handling our INF, -INF, and NaN notations.
+ * @param s
+ * @return
+ */
+ def strToFloat(s: String): Float = {
+ s match {
+ case PositiveInfinityString => Float.PositiveInfinity
+ case NegativeInfinityString => Float.NegativeInfinity
+ case NaNString => Float.NaN
+ case _ => s.toFloat
+ }
+ }
+
+ /**
+ * Converts a string to a double, including handling our INF, -INF, and NaN notations.
+ * @param s
+ * @return
+ */
+ def strToDouble(s: String): Double = {
+ s match {
+ case PositiveInfinityString => Double.PositiveInfinity
+ case NegativeInfinityString => Double.NegativeInfinity
+ case NaNString => Double.NaN
+ case _ => s.toDouble
+ }
+ }
+
Review Comment:
The `NodeInfo.Float.fromXMLString` and `NodeInfo.Double.fromXMLString` functions do something similar with a little extra error checking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes for Java 21 (LTS) [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle merged PR #1090:
URL: https://github.com/apache/daffodil/pull/1090
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org