stevedlawrence commented on code in PR #1452:
URL: https://github.com/apache/daffodil-vscode/pull/1452#discussion_r2411179268
##########
src/daffodilDebugger/utils.ts:
##########
@@ -59,33 +66,58 @@ export async function runDebugger(
dfdlDebugger: DFDLDebugger,
createTerminal: boolean = false
): Promise<vscode.Terminal> {
- const dfdlVersion = daffodilVersion(filePath)
+ if (!['2.12', '2.13', '3'].includes(dfdlDebugger.version)) {
+ vscode.window.showErrorMessage(
+ `DFDL Debugger Version ${dfdlDebugger.version} not supported. Supported
versions are 2.12, 2.13 and 3.`
Review Comment:
Calling this `dfdlDebugger.version` confused me a but. I would consider the
version of the dfdlDebugger to be what's in the VERSION file, but this seems to
be the scala version? Maybe just rename this scalaVersion or crossVersion to
make that clear it's more about the scala version and not the actual version?
##########
build.sbt:
##########
@@ -221,3 +183,122 @@ lazy val xjcSettings =
cachedFun(Set(daffodilLibJar)).toSeq
}
)
+
+lazy val `daffodil-debugger` = project
+ .in(file("."))
+ .settings(commonSettings, ratSettings)
+ .settings(publish / skip := true)
+ .aggregate(debuggers.projectRefs: _*)
+
+/** Since using projectMatrix, there will be a debugger, debugger2_12 and
debugger3 target. The debugger target is for
+ * Daffodil 3.11.0 and Scala 2.13. The debugger2_12 target is for Daffodil
3.10.0 and Scala 2.12. The debugger3 target
+ * is for Daffodil 4.0.0 and Scala 3. (only availabe when using JDK 17+)
+ *
+ * When running something like "sbt test" that will run all targets. To use a
single target do one of: sbt
+ * debugger/test OR sbt debugger2_12/test OR sbt debugger3/test. Based on
which version of the debugger you are
+ * targeting.
+ */
+lazy val debuggers = {
+ val debugger = (projectMatrix in (file("debugger")))
+ .enablePlugins(BuildInfoPlugin, JavaAppPackaging, UniversalPlugin,
ClasspathJarPlugin, SbtXjcPlugin)
+ .settings(commonSettings)
+ .settings(xjcSettings)
+ .settings(
+ name := "daffodil-debugger",
+ scalacOptions ++= buildScalacOptions(scalaVersion.value),
+ javacOptions ++= buildJavacOptions(scalaVersion.value),
+ libraryDependencies ++= Seq(
+ /* NOTE: To support Java 8:
+ * logback-classic can not go above version 1.2.11.
+ * com.microsoft.java.debug.core can not go above version 0.34.0.
+ * jansi can not go above version 1.18.
+ */
+ // scala-steward:off
+ "ch.qos.logback" % "logback-classic" % "1.2.11",
+ "com.microsoft.java" % "com.microsoft.java.debug.core" % "0.34.0",
+ "org.fusesource.jansi" % "jansi" % "1.18",
+ // scala-steward:on
+ "co.fs2" %% "fs2-io" % "3.12.0",
+ "com.monovore" %% "decline-effect" % "2.5.0",
+ "org.typelevel" %% "log4cats-slf4j" % "2.7.1",
+ "org.scalameta" %% "munit" % "1.1.1" % Test
+ ),
+ buildInfoPackage := "org.apache.daffodil.debugger.dap",
+ buildInfoKeys := Seq[BuildInfoKey](
+ name,
+ version,
+ scalaVersion,
+ sbtVersion,
+ "daffodilVersion" -> getDaffodilVersion(scalaVersion.value)
+ ),
+ packageName := s"${name.value}-${getDaffodilVersion(scalaVersion.value)}"
Review Comment:
This feels like it doesn't really want to include the daffodil Version. I
think we want to avoid getting into a situation where we have to have a
daffodil-debugger package for every version of Daffodil, which this including
the daffodil version in the package name kindof implies. We really only want
three versions--one for each the three binary scala versions. So this feels it
wants to be something like
```scala
packageName := s"${name.value}-${scalaBinaryVersion.value}"
```
##########
src/launchWizard/launchWizard.ts:
##########
@@ -535,10 +535,21 @@ class LaunchWizard {
})
let dfdlDebugger: DFDLDebugger = defaultValues.dfdlDebugger
+
+ let debuggerVersionSelect = ''
+ let debuggerAllowedVersions = ['2', '3']
+ let debuggerVersion = dfdlDebugger.version
+ debuggerAllowedVersions.forEach((version) => {
+ if (version === debuggerVersion) {
+ debuggerVersionSelect += `<option selected
value="${version}">${version}</option>`
+ } else {
+ debuggerVersionSelect += `<option
value="${version}">${version}</option>`
+ }
+ })
Review Comment:
Is this letting users select between version 2 and version 3? It feels like
users shouldn't be selecting the scala vesrsion, but should instead select the
daffodil version. For example, they could select "3.11.0" and then this
extension should be smart enough to pick the Scala 2.13 variant of the debugger
jar and set the classpath up using that.
##########
vite/package.vite.config.mjs:
##########
@@ -109,23 +121,38 @@ function copyToPkgDirPlugin() {
{ from: 'src/styles/styles.css', to: `${pkg_dir}/src/styles/styles.css` },
{ from: 'src/tdmlEditor/', to: `${pkg_dir}/src/tdmlEditor` },
]
- const serverPackageFolder = path.join(
- path.resolve('dist/package'),
- serverPackage
- )
-
- console.debug(`== [Vite] | serverPackageFolder: ${serverPackageFolder}`)
- // remove debugger package folder if exists
- if (fs.existsSync(serverPackageFolder)) {
- fs.rmSync(serverPackageFolder, { recursive: true })
+
+ const serverPackageFolders = {
+ scala2_12: path.join(
+ path.resolve('dist/package'),
+ `daffodil-debugger-${daffodilVersions['scala2.12']}-${pkg_version}`
+ ),
+ scala2_13: path.join(
+ path.resolve('dist/package'),
+ `daffodil-debugger-${daffodilVersions['scala2.13']}-${pkg_version}`
+ ),
+ scala3: path.join(
+ path.resolve('dist/package'),
+ `daffodil-debugger-${daffodilVersions['scala3']}-${pkg_version}`
+ ),
Review Comment:
This seems like were are going to be limited to only 3 daffodil versions
that are packaed with the extension, one for each of scala 2.12,2.13, and 3. Is
that the intention?
It feels like the goal of this wants to be to support any daffodil version,
past, present, and future. So the Scala 2.12 version of the debugger is used
for Daffodil 3.10.0 and older, the 2.13 version is used for Daffodil 3.11.0,
and the 3.0 version is used for Daffodil 4.0.0 and older.
It feels like maybe the right thing to do is to have the ability to download
the Daffodil version that the user wants. You can do this by downloading the
release, e.g.:
```https://www.apache.org/dyn/closer.lua/download/daffodil/{version}/bin/apache-daffodil-{version}-bin.zip```
You can extract the jars out of that and use those for the classpath.
Maybe part of building the vsix is to download the latest version of
Daffodil available at the time of release and include that in the vsix file so
it's kindof already available. The extension will default to that version, and
if acuser wants a different version the extension and download and cached
somewhere.
##########
debugger/src/main/scala-2.12/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -0,0 +1,799 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
Review Comment:
I'm concerned with how much duplication there is in the scala-2.12,
scala-2.13, and scala-3 directories. It looks like there is a lot more than
just API shims. There are a lot of logic in these files that feels like could
make maintencance very difficult, since any change made in one needs to be
duplicated in another.
Feels like these that actual logic could be split out into shared classes so
these are basically just shim layers.
##########
.github/workflows/release-candidate.yml:
##########
@@ -56,7 +56,7 @@ jobs:
uses: actions/setup-java@a7ab372554b6eb1a8eb25e7d9aec1cc9f3ea1a76 #
v4.6.0
with:
distribution: temurin
- java-version: 8
+ java-version: 17
Review Comment:
Note that we'll also need to update the [build release
container](https://github.com/apache/daffodil-infrastructure/tree/main/containers/build-release)
to use Java 17. Should just be a one-line change here:
https://github.com/apache/daffodil-infrastructure/blob/main/containers/build-release/src/daffodil-build-release#L108
##########
build.sbt:
##########
@@ -221,3 +183,122 @@ lazy val xjcSettings =
cachedFun(Set(daffodilLibJar)).toSeq
}
)
+
+lazy val `daffodil-debugger` = project
+ .in(file("."))
+ .settings(commonSettings, ratSettings)
+ .settings(publish / skip := true)
+ .aggregate(debuggers.projectRefs: _*)
+
+/** Since using projectMatrix, there will be a debugger, debugger2_12 and
debugger3 target. The debugger target is for
+ * Daffodil 3.11.0 and Scala 2.13. The debugger2_12 target is for Daffodil
3.10.0 and Scala 2.12. The debugger3 target
+ * is for Daffodil 4.0.0 and Scala 3. (only availabe when using JDK 17+)
+ *
+ * When running something like "sbt test" that will run all targets. To use a
single target do one of: sbt
+ * debugger/test OR sbt debugger2_12/test OR sbt debugger3/test. Based on
which version of the debugger you are
+ * targeting.
+ */
+lazy val debuggers = {
+ val debugger = (projectMatrix in (file("debugger")))
+ .enablePlugins(BuildInfoPlugin, JavaAppPackaging, UniversalPlugin,
ClasspathJarPlugin, SbtXjcPlugin)
+ .settings(commonSettings)
+ .settings(xjcSettings)
+ .settings(
+ name := "daffodil-debugger",
+ scalacOptions ++= buildScalacOptions(scalaVersion.value),
+ javacOptions ++= buildJavacOptions(scalaVersion.value),
+ libraryDependencies ++= Seq(
+ /* NOTE: To support Java 8:
+ * logback-classic can not go above version 1.2.11.
+ * com.microsoft.java.debug.core can not go above version 0.34.0.
+ * jansi can not go above version 1.18.
+ */
+ // scala-steward:off
+ "ch.qos.logback" % "logback-classic" % "1.2.11",
+ "com.microsoft.java" % "com.microsoft.java.debug.core" % "0.34.0",
+ "org.fusesource.jansi" % "jansi" % "1.18",
+ // scala-steward:on
+ "co.fs2" %% "fs2-io" % "3.12.0",
+ "com.monovore" %% "decline-effect" % "2.5.0",
+ "org.typelevel" %% "log4cats-slf4j" % "2.7.1",
+ "org.scalameta" %% "munit" % "1.1.1" % Test
+ ),
+ buildInfoPackage := "org.apache.daffodil.debugger.dap",
+ buildInfoKeys := Seq[BuildInfoKey](
+ name,
+ version,
+ scalaVersion,
+ sbtVersion,
+ "daffodilVersion" -> getDaffodilVersion(scalaVersion.value)
+ ),
+ packageName := s"${name.value}-${getDaffodilVersion(scalaVersion.value)}"
+ )
+ .jvmPlatform(
+ scalaVersions = Seq("2.12.20"),
+ settings = Seq(
+ libraryDependencies ++=
getPlatformSpecificLibraries(scalaVersion.value)
+ )
+ )
+ .jvmPlatform(
+ scalaVersions = Seq("2.13.16"),
+ settings = Seq(
+ libraryDependencies ++=
getPlatformSpecificLibraries(scalaVersion.value)
+ )
+ )
+
+ if (scala.util.Properties.isJavaAtLeast("17"))
+ debugger.jvmPlatform(
+ scalaVersions = Seq("3.3.6"),
+ settings = Seq(
+ libraryDependencies ++=
getPlatformSpecificLibraries(scalaVersion.value)
+ )
+ )
+ else debugger
+}
+
+def getPlatformSpecificLibraries(scalaVersion: String) = {
+ val daffodilVersion = getDaffodilVersion(scalaVersion)
+
+ if (scalaVersion.startsWith("3"))
+ Seq("org.apache.daffodil" %% "daffodil-core" % daffodilVersion)
+ else
+ Seq(
+ "org.apache.daffodil" %% "daffodil-sapi" % daffodilVersion,
+ "org.apache.daffodil" %% "daffodil-runtime1" % daffodilVersion,
+ "org.apache.daffodil" %% "daffodil-lib" % daffodilVersion % Test
+ )
Review Comment:
Do these want to be marked as provided dependencies? I don't think we really
want to include the daffodil jars into the projects because then you can only
ever support three versions of Daffodil, one for each major scala version.
Instead we want three debugger projects, one for each Scala version (2.12,
2.13, and 3), and those can be used for any version of Daffodil if setup
correctly.
##########
build.sbt:
##########
@@ -221,3 +183,122 @@ lazy val xjcSettings =
cachedFun(Set(daffodilLibJar)).toSeq
}
)
+
+lazy val `daffodil-debugger` = project
+ .in(file("."))
+ .settings(commonSettings, ratSettings)
+ .settings(publish / skip := true)
+ .aggregate(debuggers.projectRefs: _*)
+
+/** Since using projectMatrix, there will be a debugger, debugger2_12 and
debugger3 target. The debugger target is for
+ * Daffodil 3.11.0 and Scala 2.13. The debugger2_12 target is for Daffodil
3.10.0 and Scala 2.12. The debugger3 target
+ * is for Daffodil 4.0.0 and Scala 3. (only availabe when using JDK 17+)
+ *
+ * When running something like "sbt test" that will run all targets. To use a
single target do one of: sbt
+ * debugger/test OR sbt debugger2_12/test OR sbt debugger3/test. Based on
which version of the debugger you are
+ * targeting.
+ */
+lazy val debuggers = {
+ val debugger = (projectMatrix in (file("debugger")))
+ .enablePlugins(BuildInfoPlugin, JavaAppPackaging, UniversalPlugin,
ClasspathJarPlugin, SbtXjcPlugin)
+ .settings(commonSettings)
+ .settings(xjcSettings)
+ .settings(
+ name := "daffodil-debugger",
+ scalacOptions ++= buildScalacOptions(scalaVersion.value),
+ javacOptions ++= buildJavacOptions(scalaVersion.value),
+ libraryDependencies ++= Seq(
+ /* NOTE: To support Java 8:
+ * logback-classic can not go above version 1.2.11.
+ * com.microsoft.java.debug.core can not go above version 0.34.0.
+ * jansi can not go above version 1.18.
+ */
+ // scala-steward:off
+ "ch.qos.logback" % "logback-classic" % "1.2.11",
+ "com.microsoft.java" % "com.microsoft.java.debug.core" % "0.34.0",
+ "org.fusesource.jansi" % "jansi" % "1.18",
+ // scala-steward:on
+ "co.fs2" %% "fs2-io" % "3.12.0",
+ "com.monovore" %% "decline-effect" % "2.5.0",
+ "org.typelevel" %% "log4cats-slf4j" % "2.7.1",
+ "org.scalameta" %% "munit" % "1.1.1" % Test
+ ),
+ buildInfoPackage := "org.apache.daffodil.debugger.dap",
+ buildInfoKeys := Seq[BuildInfoKey](
+ name,
+ version,
+ scalaVersion,
+ sbtVersion,
+ "daffodilVersion" -> getDaffodilVersion(scalaVersion.value)
+ ),
+ packageName := s"${name.value}-${getDaffodilVersion(scalaVersion.value)}"
+ )
+ .jvmPlatform(
+ scalaVersions = Seq("2.12.20"),
+ settings = Seq(
+ libraryDependencies ++=
getPlatformSpecificLibraries(scalaVersion.value)
+ )
+ )
+ .jvmPlatform(
+ scalaVersions = Seq("2.13.16"),
+ settings = Seq(
+ libraryDependencies ++=
getPlatformSpecificLibraries(scalaVersion.value)
+ )
+ )
+
+ if (scala.util.Properties.isJavaAtLeast("17"))
+ debugger.jvmPlatform(
+ scalaVersions = Seq("3.3.6"),
+ settings = Seq(
+ libraryDependencies ++=
getPlatformSpecificLibraries(scalaVersion.value)
+ )
+ )
+ else debugger
+}
+
+def getPlatformSpecificLibraries(scalaVersion: String) = {
+ val daffodilVersion = getDaffodilVersion(scalaVersion)
+
+ if (scalaVersion.startsWith("3"))
+ Seq("org.apache.daffodil" %% "daffodil-core" % daffodilVersion)
+ else
+ Seq(
+ "org.apache.daffodil" %% "daffodil-sapi" % daffodilVersion,
+ "org.apache.daffodil" %% "daffodil-runtime1" % daffodilVersion,
+ "org.apache.daffodil" %% "daffodil-lib" % daffodilVersion % Test
+ )
+}
+
+def getMinSupportedJavaVersion(scalaVersion: String): String =
+ if (scalaVersion.startsWith("3")) "17"
+ else if (scala.util.Properties.isJavaAtLeast("21")) "11"
+ else "8"
+
+def buildJavacOptions(scalaVersion: String) = {
+ val jdkCompat = getMinSupportedJavaVersion(scalaVersion)
+
+ if (jdkCompat == "8")
+ Seq("-source", jdkCompat, "-target", jdkCompat)
+ else
+ Seq("--release", jdkCompat)
+}
+
+def buildScalacOptions(scalaVersion: String) = {
+ val jdkCompat = getMinSupportedJavaVersion(scalaVersion)
+
+ val commonSettings = Seq(if (scalaVersion.startsWith("2.12"))
"-Ypartial-unification" else "")
+
+ if (scalaVersion.startsWith("2.12") && jdkCompat == "8")
+ commonSettings ++ Seq(s"--target:jvm-${jdkCompat}")
+ else
+ commonSettings ++ Seq("--release", jdkCompat)
+}
+
+def getScalaVersionKey(scalaVersion: String) =
+ "scala" + scalaVersion.split('.').dropRight(if
(scalaVersion.startsWith("3")) 2 else 1).mkString(".")
+
+def getDaffodilVersion(scalaVersion: String) =
daffodilVers(getScalaVersionKey(scalaVersion))
+
+def getDaffodilJarName(scalaVersion: String) =
+ if (scalaVersion.startsWith("3")) "daffodil-core"
+ else "daffodil-lib"
Review Comment:
I think it might clean things up if you pass in `scalaBinaryVersion.value`
into all of these functions instead of `scalaVersion.value`. The
`scalaBinaryVersion` setting is one of "2.12", "2.13", or "3", so alot of the
starts with or drop right stuff can be be replaced with just straight string
comparisons which would probably be more clear.
--
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]
