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/06 17:17:08 UTC
[GitHub] [daffodil-vscode] Shanedell opened a new pull request, #206: Build Script & General updates
Shanedell opened a new pull request, #206:
URL: https://github.com/apache/daffodil-vscode/pull/206
Multiple Updates:
- Create node script to handle pre and post build processes instead of using OS commands
- Move location of script to download omega_edit. Also made it a static script instead of one that is compiled.
- Remove run-script-os
- Create artifact class
Closes #121
--
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
[GitHub] [daffodil-vscode] Shanedell commented on a diff in pull request #206: Build Script & General updates
Posted by GitBox <gi...@apache.org>.
Shanedell commented on code in PR #206:
URL: https://github.com/apache/daffodil-vscode/pull/206#discussion_r915245623
##########
build/scripts/omega_edit_download.ts:
##########
@@ -15,40 +15,38 @@
* limitations under the License.
*/
-import * as fs from 'fs'
-import * as path from 'path'
-import * as os from 'os'
-import { HttpClient } from 'typed-rest-client/HttpClient'
-import * as crypto from 'crypto'
+// @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 os = require('os')
+const path = require('path')
+const HttpClient = require('typed-rest-client/HttpClient').HttpClient
+const crypto = require('crypto')
const hashSum = crypto.createHash('sha512')
-// Method to get omega-edit version from a JSON file
-export function getOmegaEditPackageVersion(filePath: fs.PathLike) {
- return JSON.parse(fs.readFileSync(filePath).toString())['dependencies'][
- 'omega-edit'
- ]
-}
-
-// Method to get omegaEditServerHash from a JSON file
-export function getOmegaEditServerHash(filePath: fs.PathLike) {
- return JSON.parse(fs.readFileSync(filePath).toString())['omegaEditServerHash']
-}
+/**
+ * The below classes (Backend, Artifact) are defined similarly in src/classes/artifact.ts.
+ * However for this script to work properly they needed to be redefined in a more JS style.
+ */
-export class Backend {
- constructor(readonly owner: string, readonly repo: string) {}
+class Backend {
+ constructor(owner, repo) {
+ this.owner = owner
+ this.repo = repo
+ }
}
-export class Artifact {
- constructor(readonly omegaEditVersion: string) {}
-
- name = `omega-edit-scala-server-${this.omegaEditVersion}`
- archive = `${this.name}.zip`
- archiveUrl = (backend: Backend) =>
- `https://github.com/${backend.owner}/${backend.repo}/releases/download/v${this.omegaEditVersion}/${this.archive}`
-
- scriptName = os.platform().toLowerCase().startsWith('win32')
- ? 'example-grpc-server.bat'
- : './example-grpc-server'
+class Artifact {
+ constructor(omegaEditVersion) {
+ this.omegaEditVersion = omegaEditVersion
+ this.name = `omega-edit-scala-server-${this.omegaEditVersion}`
+ this.archive = `${this.name}.zip`
+ this.archiveUrl = (backend) =>
+ `https://github.com/${backend.owner}/${backend.repo}/releases/download/v${this.omegaEditVersion}/${this.archive}`
Review Comment:
So we can't do that because the `omega-edit` scala server is only packaged using `universal:packageBin`, like we do with the bugger, so creates a zip with the needed scripts and JAR files. Moreover, this ZIP not published to any maven/sbt repos it is only published to the GitHub release page. So this file is not building anything necessarily it is only downloading the `omega-edit` scala server from the github releases.
--
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
[GitHub] [daffodil-vscode] scholarsmate commented on pull request #206: Build Script & General updates
Posted by GitBox <gi...@apache.org>.
scholarsmate commented on PR #206:
URL: https://github.com/apache/daffodil-vscode/pull/206#issuecomment-1181163468
Is this just waiting on another +1?
--
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
[GitHub] [daffodil-vscode] Shanedell commented on a diff in pull request #206: Build Script & General updates
Posted by GitBox <gi...@apache.org>.
Shanedell commented on code in PR #206:
URL: https://github.com/apache/daffodil-vscode/pull/206#discussion_r915253661
##########
yarn.lock:
##########
@@ -36,9 +36,9 @@
"@jridgewell/trace-mapping" "^0.3.9"
"@jridgewell/resolve-uri@^3.0.3":
- version "3.0.8"
- resolved "https://registry.yarnpkg.com/@jridgewell/resolve-uri/-/resolve-uri-3.0.8.tgz#687cc2bbf243f4e9a868ecf2262318e2658873a1"
- integrity sha512-YK5G9LaddzGbcucK4c8h5tWFmMPBvRZ/uyWmN1/SbBdIvqGUdWGkJ5BAaccgs6XbzVLsqbPJrBSFwKv3kT9i7w==
+ version "3.1.0"
Review Comment:
Yes it should be checked in. The `yarn.lock` ensures that no matter who pulls this code when they run `yarn install` everybody would get the same version of the packages. In a handful of places in the `package.json` we need to use `^VERSION` meaning anything above and including that specified version can be used.
Without the `yarn.lock` the items that don't have a strict version can change depending on when the `yarn install` command is ran.
- Example:
```
On 5/6 John Smith cloned the repo, ran `yarn install` and got version `7.1.3` of `@types/glob`.
However, later that day (5/6) version `7.1.4` was released to npmjs.
On 5/7 Jane Doe cloned the repo, ran `yarn install` and got version `7.1.4` of `@types/glob`.
For John Smith everything works fine but for Jane Doe something broke in `daffodil-vscode` from
the changes made in `@types/glob` between `7.1.3` and `7.1.4`.
```
--
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
[GitHub] [daffodil-vscode] Shanedell commented on a diff in pull request #206: Build Script & General updates
Posted by GitBox <gi...@apache.org>.
Shanedell commented on code in PR #206:
URL: https://github.com/apache/daffodil-vscode/pull/206#discussion_r915253661
##########
yarn.lock:
##########
@@ -36,9 +36,9 @@
"@jridgewell/trace-mapping" "^0.3.9"
"@jridgewell/resolve-uri@^3.0.3":
- version "3.0.8"
- resolved "https://registry.yarnpkg.com/@jridgewell/resolve-uri/-/resolve-uri-3.0.8.tgz#687cc2bbf243f4e9a868ecf2262318e2658873a1"
- integrity sha512-YK5G9LaddzGbcucK4c8h5tWFmMPBvRZ/uyWmN1/SbBdIvqGUdWGkJ5BAaccgs6XbzVLsqbPJrBSFwKv3kT9i7w==
+ version "3.1.0"
Review Comment:
Yes it should be checked in. The `yarn.lock` ensures that no matter who pulls this code when they run `yarn install` everybody would get the same version of the packages. In a handful of places in the `package.json` we need to use `^VERSION` meaning anything above and including that specified version can be used.
Without the `yarn.lock` the items that don't have a strict version can change depending on when the `yarn install` command is ran.
- Example:
```
On 5/6 John Smith cloned the repo, ran `yarn install` and got version `7.1.3` of `@types/glob`.
However, later that day (5/6) version `7.1.4` was released to npmjs.
On 5/7 Jane Doe cloned the repo, ran `yarn install` and got version `7.1.4` of `@types/glob`.
For John Smith everything works fine but for Jane Doe something broke in `daffodil-vscode` from the changes made in `@types/glob` between `7.1.3` and `7.1.4`.
```
--
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
[GitHub] [daffodil-vscode] Shanedell merged pull request #206: Build Script & General updates
Posted by GitBox <gi...@apache.org>.
Shanedell merged PR #206:
URL: https://github.com/apache/daffodil-vscode/pull/206
--
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
[GitHub] [daffodil-vscode] Shanedell commented on a diff in pull request #206: Build Script & General updates
Posted by GitBox <gi...@apache.org>.
Shanedell commented on code in PR #206:
URL: https://github.com/apache/daffodil-vscode/pull/206#discussion_r915253661
##########
yarn.lock:
##########
@@ -36,9 +36,9 @@
"@jridgewell/trace-mapping" "^0.3.9"
"@jridgewell/resolve-uri@^3.0.3":
- version "3.0.8"
- resolved "https://registry.yarnpkg.com/@jridgewell/resolve-uri/-/resolve-uri-3.0.8.tgz#687cc2bbf243f4e9a868ecf2262318e2658873a1"
- integrity sha512-YK5G9LaddzGbcucK4c8h5tWFmMPBvRZ/uyWmN1/SbBdIvqGUdWGkJ5BAaccgs6XbzVLsqbPJrBSFwKv3kT9i7w==
+ version "3.1.0"
Review Comment:
Yes it should be checked in. The `yarn.lock` ensures that no matter who pulls this code when they run `yarn install` everybody would get the same version of the packages. In a handful of places in the `package.json` we need to use `^VERSION` meaning anything above and including that specified version can be used.
Without the `yarn.lock` the items that don't have a strict version can change depending on when the `yarn install` command is ran. Eg on 5/6 John Smith cloned the repo, ran `yarn install` and got version `7.1.3` of `@types/glob`. However, later that day version `7.1.4` was released to npmjs. On 5/7 Jane Doe cloned the repo, ran `yarn install` and got version `7.1.4` of `@types/glob` but for this individual something broke in `daffodil-vscode` from the changes made in `@types/glob` between `7.1.3` and `7.1.4`.
--
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
[GitHub] [daffodil-vscode] Shanedell commented on a diff in pull request #206: Build Script & General updates
Posted by GitBox <gi...@apache.org>.
Shanedell commented on code in PR #206:
URL: https://github.com/apache/daffodil-vscode/pull/206#discussion_r915242738
##########
build/scripts/process_build.ts:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.
+ */
+
+// @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
+
+function prebuild() {
Review Comment:
So what this script is doing is replacing the OS specific commands to move `bin.LICENSE` and `bin.NOTICE` to be `LICENSE` and `NOTICE` before packaging the VSIX, then reverting them back once the VSIX is done.
So the sync variants just ensure that nothing else is going to happen before this item completes its tasks.
--
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
[GitHub] [daffodil-vscode] mbeckerle commented on a diff in pull request #206: Build Script & General updates
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on code in PR #206:
URL: https://github.com/apache/daffodil-vscode/pull/206#discussion_r915194591
##########
build/scripts/omega_edit_download.ts:
##########
@@ -15,40 +15,38 @@
* limitations under the License.
*/
-import * as fs from 'fs'
-import * as path from 'path'
-import * as os from 'os'
-import { HttpClient } from 'typed-rest-client/HttpClient'
-import * as crypto from 'crypto'
+// @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 os = require('os')
+const path = require('path')
+const HttpClient = require('typed-rest-client/HttpClient').HttpClient
+const crypto = require('crypto')
const hashSum = crypto.createHash('sha512')
-// Method to get omega-edit version from a JSON file
-export function getOmegaEditPackageVersion(filePath: fs.PathLike) {
- return JSON.parse(fs.readFileSync(filePath).toString())['dependencies'][
- 'omega-edit'
- ]
-}
-
-// Method to get omegaEditServerHash from a JSON file
-export function getOmegaEditServerHash(filePath: fs.PathLike) {
- return JSON.parse(fs.readFileSync(filePath).toString())['omegaEditServerHash']
-}
+/**
+ * The below classes (Backend, Artifact) are defined similarly in src/classes/artifact.ts.
+ * However for this script to work properly they needed to be redefined in a more JS style.
+ */
-export class Backend {
- constructor(readonly owner: string, readonly repo: string) {}
+class Backend {
+ constructor(owner, repo) {
+ this.owner = owner
+ this.repo = repo
+ }
}
-export class Artifact {
- constructor(readonly omegaEditVersion: string) {}
-
- name = `omega-edit-scala-server-${this.omegaEditVersion}`
- archive = `${this.name}.zip`
- archiveUrl = (backend: Backend) =>
- `https://github.com/${backend.owner}/${backend.repo}/releases/download/v${this.omegaEditVersion}/${this.archive}`
-
- scriptName = os.platform().toLowerCase().startsWith('win32')
- ? 'example-grpc-server.bat'
- : './example-grpc-server'
+class Artifact {
+ constructor(omegaEditVersion) {
+ this.omegaEditVersion = omegaEditVersion
+ this.name = `omega-edit-scala-server-${this.omegaEditVersion}`
+ this.archive = `${this.name}.zip`
+ this.archiveUrl = (backend) =>
+ `https://github.com/${backend.owner}/${backend.repo}/releases/download/v${this.omegaEditVersion}/${this.archive}`
Review Comment:
Why do we have to build this stuff?
Why doesn't maven or sbt just do this for us using managed dependencies? We just say we depend on version x.y.z of omegaEdit, which has been published to maven central etc. Or does typescript/javascript not work that way?
##########
yarn.lock:
##########
@@ -36,9 +36,9 @@
"@jridgewell/trace-mapping" "^0.3.9"
"@jridgewell/resolve-uri@^3.0.3":
- version "3.0.8"
- resolved "https://registry.yarnpkg.com/@jridgewell/resolve-uri/-/resolve-uri-3.0.8.tgz#687cc2bbf243f4e9a868ecf2262318e2658873a1"
- integrity sha512-YK5G9LaddzGbcucK4c8h5tWFmMPBvRZ/uyWmN1/SbBdIvqGUdWGkJ5BAaccgs6XbzVLsqbPJrBSFwKv3kT9i7w==
+ version "3.1.0"
Review Comment:
Should yarn.lock be checked in at all? Should we be ignoring it in .gitignore?
##########
build/scripts/process_build.ts:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.
+ */
+
+// @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
+
+function prebuild() {
Review Comment:
What is this doing? Add comment that this "Combines license and notice information into the bin.License or whatever so that we don't have to maintain the bin.NOTICE manually. Assuming I've guessed correctly why we need this.
Why are all these calls "Sync" variants? What's happening that requires that?
--
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
[GitHub] [daffodil-vscode] mbeckerle commented on a diff in pull request #206: Build Script & General updates
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on code in PR #206:
URL: https://github.com/apache/daffodil-vscode/pull/206#discussion_r915315596
##########
build/scripts/omega_edit_download.ts:
##########
@@ -15,40 +15,38 @@
* limitations under the License.
*/
-import * as fs from 'fs'
-import * as path from 'path'
-import * as os from 'os'
-import { HttpClient } from 'typed-rest-client/HttpClient'
-import * as crypto from 'crypto'
+// @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 os = require('os')
+const path = require('path')
+const HttpClient = require('typed-rest-client/HttpClient').HttpClient
+const crypto = require('crypto')
const hashSum = crypto.createHash('sha512')
-// Method to get omega-edit version from a JSON file
-export function getOmegaEditPackageVersion(filePath: fs.PathLike) {
- return JSON.parse(fs.readFileSync(filePath).toString())['dependencies'][
- 'omega-edit'
- ]
-}
-
-// Method to get omegaEditServerHash from a JSON file
-export function getOmegaEditServerHash(filePath: fs.PathLike) {
- return JSON.parse(fs.readFileSync(filePath).toString())['omegaEditServerHash']
-}
+/**
+ * The below classes (Backend, Artifact) are defined similarly in src/classes/artifact.ts.
+ * However for this script to work properly they needed to be redefined in a more JS style.
+ */
-export class Backend {
- constructor(readonly owner: string, readonly repo: string) {}
+class Backend {
+ constructor(owner, repo) {
+ this.owner = owner
+ this.repo = repo
+ }
}
-export class Artifact {
- constructor(readonly omegaEditVersion: string) {}
-
- name = `omega-edit-scala-server-${this.omegaEditVersion}`
- archive = `${this.name}.zip`
- archiveUrl = (backend: Backend) =>
- `https://github.com/${backend.owner}/${backend.repo}/releases/download/v${this.omegaEditVersion}/${this.archive}`
-
- scriptName = os.platform().toLowerCase().startsWith('win32')
- ? 'example-grpc-server.bat'
- : './example-grpc-server'
+class Artifact {
+ constructor(omegaEditVersion) {
+ this.omegaEditVersion = omegaEditVersion
+ this.name = `omega-edit-scala-server-${this.omegaEditVersion}`
+ this.archive = `${this.name}.zip`
+ this.archiveUrl = (backend) =>
+ `https://github.com/${backend.owner}/${backend.repo}/releases/download/v${this.omegaEditVersion}/${this.archive}`
Review Comment:
I took a look and I didn't find exactly the thing I wanted which is sbt dependencies that look for github releases. You can do source code dependencies, but not binary dependencies.
--
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
[GitHub] [daffodil-vscode] mbeckerle commented on pull request #206: Build Script & General updates
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on PR #206:
URL: https://github.com/apache/daffodil-vscode/pull/206#issuecomment-1181830102
Yes, I think just waiting for a 2nd reviewer.
--
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