stevedlawrence commented on code in PR #1057:
URL: https://github.com/apache/daffodil/pull/1057#discussion_r1338491517
##########
build.sbt:
##########
@@ -211,6 +212,43 @@ lazy val testStdLayout =
Project("daffodil-test-stdLayout", file("test-stdLayout
.dependsOn(tdmlProc % "test")
.settings(commonSettings, nopublish)
+/* Workaround: certain reflection (used by JAXB) isn't allowed by default in
JDK 17:
+ *
https://docs.oracle.com/en/java/javase/17/migrate/migrating-jdk-8-later-jdk-releases.html#GUID-7BB28E4D-99B3-4078-BDC4-FC24180CE82B
+ *
+ * While we can handle this JVM quirk at build time, at runtime we won't know
+ * a user's JVM version. We'll provide documentation and an extension setting
+ * to add these flags to the extension-launched debugger backend.
+ */
+lazy val extraJvmOptions: Seq[String] =
+ if (scala.util.Properties.isJavaAtLeast("17"))
+ Seq(
+ "--add-opens",
+ "java.base/java.lang=ALL-UNNAMED",
+ )
+ else Seq()
+
+lazy val xjcSettings =
+ Seq(
+ libraryDependencies ++= Seq(
+ "com.sun.xml.bind" % "jaxb-impl" % "2.2.11",
+ "javax.activation" % "activation" % "1.1.1",
+ "org.glassfish.jaxb" % "jaxb-xjc" % "2.2.11",
+ ),
+ xjcCommandLine += "-nv",
+ xjcCommandLine += "-p",
+ xjcCommandLine += "org.apache.daffodil.tdml",
Review Comment:
Can you combine these into a single setting, i.e.:
```scala
xjcCommandLine ++= Seq(
"-nv",
"-p",
"org.apache.daffodil.tdml",
),
```
That's the standard for things like this, especially since it makes it more
clear the order is important.
##########
build.sbt:
##########
@@ -211,6 +212,43 @@ lazy val testStdLayout =
Project("daffodil-test-stdLayout", file("test-stdLayout
.dependsOn(tdmlProc % "test")
.settings(commonSettings, nopublish)
+/* Workaround: certain reflection (used by JAXB) isn't allowed by default in
JDK 17:
+ *
https://docs.oracle.com/en/java/javase/17/migrate/migrating-jdk-8-later-jdk-releases.html#GUID-7BB28E4D-99B3-4078-BDC4-FC24180CE82B
+ *
+ * While we can handle this JVM quirk at build time, at runtime we won't know
+ * a user's JVM version. We'll provide documentation and an extension setting
+ * to add these flags to the extension-launched debugger backend.
+ */
+lazy val extraJvmOptions: Seq[String] =
+ if (scala.util.Properties.isJavaAtLeast("17"))
+ Seq(
+ "--add-opens",
+ "java.base/java.lang=ALL-UNNAMED",
+ )
+ else Seq()
+
+lazy val xjcSettings =
+ Seq(
+ libraryDependencies ++= Seq(
+ "com.sun.xml.bind" % "jaxb-impl" % "2.2.11",
+ "javax.activation" % "activation" % "1.1.1",
+ "org.glassfish.jaxb" % "jaxb-xjc" % "2.2.11",
+ ),
+ xjcCommandLine += "-nv",
+ xjcCommandLine += "-p",
+ xjcCommandLine += "org.apache.daffodil.tdml",
+ xjcLibs := Seq(
+ "org.glassfish.jaxb" % "jaxb-xjc" % "2.2.11",
+ "com.sun.xml.bind" % "jaxb-impl" % "2.2.11",
+ "javax.activation" % "activation" % "1.1.1",
+ ),
+ xjcJvmOpts ++= extraJvmOptions,
+ Compile / xjc / sources := Seq(
+
file("daffodil-lib/src/main/resources/org/apache/daffodil/xsd/tdml-core.xsd"),
+ ),
+ Compile / doc / sources := Seq(file("")),
Review Comment:
Is this necessary? I think this effectively disables scala/javadocs and the
-doc.jar for projects that use this setting (i.e. tdml-lib currently). Not sure
why doc sources would affect xjc.
##########
daffodil-lib/src/main/resources/org/apache/daffodil/xsd/tdml-core.xsd:
##########
@@ -0,0 +1,325 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<xsd:schema xmlns:xsd="http://www.w3.org/2001/XMLSchema";
+ xmlns:xs="http://www.w3.org/2001/XMLSchema";
+ xmlns="http://www.w3.org/2001/XMLSchema";
+ targetNamespace="http://www.ibm.com/xmlns/dfdl/testData";
+ xmlns:tns="http://www.ibm.com/xmlns/dfdl/testData";
+ elementFormDefault="unqualified"
+ xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/";
+ xmlns:daf="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:ext"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";>
+
+ <xs:attribute name="tutorialInclude" type="xs:string" fixed="no"/>
+
+<!--
+ This element's type is a stub that satisfies the need for xhtml5 validation
as part of TDML files.
+
+ It is just a wildcard with skip validation in it. So it's not really
validating anything.
+
+ Users of TDML creating tutorials should paste in known-correct xhtml5 as the
contents of the
+ tutorial elements.
+-->
+ <xs:element name="tutorial" substitutionGroup="tns:testSuiteChoices">
+ <xs:complexType mixed="true">
+ <xs:sequence>
+ <xs:any namespace="##any" processContents="skip" minOccurs="0"
maxOccurs="unbounded"/>
+ </xs:sequence>
+ <xs:anyAttribute namespace="##any" processContents="skip"/>
+ </xs:complexType>
+ </xs:element>
+
+ <!-- IBM uses this namespace http://www.ibm.com/xmlns/dfdl/testData -->
Review Comment:
This comment makes it looks like it's talking about testSuiteChoices but
it's not. I think it was intended to be about the testSuite element below,
which is in the tdml namespace?
Also, can you also add a comment explaining testSuiteChoices, substituion
groups, and why we need it?
My understanding is anything that references this element as a
substituionGroup can appear where this element is referenced? And those
elements are `tutorial`, `parserTestCase`, and `unparserTest` case in the
context of `tdml-core.xsd`, but additionally `defineSchema` and `defineConfig`
in the context of `tdml.xsd`, which includes this file?
##########
build.sbt:
##########
@@ -211,6 +212,43 @@ lazy val testStdLayout =
Project("daffodil-test-stdLayout", file("test-stdLayout
.dependsOn(tdmlProc % "test")
.settings(commonSettings, nopublish)
+/* Workaround: certain reflection (used by JAXB) isn't allowed by default in
JDK 17:
+ *
https://docs.oracle.com/en/java/javase/17/migrate/migrating-jdk-8-later-jdk-releases.html#GUID-7BB28E4D-99B3-4078-BDC4-FC24180CE82B
+ *
+ * While we can handle this JVM quirk at build time, at runtime we won't know
+ * a user's JVM version. We'll provide documentation and an extension setting
+ * to add these flags to the extension-launched debugger backend.
+ */
+lazy val extraJvmOptions: Seq[String] =
+ if (scala.util.Properties.isJavaAtLeast("17"))
+ Seq(
+ "--add-opens",
+ "java.base/java.lang=ALL-UNNAMED",
+ )
+ else Seq()
+
+lazy val xjcSettings =
+ Seq(
+ libraryDependencies ++= Seq(
+ "com.sun.xml.bind" % "jaxb-impl" % "2.2.11",
+ "javax.activation" % "activation" % "1.1.1",
+ "org.glassfish.jaxb" % "jaxb-xjc" % "2.2.11",
+ ),
+ xjcCommandLine += "-nv",
+ xjcCommandLine += "-p",
+ xjcCommandLine += "org.apache.daffodil.tdml",
Review Comment:
Can we spend some time trying to figure out the reproducible builds issue
with jaxb generated files that we saw with vscode? Daffodil currently has
reproducible builds for jars (the other zip/tar/etc release artifacts have
timestamp issues) and I would like to keep it that way. There's some
discussions in the ASF about automating things like release signing, but that
requires reproducible builds.
I see there's a `-no-header` xjc option that prevents a header comment
containing a timestamp from being added. I wouldn't expect a comment to affect
the .class file, so I'm not optimistic it would fix it, but at least it's one
less piece of randomness to consider.
##########
daffodil-lib/src/main/resources/org/apache/daffodil/xsd/tdml-core.xsd:
##########
@@ -0,0 +1,325 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<xsd:schema xmlns:xsd="http://www.w3.org/2001/XMLSchema";
+ xmlns:xs="http://www.w3.org/2001/XMLSchema";
+ xmlns="http://www.w3.org/2001/XMLSchema";
+ targetNamespace="http://www.ibm.com/xmlns/dfdl/testData";
+ xmlns:tns="http://www.ibm.com/xmlns/dfdl/testData";
+ elementFormDefault="unqualified"
+ xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/";
+ xmlns:daf="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:ext"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";>
+
+ <xs:attribute name="tutorialInclude" type="xs:string" fixed="no"/>
+
+<!--
+ This element's type is a stub that satisfies the need for xhtml5 validation
as part of TDML files.
+
+ It is just a wildcard with skip validation in it. So it's not really
validating anything.
+
+ Users of TDML creating tutorials should paste in known-correct xhtml5 as the
contents of the
+ tutorial elements.
+-->
+ <xs:element name="tutorial" substitutionGroup="tns:testSuiteChoices">
+ <xs:complexType mixed="true">
+ <xs:sequence>
+ <xs:any namespace="##any" processContents="skip" minOccurs="0"
maxOccurs="unbounded"/>
+ </xs:sequence>
+ <xs:anyAttribute namespace="##any" processContents="skip"/>
+ </xs:complexType>
+ </xs:element>
+
+ <!-- IBM uses this namespace http://www.ibm.com/xmlns/dfdl/testData -->
+ <element name="testSuiteChoices" abstract="true"/>
+
+ <element name="testSuite">
+ <complexType>
+ <sequence>
+ <xsd:element minOccurs="0" maxOccurs="unbounded"
ref="tns:testSuiteChoices"></xsd:element>
Review Comment:
For consistency with the rest of the file, can you remove the xsd prefix and
close with `/>`
##########
build.sbt:
##########
@@ -211,6 +212,43 @@ lazy val testStdLayout =
Project("daffodil-test-stdLayout", file("test-stdLayout
.dependsOn(tdmlProc % "test")
.settings(commonSettings, nopublish)
+/* Workaround: certain reflection (used by JAXB) isn't allowed by default in
JDK 17:
+ *
https://docs.oracle.com/en/java/javase/17/migrate/migrating-jdk-8-later-jdk-releases.html#GUID-7BB28E4D-99B3-4078-BDC4-FC24180CE82B
+ *
+ * While we can handle this JVM quirk at build time, at runtime we won't know
+ * a user's JVM version. We'll provide documentation and an extension setting
+ * to add these flags to the extension-launched debugger backend.
+ */
+lazy val extraJvmOptions: Seq[String] =
+ if (scala.util.Properties.isJavaAtLeast("17"))
+ Seq(
+ "--add-opens",
+ "java.base/java.lang=ALL-UNNAMED",
+ )
+ else Seq()
+
+lazy val xjcSettings =
+ Seq(
+ libraryDependencies ++= Seq(
+ "com.sun.xml.bind" % "jaxb-impl" % "2.2.11",
Review Comment:
Yeah, please define these in `project/Dependencies.scala`.
Also do we need all of these for Daffodil, or are some only needed for
generating the .java files? I'd assume we just need impl, and the other two are
for generating XML? But maybe that's not the case?
Also, if we are adding new dependencies, we must update the LICENSE/NOTICE
files (assuming these dependencies are ASF compatible).
##########
build.sbt:
##########
@@ -211,6 +212,43 @@ lazy val testStdLayout =
Project("daffodil-test-stdLayout", file("test-stdLayout
.dependsOn(tdmlProc % "test")
.settings(commonSettings, nopublish)
+/* Workaround: certain reflection (used by JAXB) isn't allowed by default in
JDK 17:
+ *
https://docs.oracle.com/en/java/javase/17/migrate/migrating-jdk-8-later-jdk-releases.html#GUID-7BB28E4D-99B3-4078-BDC4-FC24180CE82B
+ *
+ * While we can handle this JVM quirk at build time, at runtime we won't know
+ * a user's JVM version. We'll provide documentation and an extension setting
+ * to add these flags to the extension-launched debugger backend.
+ */
+lazy val extraJvmOptions: Seq[String] =
Review Comment:
I would suggest adding this logic directly to the `xjcJvmOpts` below, e.g
```scala
xjcJvmSettings ++= {
if (scala.util.Properties.isJavaAtLeast("17"))
Seq(
"--add-opens",
"java.base/java.lang=ALL-UNNAMED",
)
else Seq()
},
```
Otherwise it could be easily confused that these options are added to a JVM
doing compilation or running tests or something, which isn't the case. These
are only for running the xjc code generator.
##########
build.sbt:
##########
@@ -211,6 +212,43 @@ lazy val testStdLayout =
Project("daffodil-test-stdLayout", file("test-stdLayout
.dependsOn(tdmlProc % "test")
.settings(commonSettings, nopublish)
+/* Workaround: certain reflection (used by JAXB) isn't allowed by default in
JDK 17:
+ *
https://docs.oracle.com/en/java/javase/17/migrate/migrating-jdk-8-later-jdk-releases.html#GUID-7BB28E4D-99B3-4078-BDC4-FC24180CE82B
+ *
+ * While we can handle this JVM quirk at build time, at runtime we won't know
+ * a user's JVM version. We'll provide documentation and an extension setting
+ * to add these flags to the extension-launched debugger backend.
+ */
+lazy val extraJvmOptions: Seq[String] =
+ if (scala.util.Properties.isJavaAtLeast("17"))
+ Seq(
+ "--add-opens",
+ "java.base/java.lang=ALL-UNNAMED",
+ )
+ else Seq()
+
+lazy val xjcSettings =
+ Seq(
+ libraryDependencies ++= Seq(
+ "com.sun.xml.bind" % "jaxb-impl" % "2.2.11",
+ "javax.activation" % "activation" % "1.1.1",
+ "org.glassfish.jaxb" % "jaxb-xjc" % "2.2.11",
+ ),
+ xjcCommandLine += "-nv",
+ xjcCommandLine += "-p",
+ xjcCommandLine += "org.apache.daffodil.tdml",
+ xjcLibs := Seq(
+ "org.glassfish.jaxb" % "jaxb-xjc" % "2.2.11",
Review Comment:
Actually, looking a the plugin, the `xjcLibs` aren't compile or test. They
are actually added to a custom "xjc-tool" ivy configuration that is used to
download all the jars and fork xjc with a main class and arguments. So this is
actually correct.
That said, the plugin defines a default value of xjcLibs that is the same as
this, so this setting is just duplication and can be removed. This is only
needed if we want to use a different version of xjc.
##########
build.sbt:
##########
@@ -211,6 +212,43 @@ lazy val testStdLayout =
Project("daffodil-test-stdLayout", file("test-stdLayout
.dependsOn(tdmlProc % "test")
.settings(commonSettings, nopublish)
+/* Workaround: certain reflection (used by JAXB) isn't allowed by default in
JDK 17:
+ *
https://docs.oracle.com/en/java/javase/17/migrate/migrating-jdk-8-later-jdk-releases.html#GUID-7BB28E4D-99B3-4078-BDC4-FC24180CE82B
+ *
+ * While we can handle this JVM quirk at build time, at runtime we won't know
+ * a user's JVM version. We'll provide documentation and an extension setting
Review Comment:
I think it depends on where the reflection is happening. If it's only when
executing xjc to to generate the java files, then it should be fine to only be
here. If the reflection also happens when parsing XML to instantiate the Java
classes it's generated, then it probably is needed at runtime. From looking at
the xjcPlugin, my guess is that these options are only needed to during build,
so should be fine.
That said, we should add tests that actually try to parse a TDML file the
fields so we know this works on different JVMs without additional optinos.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
