You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2022/07/18 21:33:29 UTC
[GitHub] [daffodil-vscode] stevedlawrence commented on a diff in pull request #232: Update build script.
stevedlawrence commented on code in PR #232:
URL: https://github.com/apache/daffodil-vscode/pull/232#discussion_r923873964
##########
build/scripts/process_build.ts:
##########
@@ -18,39 +18,88 @@
// @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 execSync = require('child_process').execSync
-function prebuild() {
- fs.renameSync('LICENSE', 'tmp.LICENSE')
- fs.renameSync('NOTICE', 'tmp.NOTICE')
- fs.copyFileSync('build/bin.NOTICE', 'NOTICE')
- fs.copyFileSync('build/bin.LICENSE', 'LICENSE')
+const pkg_dir = 'dist/package'
+
+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 dirCopy(file) {
+ fs.readdirSync(file).forEach((dirFile) => {
+ const filePath = path.join(file, dirFile)
+ fileCopy(filePath, path.join(pkg_dir, file, dirFile))
+ })
+}
+
+function build_package() {
+ if (fs.existsSync(pkg_dir)) {
+ fs.rmdirSync(pkg_dir, { recursive: true })
+ }
+
+ fs.mkdirSync(pkg_dir)
+
+ let fileList = fs.readFileSync('.vscodeignore').toString().split('\n')
Review Comment:
This is minor, but what are your thoughts on moving the .vscodeignore file to another directory since it is really only used during packaging? I'm thinking we could put .vscodeignore, bin.LICENSE, and bin.NOTICE files in something like `build/package/`. After we've copied all the files from .vscodeignore into `dist/package`, we could then just copy `build/package/*` into `dist/package/`. That way adding new static files to `dist/package` is as easy as putting them in `build/package`.
##########
build/scripts/process_build.ts:
##########
@@ -18,39 +18,81 @@
// @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 execSync = require('child_process').execSync
-function prebuild() {
- fs.renameSync('LICENSE', 'tmp.LICENSE')
- fs.renameSync('NOTICE', 'tmp.NOTICE')
- fs.copyFileSync('build/bin.NOTICE', 'NOTICE')
- fs.copyFileSync('build/bin.LICENSE', 'LICENSE')
+const pkg_dir = 'dist/package'
+
+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 dirCopy(file) {
+ fs.readdirSync(file).forEach((dirFile) => {
+ const filePath = path.join(file, dirFile)
+ fileCopy(filePath, path.join(pkg_dir, file, dirFile))
+ })
+}
+
+function build_package() {
+ if (fs.existsSync(pkg_dir)) {
+ fs.rmdirSync(pkg_dir, { recursive: true })
+ }
+
+ fs.mkdirSync(pkg_dir)
+
+ let fileList = fs.readFileSync('.vscodeignore').toString().split('\n')
+
+ for (var i = 0; i < fileList.length; i++) {
+ var file = fileList[i].replace(/\*/gi, '').replace('!', '')
- if (
- fs.readFileSync('build/bin.NOTICE').toString() ===
- fs.readFileSync('NOTICE').toString()
- ) {
- execSync('git checkout NOTICE')
+ if (file.includes('#') || ['', '/', '\\', '\\r', '\\n'].includes(file)) {
+ continue
+ }
+
+ if (file.includes('zip')) {
+ var dir = getDir(file)
+
+ fs.readdirSync(dir).forEach((dirFile) => {
+ if (dirFile.includes('zip')) {
+ file = `${dir}/${dirFile}`
+ }
+ })
+ }
+
+ let directory = fs.statSync(file).isFile() ? getDir(file) : file
+
+ fs.mkdirSync(`${pkg_dir}/${directory}`, { recursive: true })
+
+ fs.statSync(file).isFile()
+ ? fileCopy(file, `${pkg_dir}/${file}`)
+ : dirCopy(file)
}
+
+ fileCopy('.vscodeignore', `${pkg_dir}/.vscodeignore`)
Review Comment:
Do we also need to copy bin.NOTICE and bin.LICENSE files?
##########
build/scripts/process_build.ts:
##########
@@ -18,39 +18,88 @@
// @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 execSync = require('child_process').execSync
-function prebuild() {
- fs.renameSync('LICENSE', 'tmp.LICENSE')
- fs.renameSync('NOTICE', 'tmp.NOTICE')
- fs.copyFileSync('build/bin.NOTICE', 'NOTICE')
- fs.copyFileSync('build/bin.LICENSE', 'LICENSE')
+const pkg_dir = 'dist/package'
+
+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 dirCopy(file) {
+ fs.readdirSync(file).forEach((dirFile) => {
+ const filePath = path.join(file, dirFile)
+ fileCopy(filePath, path.join(pkg_dir, file, dirFile))
+ })
+}
+
+function build_package() {
+ if (fs.existsSync(pkg_dir)) {
+ fs.rmdirSync(pkg_dir, { recursive: true })
+ }
+
+ fs.mkdirSync(pkg_dir)
+
+ let fileList = fs.readFileSync('.vscodeignore').toString().split('\n')
+
+ for (var i = 0; i < fileList.length; i++) {
+ var file = fileList[i].replace(/\*/gi, '').replace('!', '')
- if (
- fs.readFileSync('build/bin.NOTICE').toString() ===
- fs.readFileSync('NOTICE').toString()
- ) {
- execSync('git checkout NOTICE')
+ if (
+ file.includes('#') ||
+ file == '' ||
+ file == '/' ||
+ file == '\\' ||
+ file == '\\r' ||
+ file === '\\n'
+ ) {
+ continue
+ }
+
+ if (file.includes('zip')) {
+ var dir = getDir(file)
+
+ fs.readdirSync(dir).forEach((dirFile) => {
+ if (dirFile.includes('zip')) {
+ file = `${dir}/${dirFile}`
+ }
+ })
+ }
+
+ let directory = fs.statSync(file).isFile() ? getDir(file) : file
+
+ fs.mkdirSync(`${pkg_dir}/${directory}`, { recursive: true })
+
+ fs.statSync(file).isFile()
+ ? fileCopy(file, `${pkg_dir}/${file}`)
+ : dirCopy(file)
}
+
+ fileCopy('.vscodeignore', `${pkg_dir}/.vscodeignore`)
+ execSync('yarn install', { cwd: pkg_dir })
Review Comment:
Is it possible to remove or replace yarn install some how? I think this is needed to create the `node_modules` stuff needed to run vsce? It would be really nice if yarn package/vsce package could just reuse the existing `node_modules` in the root directory somehow?
If it's not possible, do we need to also copy yarn.lock into `dist/package`? It looks like this creates a new one that isn't exactly the same as the existing one if we don't copy?
##########
build/scripts/process_build.ts:
##########
@@ -18,39 +18,81 @@
// @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 execSync = require('child_process').execSync
-function prebuild() {
- fs.renameSync('LICENSE', 'tmp.LICENSE')
- fs.renameSync('NOTICE', 'tmp.NOTICE')
- fs.copyFileSync('build/bin.NOTICE', 'NOTICE')
- fs.copyFileSync('build/bin.LICENSE', 'LICENSE')
+const pkg_dir = 'dist/package'
+
+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 dirCopy(file) {
+ fs.readdirSync(file).forEach((dirFile) => {
+ const filePath = path.join(file, dirFile)
+ fileCopy(filePath, path.join(pkg_dir, file, dirFile))
+ })
+}
+
+function build_package() {
+ if (fs.existsSync(pkg_dir)) {
+ fs.rmdirSync(pkg_dir, { recursive: true })
+ }
+
+ fs.mkdirSync(pkg_dir)
+
+ let fileList = fs.readFileSync('.vscodeignore').toString().split('\n')
+
+ for (var i = 0; i < fileList.length; i++) {
+ var file = fileList[i].replace(/\*/gi, '').replace('!', '')
- if (
- fs.readFileSync('build/bin.NOTICE').toString() ===
- fs.readFileSync('NOTICE').toString()
- ) {
- execSync('git checkout NOTICE')
+ if (file.includes('#') || ['', '/', '\\', '\\r', '\\n'].includes(file)) {
+ continue
+ }
+
+ if (file.includes('zip')) {
+ var dir = getDir(file)
+
+ fs.readdirSync(dir).forEach((dirFile) => {
+ if (dirFile.includes('zip')) {
+ file = `${dir}/${dirFile}`
+ }
+ })
+ }
Review Comment:
This code seems a bit fragile. For example, it removes `*`'s which are useful since some of the lines in .vscodeignore are globs. Also, the way it handles zip is fragile since it just copies all zips from a directory? I think because we use globs for zip files in the .vscodeignore file?
What are you thoughts on this approach, where the key differences is 1) we only look at lines that start with an exclamation, and 2) those lines are treated as globs using the glob package:
```node
let lines = fs.readFileSync('.vscodeignore').toString().split('\n')
for (var i = 0; i < lines.length; i++) {
let line = lines[i]
// ignore any lines that don't start with an exclamation
if (!line.startsWith("!")) continue
// remove the exclamation
let pattern = line.substring(1)
// find all files matching the line glob and copy into dist/package/
glob(pattern, (error, files) => {
for (var j = 0; j < files.length; j++) {
let file = files[j]
// code here from below that creates directories and copies file/dir
}
})
```
This isn't tested, and I'm sure there's some node issues, but something along those lines?
##########
build/scripts/process_build.ts:
##########
@@ -18,39 +18,88 @@
// @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 execSync = require('child_process').execSync
-function prebuild() {
- fs.renameSync('LICENSE', 'tmp.LICENSE')
- fs.renameSync('NOTICE', 'tmp.NOTICE')
- fs.copyFileSync('build/bin.NOTICE', 'NOTICE')
- fs.copyFileSync('build/bin.LICENSE', 'LICENSE')
+const pkg_dir = 'dist/package'
+
+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 dirCopy(file) {
+ fs.readdirSync(file).forEach((dirFile) => {
+ const filePath = path.join(file, dirFile)
+ fileCopy(filePath, path.join(pkg_dir, file, dirFile))
+ })
+}
+
+function build_package() {
+ if (fs.existsSync(pkg_dir)) {
+ fs.rmdirSync(pkg_dir, { recursive: true })
+ }
+
+ fs.mkdirSync(pkg_dir)
+
+ let fileList = fs.readFileSync('.vscodeignore').toString().split('\n')
+
+ for (var i = 0; i < fileList.length; i++) {
+ var file = fileList[i].replace(/\*/gi, '').replace('!', '')
- if (
- fs.readFileSync('build/bin.NOTICE').toString() ===
- fs.readFileSync('NOTICE').toString()
- ) {
- execSync('git checkout NOTICE')
+ if (
+ file.includes('#') ||
+ file == '' ||
+ file == '/' ||
+ file == '\\' ||
+ file == '\\r' ||
+ file === '\\n'
+ ) {
+ continue
+ }
+
+ if (file.includes('zip')) {
+ var dir = getDir(file)
+
+ fs.readdirSync(dir).forEach((dirFile) => {
+ if (dirFile.includes('zip')) {
+ file = `${dir}/${dirFile}`
+ }
+ })
+ }
+
+ let directory = fs.statSync(file).isFile() ? getDir(file) : file
+
+ fs.mkdirSync(`${pkg_dir}/${directory}`, { recursive: true })
+
+ fs.statSync(file).isFile()
+ ? fileCopy(file, `${pkg_dir}/${file}`)
+ : dirCopy(file)
}
+
+ fileCopy('.vscodeignore', `${pkg_dir}/.vscodeignore`)
+ execSync('yarn install', { cwd: pkg_dir })
+ execSync('yarn package', { cwd: pkg_dir })
Review Comment:
yarn package just calls vsce, thoughts on just directly calling vsce ourselves here and removing the "yarn package script? Is there ever a case where we would want to run "yarn package" without all thise new copy stuff?
vsce package also has a `--out` parameter which could be used instead of having to call get_vsix? Then `yarn build` just becomes this:
```
"build": "node -e \"require('./build/scripts/process_build.ts').build_package()\"",
```
and it simplifies the package.json a bit?
--
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