stevedlawrence commented on code in PR #1471:
URL: https://github.com/apache/daffodil-vscode/pull/1471#discussion_r2455050403
##########
vite.config.mjs:
##########
@@ -45,41 +44,32 @@ const packageData = jsoncParse(
const pkg_version = packageData['version']
const scalaVersions = ['2.12', '2.13', '3']
-function unzipAfterBuild() {
+function copyDebuggerOutAfterBuild() {
return {
- name: 'unzip-server-package',
+ name: 'copy-debugger-package',
apply: 'build',
async closeBundle() {
scalaVersions.forEach(async (scalaVersion) => {
const serverPackage =
`daffodil-debugger-${scalaVersion}-${pkg_version}`
const jvmFolderName = `jvm-${scalaVersion}`
- const zipFilePath = path.resolve(
- `debugger/target/${jvmFolderName}/universal/${serverPackage}.zip`
+ const stageFilePath = path.resolve(
+ `debugger/target/${jvmFolderName}/universal/stage`
)
- const serverPackageFolder = path.join(
- path.resolve('dist/package'),
- serverPackage
- )
+ const serverPackageFolder = path.join('dist/debuggers', serverPackage)
// remove debugger package folder if exists
if (fs.existsSync(serverPackageFolder)) {
fs.rmSync(serverPackageFolder, { recursive: true, force: true })
}
// if the debugger package doesn't exist continue
Review Comment:
Might be worth adding a comment that this should only happen on versions
older than JDK17 and the Scala 3 version, simlar to a comment below.
##########
vite.config.mjs:
##########
@@ -122,12 +112,19 @@ function copyToPkgDirPlugin() {
const serverPackageFolders = []
scalaVersions.forEach((scalaVersion) => {
- serverPackageFolders.push(
- path.join(
- path.resolve('dist/package'),
- `daffodil-debugger-${scalaVersion}-${pkg_version}`
- )
+ const serverPackage = `daffodil-debugger-${scalaVersion}-${pkg_version}`
+ const jvmFolderName = `jvm-${scalaVersion}`
+ const stageFilePath = path.resolve(
+ `debugger/target/${jvmFolderName}/universal/stage`
+ )
+ const serverPackageFolder = path.join(
+ path.resolve('dist/package/dist/debuggers'),
Review Comment:
Is the extra `dist` needed because the extension always wants to look in the
same directory for debuggers (and you've chosen `dist/debuggers`), it just has
a different root when debugging the extension vs running it normally? So when
debugging the extension the root is the repo and when running normally the root
is effectively `dist/package/`?
The extra dist is fine (and it looks like there is already some other stuff
in dist/package/dist), just wanting to make sure I understand.
##########
src/tests/suite/daffodilDebugger.test.ts:
##########
@@ -69,9 +69,8 @@ suite('Daffodil Debugger', function (this: Suite) {
*/
for (var i = 0; i < dfdlDebuggerConfigs.length; i++) {
let newDebugger = await runDebugger(
- PROJECT_ROOT,
+ path.join(PROJECT_ROOT, 'dist/debuggers'),
Review Comment:
This means that in order to run tests the staged debuggers have to be copied
to `<repo_root>/dist/debugger`, right? So coping to `dist/debugger` isn't a
thing needed just for debugging, but also to run tests. So it's kindof required
if you want to do any real development?
##########
vite.config.mjs:
##########
@@ -122,12 +112,23 @@ function copyToPkgDirPlugin() {
const serverPackageFolders = []
scalaVersions.forEach((scalaVersion) => {
- serverPackageFolders.push(
- path.join(
- path.resolve('dist/package'),
- `daffodil-debugger-${scalaVersion}-${pkg_version}`
- )
+ const serverPackage = `daffodil-debugger-${scalaVersion}-${pkg_version}`
+ const jvmFolderName = `jvm-${scalaVersion}`
+ const stageFilePath = path.resolve(
+ `debugger/target/${jvmFolderName}/universal/stage`
+ )
+ const serverPackageFolder = path.join(
+ path.resolve('dist/package/dist/debuggers'),
+ serverPackage
)
+
+ // Only push the debugger path if the staged file path exists
+ // When using jdk <= 17 scala 3 shouldn't be mapped as that debugger won't
be built.
Review Comment:
This should be < instead of <=. Daffodi lwith JDK 17 supports Scala 3.
Alternatively, is it possible for typescript to detect the java version?
Then you could make it so the `scalaVersions` array doesn't contain '3' for
older versions of java and these checks aren't needed. And then things can fail
if they are unexpectedly missing. Not sure how easy it would be to reliable
detect the java version though in ts, so might not be worth it.
##########
vite.config.mjs:
##########
@@ -122,12 +112,23 @@ function copyToPkgDirPlugin() {
const serverPackageFolders = []
scalaVersions.forEach((scalaVersion) => {
- serverPackageFolders.push(
- path.join(
- path.resolve('dist/package'),
- `daffodil-debugger-${scalaVersion}-${pkg_version}`
- )
+ const serverPackage = `daffodil-debugger-${scalaVersion}-${pkg_version}`
+ const jvmFolderName = `jvm-${scalaVersion}`
+ const stageFilePath = path.resolve(
+ `debugger/target/${jvmFolderName}/universal/stage`
+ )
+ const serverPackageFolder = path.join(
+ path.resolve('dist/package/dist/debuggers'),
+ serverPackage
)
+
+ // Only push the debugger path if the staged file path exists
+ // When using jdk <= 17 scala 3 shouldn't be mapped as that debugger won't
be built.
+ if (fs.existsSync(stageFilePath)) {
+ serverPackageFolders.push(serverPackageFolder)
+
+ patterns.push({ from: stageFilePath, to: serverPackageFolder })
+ }
})
Review Comment:
If we want to simplify things and remove all/most of this logic surrounding
serverPackageFolders, and since it feels like `<repo_root>/dist/debugger`
kindof always wants to exist for developing/testing, maybe we just add a
pattern like:
```ts
{ from: 'dist/debuggers', to: '${pkg_dir}/dist/debuggers' }
```
So we always copy to `<repo_root>/dist/debuggers` and then copy that to
`pkg_dir` for packaging. There's some extra copying, which isn't great, but it
kindof feels unavoidable for any real testing/development. And you've fixed the
polluting of the root repo which was the main thing I didn't love.
It also reduces differences between dev and prod build, which is pretty
ideal. With this change, it looks like the to two are almost exactly the same
except for one or two things, which is great.
##########
vite/package.vite.config.mjs:
##########
@@ -45,41 +44,32 @@ const packageData = jsoncParse(
const pkg_version = packageData['version']
const scalaVersions = ['2.12', '2.13', '3']
-function unzipAfterBuild() {
+function copyDebuggerOutAfterBuild() {
return {
- name: 'unzip-server-package',
+ name: 'copy-debugger-package',
apply: 'build',
async closeBundle() {
scalaVersions.forEach(async (scalaVersion) => {
const serverPackage =
`daffodil-debugger-${scalaVersion}-${pkg_version}`
const jvmFolderName = `jvm-${scalaVersion}`
- const zipFilePath = path.resolve(
Review Comment:
There's a entry in the .gitignore file that has this:
```bash
# ignore tests unzip daffodil-debugger
daffodil-debugger-*
**/bin
**/lib
```
I think that was for these things getting copied to the root of the repo?
Now that those are in `dist/` can those lines be removed since `dist/` is
already listed ignored?
##########
vite.config.mjs:
##########
@@ -142,72 +143,70 @@ function copyToPkgDirPlugin() {
name: 'copy-patterns-plugin',
apply: 'build',
async buildStart(opts) {
- if (!fs.existsSync(pkg_dir)) {
- serverPackageFolders.forEach((serverPackageFolder) => {
- fs.mkdirSync(serverPackageFolder, { recursive: true })
- })
- fs.mkdirSync(pkg_dir + '/dist')
- fs.mkdirSync(pkg_dir + '/src/language', {
- recursive: true,
- })
- fs.mkdirSync(pkg_dir + '/src/language/providers', {
- recursive: true,
- })
- fs.mkdirSync(pkg_dir + '/src/language/providers/intellisense/', {
- recursive: true,
- })
- fs.mkdirSync(pkg_dir + '/src/launchWizard', {
- recursive: true,
- })
- fs.mkdirSync(pkg_dir + '/src/styles', {
- recursive: true,
- })
- fs.mkdirSync(pkg_dir + '/src/tdmlEditor', {
- recursive: true,
- })
- }
+ ;[
+ pkg_dir + '/dist',
+ pkg_dir + '/src/language',
+ pkg_dir + '/src/language/providers',
+ pkg_dir + '/src/language/providers/intellisense/',
+ pkg_dir + '/src/launchWizard',
+ pkg_dir + '/src/styles',
+ pkg_dir + '/src/tdmlEditor',
+ ...serverPackageFolders,
+ ].forEach((folder) => {
+ if (!fs.existsSync(folder)) {
+ fs.mkdirSync(folder, { recursive: true })
+ }
+ })
for (const { from, to } of patterns) {
- from.includes('.')
+ fs.statSync(from).isFile()
? fs.copyFileSync(from, to)
: fs.cpSync(from, to, { recursive: true })
}
},
}
}
-const shouldMinify = process.env.MINIFY === '1'
-export default defineConfig({
- resolve: {
- alias: {
- ...localModuleAliases,
- },
- extensions: ['.ts', '.js'],
- },
- build: {
- sourcemap: true,
-
- minify: shouldMinify ? 'esbuild' : false,
-
- outDir: path.resolve(__dirname, '../dist/package/dist/ext'),
- emptyOutDir: true,
+const shouldMinify = process.env.MINIFY === '1'
Review Comment:
Does this want to be controlled via production vs development mode rather
than an env variable? Or is the MINIFY env variable the standard way to do
this? Should we be enabling MINIFY as part of the release-candidate script?
--
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]
