stevedlawrence commented on code in PR #232:
URL: https://github.com/apache/daffodil-vscode/pull/232#discussion_r923964938
##########
build/scripts/process_build.ts:
##########
@@ -18,39 +18,98 @@
// @ts-nocheck <-- This is needed as this file is basically a JavaScript script
// but with some TypeScript niceness baked in
const fs = require('fs')
+const path = require('path')
+const glob = require('glob')
const execSync = require('child_process').execSync
+const pkg_dir = 'dist/package'
+var fileCount = 0
-function prebuild() {
- fs.renameSync('LICENSE', 'tmp.LICENSE')
- fs.renameSync('NOTICE', 'tmp.NOTICE')
- fs.copyFileSync('build/bin.NOTICE', 'NOTICE')
- fs.copyFileSync('build/bin.LICENSE', 'LICENSE')
+function copyGlob(pattern, skipDirectory = false) {
Review Comment:
skipDirectory is maybe a bit confusing, especially on line 32? I *think* the
way it works is it flattens a directory structure and just copies all files?
Currently that doesn't matter since the globs where we set skipDirectory = true
are already flat. But I wonder if that might change in the future and we'll
want to recursively copy the full `build/package` directory structure?
What about an alternative approach where we change the directory where the
glob is run using the `cwd` glob option and then fix the paths to copy to/from
the right locations? Something like this (untested):
```node
function copyGlob(pattern, dir = ".") {
glob(pattern, {cwd=dir}, (error, files) => {
for (var i = 0; i < files.length; i++) {
let src = path.join(dir, file[i])
let dst = path.join(pkg_dir, file[i])
let parentDir = path.dirname(dst)
... // ensure parentDir exists
fileCopy(src, dst)
}
})
}
```
Then the usage to copy the build/package something like this:
```node
copyGlob("*", "build/package/")
```
The other `copyGlob` uses don't need to change since they already copy from
cwd.
##########
package.json:
##########
@@ -25,21 +25,19 @@
},
"scripts": {
"omega-edit-download": "node -e
\"require('./build/scripts/omega_edit_download.ts').downloadServer()\"",
- "vscode:prepublish": "yarn run package-ext",
"precompile": "node -p \"'export const LIB_VERSION = ' +
JSON.stringify(require('./package.json').version) + ';'\" > src/version.ts",
"compile": "tsc -p ./ && yarn omega-edit-download",
"lint": "yarn run prettier src -c",
"watch": "yarn omega-edit-download && webpack --watch --devtool
nosources-source-map --config ./build/extension.webpack.config.js",
"watch2": "tsc -watch -p ./",
- "package": "vsce package",
+ "webpack": "webpack --mode production --config
./build/extension.webpack.config.js",
+ "package": "vsce package",
"publish": "vsce publish",
- "package-ext": "webpack --mode production --config
./build/extension.webpack.config.js",
"pretest": "yarn run compile",
"test": "node node_modules/mocha/bin/_mocha -u tdd --timeout 999999
--colors ./out/tests",
"sbt": "sbt universal:packageBin",
- "prebuild": "node -e
\"require('./build/scripts/process_build.ts').prebuild()\"",
- "build": "yarn sbt && yarn install && yarn compile && yarn package",
- "postbuild": "node -e
\"require('./build/scripts/process_build.ts').postbuild()\""
+ "prebuild": "yarn sbt && yarn install && yarn compile && yarn webpack",
+ "build": "node -e
\"require('./build/scripts/process_build.ts').build_package()\""
Review Comment:
Just out of curiosity, what's the intended difference between
compile/precompile and build/prebuild? What kind of things should happen in one
and what should happen in the other? The two seems like similar concepts and
it's not clear to me what the difference is.
For example, yarn omega-edit-download happens in compile, but that's not
actually compiling anything, right? It just downloads a zip? Seems like maybe
that wants to happen in a build or prebuild stage? Likewise, "yarn sbt" does
potentially compile scala code, so feels like it wants to happen in compile
stage? Or maybe "sbt compile" should happen in compile stage, and "sbt
universal:packageBin" should happen in the build stage?
I'm not sure it really matters, but it's not totally clear to me. Just
trying my best to understand yarn.
##########
build/scripts/process_build.ts:
##########
@@ -18,39 +18,83 @@
// @ts-nocheck <-- This is needed as this file is basically a JavaScript script
// but with some TypeScript niceness baked in
const fs = require('fs')
-const execSync = require('child_process').execSync
+const path = require('path')
+const glob = require('glob')
+const pkg_dir = 'dist/package'
-function prebuild() {
- fs.renameSync('LICENSE', 'tmp.LICENSE')
- fs.renameSync('NOTICE', 'tmp.NOTICE')
- fs.copyFileSync('build/bin.NOTICE', 'NOTICE')
- fs.copyFileSync('build/bin.LICENSE', 'LICENSE')
+function copyGlob(pattern, skipDirectory = false) {
+ glob(pattern, (error, files) => {
+ for (var i = 0; i < files.length; i++) {
+ let file = !skipDirectory
+ ? files[i]
+ : files[i].split('/')[files[i].split('/').length - 1]
+
+ let directory = path.join(pkg_dir, getDir(file))
+
+ if (!fs.existsSync(directory)) {
+ fs.mkdirSync(directory, { recursive: true })
+ }
+
+ fileCopy(file, `${pkg_dir}/${file}`)
+ }
+ })
+}
+
+function getDir(file) {
+ return file
+ .split('/')
+ .slice(0, file.split('/').length - 1)
+ .join('/')
}
-function postbuild() {
- fs.rmSync('LICENSE')
- fs.rmSync('NOTICE')
- fs.renameSync('tmp.LICENSE', 'LICENSE')
- fs.renameSync('tmp.NOTICE', 'NOTICE')
-
- // This will make sure that if the root LICENSE and NOTICE are the same as
the build LICENSE
- // and NOTICE that they are reverted back to their original contents.
- if (
- fs.readFileSync('build/bin.LICENSE').toString() ===
- fs.readFileSync('LICENSE').toString()
- ) {
- execSync('git checkout LICENSE')
+function fileCopy(srcPath, destPath) {
+ if (fs.statSync(srcPath).isFile()) {
+ fs.copyFileSync(srcPath, destPath)
}
+}
+
+function setup_package() {
+ if (fs.existsSync(pkg_dir)) {
+ fs.rmdirSync(pkg_dir, { recursive: true })
+ }
+
+ fs.mkdirSync(pkg_dir)
- if (
- fs.readFileSync('build/bin.NOTICE').toString() ===
- fs.readFileSync('NOTICE').toString()
- ) {
- execSync('git checkout NOTICE')
+ let lines = fs
+ .readFileSync('build/package/.vscodeignore')
+ .toString()
+ .split('\n')
+
+ // Copy all files listed in the .vscodeignore
+ for (var i = 0; i < lines.length; i++) {
+ var line = lines[i]
+
+ if (!line.startsWith('!')) continue
+
+ let pattern = line.substring(1)
+
+ if (pattern.includes('*')) {
+ copyGlob(pattern)
+ } else {
+ let file = pattern
+ let directory = getDir(file)
+
+ fs.mkdirSync(`${pkg_dir}/${directory}`, { recursive: true })
+
+ fileCopy(file, `${pkg_dir}/${file}`)
+ }
}
+
+ // Copy ignore file
+ fileCopy('build/package/.vscodeignore', `${pkg_dir}/.vscodeignore`)
+
+ // Copy required package files into package directory
+ copyGlob('build/package/*', true)
Review Comment:
Thoughts on changing to this so we copy all hidden files if we ever add more?
```
copyGlob('build/package/*', true)
copyGlob('build/package/.*', true)
```
The first line copies all normal files, the second line copies all hidden
files.
##########
build/scripts/process_build.ts:
##########
@@ -18,39 +18,98 @@
// @ts-nocheck <-- This is needed as this file is basically a JavaScript script
// but with some TypeScript niceness baked in
const fs = require('fs')
+const path = require('path')
+const glob = require('glob')
const execSync = require('child_process').execSync
+const pkg_dir = 'dist/package'
+var fileCount = 0
-function prebuild() {
- fs.renameSync('LICENSE', 'tmp.LICENSE')
- fs.renameSync('NOTICE', 'tmp.NOTICE')
- fs.copyFileSync('build/bin.NOTICE', 'NOTICE')
- fs.copyFileSync('build/bin.LICENSE', 'LICENSE')
+function copyGlob(pattern, skipDirectory = false) {
+ glob(pattern, (error, files) => {
+ for (var i = 0; i < files.length; i++) {
+ let file = !skipDirectory
+ ? files[i]
+ : files[i].split('/')[files[i].split('/').length - 1]
+
+ let directory = path.join(pkg_dir, getDir(file))
+
+ if (!fs.existsSync(directory)) {
+ fs.mkdirSync(directory, { recursive: true })
+ }
+
+ fileCopy(file, `${pkg_dir}/${file}`)
+ fileCount++
+ }
+ })
+}
+
+function getDir(file) {
+ return file
+ .split('/')
+ .slice(0, file.split('/').length - 1)
+ .join('/')
+}
+
+function fileCopy(srcPath, destPath) {
+ if (fs.statSync(srcPath).isFile()) {
+ fs.copyFileSync(srcPath, destPath)
+ }
+}
+
+function create_package() {
+ // wait to run these commands till all files have been copied
+ while (fileCount < fs.readdirSync(pkg_dir).length) {
Review Comment:
I'm not sure I understand this loop and I'm not sure the comment matches?
Seems like we keep running yarn install/yarn package until the fileCount
changes?
##########
build/scripts/process_build.ts:
##########
@@ -18,39 +18,98 @@
// @ts-nocheck <-- This is needed as this file is basically a JavaScript script
// but with some TypeScript niceness baked in
const fs = require('fs')
+const path = require('path')
+const glob = require('glob')
const execSync = require('child_process').execSync
+const pkg_dir = 'dist/package'
+var fileCount = 0
-function prebuild() {
- fs.renameSync('LICENSE', 'tmp.LICENSE')
- fs.renameSync('NOTICE', 'tmp.NOTICE')
- fs.copyFileSync('build/bin.NOTICE', 'NOTICE')
- fs.copyFileSync('build/bin.LICENSE', 'LICENSE')
+function copyGlob(pattern, skipDirectory = false) {
+ glob(pattern, (error, files) => {
+ for (var i = 0; i < files.length; i++) {
+ let file = !skipDirectory
+ ? files[i]
+ : files[i].split('/')[files[i].split('/').length - 1]
+
+ let directory = path.join(pkg_dir, getDir(file))
+
+ if (!fs.existsSync(directory)) {
+ fs.mkdirSync(directory, { recursive: true })
+ }
+
+ fileCopy(file, `${pkg_dir}/${file}`)
+ fileCount++
+ }
+ })
+}
+
+function getDir(file) {
+ return file
+ .split('/')
+ .slice(0, file.split('/').length - 1)
+ .join('/')
+}
+
+function fileCopy(srcPath, destPath) {
+ if (fs.statSync(srcPath).isFile()) {
+ fs.copyFileSync(srcPath, destPath)
+ }
+}
+
+function create_package() {
+ // wait to run these commands till all files have been copied
+ while (fileCount < fs.readdirSync(pkg_dir).length) {
+ execSync('yarn install', { cwd: pkg_dir })
+ execSync('yarn package --out ../../', { cwd: pkg_dir })
+ }
}
-function postbuild() {
- fs.rmSync('LICENSE')
- fs.rmSync('NOTICE')
- fs.renameSync('tmp.LICENSE', 'LICENSE')
- fs.renameSync('tmp.NOTICE', 'NOTICE')
-
- // This will make sure that if the root LICENSE and NOTICE are the same as
the build LICENSE
- // and NOTICE that they are reverted back to their original contents.
- if (
- fs.readFileSync('build/bin.LICENSE').toString() ===
- fs.readFileSync('LICENSE').toString()
- ) {
- execSync('git checkout LICENSE')
+function build_package() {
+ if (fs.existsSync(pkg_dir)) {
+ fs.rmdirSync(pkg_dir, { recursive: true })
}
- if (
- fs.readFileSync('build/bin.NOTICE').toString() ===
- fs.readFileSync('NOTICE').toString()
- ) {
- execSync('git checkout NOTICE')
+ fs.mkdirSync(pkg_dir)
+
+ let lines = fs
+ .readFileSync('build/package/.vscodeignore')
+ .toString()
+ .split('\n')
+
+ // Copy all files listed in the .vscodeignore
+ for (var i = 0; i < lines.length; i++) {
+ var line = lines[i]
+
+ if (!line.startsWith('!')) continue
+
+ let pattern = line.substring(1).trim()
+
+ if (pattern.includes('*')) {
+ copyGlob(pattern)
+ } else {
+ let file = pattern
+ let directory = getDir(file)
+
+ fs.mkdirSync(`${pkg_dir}/${directory}`, { recursive: true })
+
+ fileCopy(file, `${pkg_dir}/${file}`)
+ fileCount++
+ }
}
+
+ // Copy ignore file
+ fileCopy('build/package/.vscodeignore', `${pkg_dir}/.vscodeignore`)
Review Comment:
I think this isn't needed? It should be copied by the glob with the `{.,}`
stuff?
##########
build/scripts/process_build.ts:
##########
@@ -18,39 +18,98 @@
// @ts-nocheck <-- This is needed as this file is basically a JavaScript script
// but with some TypeScript niceness baked in
const fs = require('fs')
+const path = require('path')
+const glob = require('glob')
const execSync = require('child_process').execSync
+const pkg_dir = 'dist/package'
+var fileCount = 0
-function prebuild() {
- fs.renameSync('LICENSE', 'tmp.LICENSE')
- fs.renameSync('NOTICE', 'tmp.NOTICE')
- fs.copyFileSync('build/bin.NOTICE', 'NOTICE')
- fs.copyFileSync('build/bin.LICENSE', 'LICENSE')
+function copyGlob(pattern, skipDirectory = false) {
+ glob(pattern, (error, files) => {
+ for (var i = 0; i < files.length; i++) {
+ let file = !skipDirectory
+ ? files[i]
+ : files[i].split('/')[files[i].split('/').length - 1]
+
+ let directory = path.join(pkg_dir, getDir(file))
+
+ if (!fs.existsSync(directory)) {
+ fs.mkdirSync(directory, { recursive: true })
+ }
+
+ fileCopy(file, `${pkg_dir}/${file}`)
+ fileCount++
+ }
+ })
+}
+
+function getDir(file) {
Review Comment:
Does this do the same thing as `path.dirname()`? Can we use that instead?
##########
package.json:
##########
@@ -25,21 +25,19 @@
},
"scripts": {
"omega-edit-download": "node -e
\"require('./build/scripts/omega_edit_download.ts').downloadServer()\"",
- "vscode:prepublish": "yarn run package-ext",
"precompile": "node -p \"'export const LIB_VERSION = ' +
JSON.stringify(require('./package.json').version) + ';'\" > src/version.ts",
"compile": "tsc -p ./ && yarn omega-edit-download",
"lint": "yarn run prettier src -c",
"watch": "yarn omega-edit-download && webpack --watch --devtool
nosources-source-map --config ./build/extension.webpack.config.js",
"watch2": "tsc -watch -p ./",
- "package": "vsce package",
+ "webpack": "webpack --mode production --config
./build/extension.webpack.config.js",
+ "package": "vsce package",
Review Comment:
Should we remove package? Doesn't yarn build and build_package() do that
now? Running vsce package from the root directory will not do the right thing
anymore? Potentially the same issue with "publish" (isn't that a manual process
any ways?)
##########
build/scripts/process_build.ts:
##########
@@ -18,39 +18,98 @@
// @ts-nocheck <-- This is needed as this file is basically a JavaScript script
// but with some TypeScript niceness baked in
const fs = require('fs')
+const path = require('path')
+const glob = require('glob')
const execSync = require('child_process').execSync
+const pkg_dir = 'dist/package'
+var fileCount = 0
-function prebuild() {
- fs.renameSync('LICENSE', 'tmp.LICENSE')
- fs.renameSync('NOTICE', 'tmp.NOTICE')
- fs.copyFileSync('build/bin.NOTICE', 'NOTICE')
- fs.copyFileSync('build/bin.LICENSE', 'LICENSE')
+function copyGlob(pattern, skipDirectory = false) {
+ glob(pattern, (error, files) => {
+ for (var i = 0; i < files.length; i++) {
+ let file = !skipDirectory
+ ? files[i]
+ : files[i].split('/')[files[i].split('/').length - 1]
+
+ let directory = path.join(pkg_dir, getDir(file))
+
+ if (!fs.existsSync(directory)) {
+ fs.mkdirSync(directory, { recursive: true })
+ }
+
+ fileCopy(file, `${pkg_dir}/${file}`)
+ fileCount++
+ }
+ })
+}
+
+function getDir(file) {
+ return file
+ .split('/')
+ .slice(0, file.split('/').length - 1)
+ .join('/')
+}
+
+function fileCopy(srcPath, destPath) {
+ if (fs.statSync(srcPath).isFile()) {
+ fs.copyFileSync(srcPath, destPath)
+ }
+}
+
+function create_package() {
+ // wait to run these commands till all files have been copied
+ while (fileCount < fs.readdirSync(pkg_dir).length) {
+ execSync('yarn install', { cwd: pkg_dir })
+ execSync('yarn package --out ../../', { cwd: pkg_dir })
+ }
}
-function postbuild() {
- fs.rmSync('LICENSE')
- fs.rmSync('NOTICE')
- fs.renameSync('tmp.LICENSE', 'LICENSE')
- fs.renameSync('tmp.NOTICE', 'NOTICE')
-
- // This will make sure that if the root LICENSE and NOTICE are the same as
the build LICENSE
- // and NOTICE that they are reverted back to their original contents.
- if (
- fs.readFileSync('build/bin.LICENSE').toString() ===
- fs.readFileSync('LICENSE').toString()
- ) {
- execSync('git checkout LICENSE')
+function build_package() {
+ if (fs.existsSync(pkg_dir)) {
+ fs.rmdirSync(pkg_dir, { recursive: true })
}
- if (
- fs.readFileSync('build/bin.NOTICE').toString() ===
- fs.readFileSync('NOTICE').toString()
- ) {
- execSync('git checkout NOTICE')
+ fs.mkdirSync(pkg_dir)
+
+ let lines = fs
+ .readFileSync('build/package/.vscodeignore')
+ .toString()
+ .split('\n')
+
+ // Copy all files listed in the .vscodeignore
+ for (var i = 0; i < lines.length; i++) {
+ var line = lines[i]
+
+ if (!line.startsWith('!')) continue
+
+ let pattern = line.substring(1).trim()
+
+ if (pattern.includes('*')) {
+ copyGlob(pattern)
+ } else {
+ let file = pattern
+ let directory = getDir(file)
+
+ fs.mkdirSync(`${pkg_dir}/${directory}`, { recursive: true })
+
+ fileCopy(file, `${pkg_dir}/${file}`)
+ fileCount++
+ }
}
+
+ // Copy ignore file
+ fileCopy('build/package/.vscodeignore', `${pkg_dir}/.vscodeignore`)
+
+ // Copy required package files into package directory
+ copyGlob('build/package/{.,}*', true)
+
+ // Copy node_modules into package directory
+ copyGlob('node_modules/{.,}**/{.,}*')
Review Comment:
Is it fine to copy node_modules without also copying yarn.lock? Also, do we
even need yarn install if we're copying node_modules? yarn is magic so I'm not
sure what's actually needed.
##########
build/scripts/process_build.ts:
##########
@@ -18,39 +18,83 @@
// @ts-nocheck <-- This is needed as this file is basically a JavaScript script
// but with some TypeScript niceness baked in
const fs = require('fs')
-const execSync = require('child_process').execSync
+const path = require('path')
+const glob = require('glob')
+const pkg_dir = 'dist/package'
-function prebuild() {
- fs.renameSync('LICENSE', 'tmp.LICENSE')
- fs.renameSync('NOTICE', 'tmp.NOTICE')
- fs.copyFileSync('build/bin.NOTICE', 'NOTICE')
- fs.copyFileSync('build/bin.LICENSE', 'LICENSE')
+function copyGlob(pattern, skipDirectory = false) {
+ glob(pattern, (error, files) => {
+ for (var i = 0; i < files.length; i++) {
+ let file = !skipDirectory
+ ? files[i]
+ : files[i].split('/')[files[i].split('/').length - 1]
+
+ let directory = path.join(pkg_dir, getDir(file))
+
+ if (!fs.existsSync(directory)) {
+ fs.mkdirSync(directory, { recursive: true })
+ }
+
+ fileCopy(file, `${pkg_dir}/${file}`)
+ }
+ })
+}
+
+function getDir(file) {
+ return file
+ .split('/')
+ .slice(0, file.split('/').length - 1)
+ .join('/')
}
-function postbuild() {
- fs.rmSync('LICENSE')
- fs.rmSync('NOTICE')
- fs.renameSync('tmp.LICENSE', 'LICENSE')
- fs.renameSync('tmp.NOTICE', 'NOTICE')
-
- // This will make sure that if the root LICENSE and NOTICE are the same as
the build LICENSE
- // and NOTICE that they are reverted back to their original contents.
- if (
- fs.readFileSync('build/bin.LICENSE').toString() ===
- fs.readFileSync('LICENSE').toString()
- ) {
- execSync('git checkout LICENSE')
+function fileCopy(srcPath, destPath) {
+ if (fs.statSync(srcPath).isFile()) {
+ fs.copyFileSync(srcPath, destPath)
}
+}
+
+function setup_package() {
+ if (fs.existsSync(pkg_dir)) {
+ fs.rmdirSync(pkg_dir, { recursive: true })
+ }
+
+ fs.mkdirSync(pkg_dir)
- if (
- fs.readFileSync('build/bin.NOTICE').toString() ===
- fs.readFileSync('NOTICE').toString()
- ) {
- execSync('git checkout NOTICE')
+ let lines = fs
+ .readFileSync('build/package/.vscodeignore')
+ .toString()
+ .split('\n')
+
+ // Copy all files listed in the .vscodeignore
+ for (var i = 0; i < lines.length; i++) {
+ var line = lines[i]
+
+ if (!line.startsWith('!')) continue
+
+ let pattern = line.substring(1)
+
+ if (pattern.includes('*')) {
Review Comment:
Although we don't use them, there are other characters that are special glob
characters, e.g. `?`, `|`, `('` `)`. We could check for them to, but `copyGlob`
should still work even with patterns that don't contain glob characters. I'd
suggest this just always calls `copyGlob` and drop the else block.
--
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]
