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/12/05 22:42:58 UTC
[GitHub] [daffodil-vscode] Shanedell opened a new pull request, #371: omega-edit updates
Shanedell opened a new pull request, #371:
URL: https://github.com/apache/daffodil-vscode/pull/371
omega-edit updates:
- Moved omega-edit server related code to the omega-edit node module.
- Moved omega-edit scala server zip to omega-edit node package.
- Delete build/scripts/omega_edit_download since the scala server is now bundled in the node package.
- Remove omegaEditServerHash from package.json.
- Display information messages when starting and stopping the omega-edit server.
- This is to give the developer a notice that it is starting/started or stopped.
- This replace the terminal being opened displaying the omega-edit server binding.
Closes #334
NOTE: Keeping a draft till 1.2.0 is fully released.
--
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 #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
Shanedell commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1049026004
##########
src/utils.ts:
##########
@@ -173,6 +173,19 @@ export async function executeScript(
return terminal
}
+export async function stopJavaProcess(processName: string) {
+ // Stop debugger if running
+ if (os.platform() === 'win32') {
+ // Windows stop debugger if already running
+ child_process.execSync('tskill java 2>nul 1>nul || echo "Java not running"')
+ } else {
+ // Linux/Mac stop debugger if already running and make sure script is executable
+ child_process.exec(
+ `kill -9 $(ps -ef | grep '${processName}' | grep 'jar' | awk '{ print $2 }') || return 0`
+ ) // ensure debugger server not running and
+ }
+}
Review Comment:
we want to limit the amount of deps we add. This is done already whenever we start up our own instances however the above code was for ensure a daffodil-debugger was not running without knowing the pid. This was only ran in one place so removing it shouldn't have much of an effect
--
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 #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1045962517
##########
src/utils.ts:
##########
@@ -173,6 +173,19 @@ export async function executeScript(
return terminal
}
+export async function stopJavaProcess() {
Review Comment:
Is there no decent os-lib type library for typescript/javascript?
--
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 #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
Shanedell commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1049052811
##########
src/tests/suite/omegaEdit.test.ts:
##########
@@ -23,30 +23,30 @@ import { Artifact, Backend } from '../../classes/artifact'
import * as omegaEditClient from '../../omega_edit/client'
import { unzipFile, runScript, killProcess } from '../../utils'
import { before, after } from 'mocha'
+import * as fs from 'fs'
+const PROJECT_ROOT = path.join(__dirname, '../../../')
Review Comment:
@scholarsmate Was able to remove most of these. However, it needed to be kept in the `src/tests/runTest.ts`
--
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 a diff in pull request #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
scholarsmate commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1049067549
##########
src/tests/suite/adapter.test.ts:
##########
@@ -20,8 +20,11 @@ import * as path from 'path'
import { DebugClient } from 'vscode-debugadapter-testsupport'
suite('Daffodil Debug Adapter', () => {
- const DEBUG_ADAPTER = path.resolve('out/adapter/debugAdapter.js')
- const PROJECT_ROOT = path.resolve('.')
+ const DEBUG_ADAPTER = path.join(
+ __dirname,
+ '../../../out/adapter/debugAdapter.js'
+ )
+ const PROJECT_ROOT = path.join(__dirname, '../../../')
Review Comment:
I think we should still try to resolve these paths instead of using these relative paths, but I won't hold up this PR over it. It's probably merits a "technical debt" ticket though.
--
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 a diff in pull request #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
scholarsmate commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1049023004
##########
src/utils.ts:
##########
@@ -173,6 +173,19 @@ export async function executeScript(
return terminal
}
+export async function stopJavaProcess(processName: string) {
+ // Stop debugger if running
+ if (os.platform() === 'win32') {
+ // Windows stop debugger if already running
+ child_process.execSync('tskill java 2>nul 1>nul || echo "Java not running"')
+ } else {
+ // Linux/Mac stop debugger if already running and make sure script is executable
+ child_process.exec(
+ `kill -9 $(ps -ef | grep '${processName}' | grep 'jar' | awk '{ print $2 }') || return 0`
+ ) // ensure debugger server not running and
+ }
+}
Review Comment:
Take a look at [spawnd](https://www.npmjs.com/package/spawnd).
--
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 #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
Shanedell commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1049056576
##########
src/tests/suite/omegaEdit.test.ts:
##########
@@ -23,30 +23,30 @@ import { Artifact, Backend } from '../../classes/artifact'
import * as omegaEditClient from '../../omega_edit/client'
import { unzipFile, runScript, killProcess } from '../../utils'
import { before, after } from 'mocha'
+import * as fs from 'fs'
+const PROJECT_ROOT = path.join(__dirname, '../../../')
Review Comment:
actually this change causes windows tests to fail so we will need to keep this how it is
--
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 #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
Shanedell commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1049068480
##########
src/tests/suite/adapter.test.ts:
##########
@@ -20,8 +20,11 @@ import * as path from 'path'
import { DebugClient } from 'vscode-debugadapter-testsupport'
suite('Daffodil Debug Adapter', () => {
- const DEBUG_ADAPTER = path.resolve('out/adapter/debugAdapter.js')
- const PROJECT_ROOT = path.resolve('.')
+ const DEBUG_ADAPTER = path.join(
+ __dirname,
+ '../../../out/adapter/debugAdapter.js'
+ )
+ const PROJECT_ROOT = path.join(__dirname, '../../../')
Review Comment:
It worked okay on mac but seemed to cause issues in windows CI. So some more testing will need to be done on the other OSes, I believe one of the ubuntu CI's ran it fine so maybe just a thing to test out further on windows?
--
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 a diff in pull request #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
scholarsmate commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1048986690
##########
src/omega_edit/client.ts:
##########
@@ -72,8 +83,14 @@ export function activate(ctx: vscode.ExtensionContext) {
subscribeToViewports: boolean = true
) => {
if (!serverRunning && startServer) {
- await getServer(ctx, omegaEditPackageVersion)
+ vscode.window.showInformationMessage('Starting omega-edit server!')
+ const [scriptName, scriptPath] = await omegaEditServer.setupServer(
+ rootPath,
+ omegaEditPackageVersion
+ )
+ serverTerminal = await runScript(scriptPath, scriptName)
serverRunning = true
+ vscode.window.showInformationMessage('omega-edit server started!')
Review Comment:
This code is duplicated in lines 62 - 69. Consider a refactor?
--
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 a diff in pull request #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
scholarsmate commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1049000434
##########
src/daffodilDebugger.ts:
##########
@@ -120,18 +120,8 @@ export async function getDebugger(
await unzipFile(filePath, rootPath)
}
- // Stop debugger if running
- if (os.platform() === 'win32') {
- // Windows stop debugger if already running
- child_process.execSync(
- 'tskill java 2>nul 1>nul || echo "Java not running"'
- )
- } else {
- // Linux/Mac stop debugger if already running and make sure script is executable
- child_process.exec(
- "kill -9 $(ps -ef | grep 'daffodil' | grep 'jar' | awk '{ print $2 }') || return 0"
- ) // ensure debugger server not running and
- }
+ // stop java program -- in this case the debugger
+ await stopJavaProcess('daffodil')
Review Comment:
Refactoring this as a utility function is a good idea. Instead of sending in the process _name_, instead send it the process ID to be more targeted and safe about the process killing.
--
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 pull request #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
Shanedell commented on PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#issuecomment-1352121537
@stevedlawrence @scholarsmate I created a new function inside of `omega-edit` for `setupServer` that will do everything except run the server. So I could technically still update the code to run the `omega-edit` server inside of the vscode terminal so the user can actually see it running as opposed to it running in the background. Do you think it would be preferred for it be seen or hidden?
--
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 #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#issuecomment-1352146680
Since omega edit is still 'experimental', running it in a terminal window seems fine for now. We may want to turn this off eventually.
--
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 a diff in pull request #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
scholarsmate commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1048995145
##########
src/utils.ts:
##########
@@ -173,6 +173,19 @@ export async function executeScript(
return terminal
}
+export async function stopJavaProcess(processName: string) {
+ // Stop debugger if running
+ if (os.platform() === 'win32') {
+ // Windows stop debugger if already running
+ child_process.execSync('tskill java 2>nul 1>nul || echo "Java not running"')
+ } else {
+ // Linux/Mac stop debugger if already running and make sure script is executable
+ child_process.exec(
+ `kill -9 $(ps -ef | grep '${processName}' | grep 'jar' | awk '{ print $2 }') || return 0`
+ ) // ensure debugger server not running and
+ }
+}
Review Comment:
I'm not a fan of this technique to kill processes. When we exec the server, we can store the process ID and send the kill signal to that PID. I could be running some other java process or even some other Ωedit process on a different port and this code will just murder those processes as collateral damage.
--
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 pull request #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
Shanedell commented on PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#issuecomment-1352147599
okay I will make that fix now
--
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 a diff in pull request #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
scholarsmate commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1049067549
##########
src/tests/suite/adapter.test.ts:
##########
@@ -20,8 +20,11 @@ import * as path from 'path'
import { DebugClient } from 'vscode-debugadapter-testsupport'
suite('Daffodil Debug Adapter', () => {
- const DEBUG_ADAPTER = path.resolve('out/adapter/debugAdapter.js')
- const PROJECT_ROOT = path.resolve('.')
+ const DEBUG_ADAPTER = path.join(
+ __dirname,
+ '../../../out/adapter/debugAdapter.js'
+ )
+ const PROJECT_ROOT = path.join(__dirname, '../../../')
Review Comment:
I think we should still try to resolve these paths instead of using these relative paths, but I won't hold up this PR over it. It probably merits a "technical debt" ticket though.
--
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 #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
Shanedell commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1049054086
##########
src/tests/suite/omegaEdit.test.ts:
##########
@@ -23,30 +23,30 @@ import { Artifact, Backend } from '../../classes/artifact'
import * as omegaEditClient from '../../omega_edit/client'
import { unzipFile, runScript, killProcess } from '../../utils'
import { before, after } from 'mocha'
+import * as fs from 'fs'
+const PROJECT_ROOT = path.join(__dirname, '../../../')
Review Comment:
In another PR I have coming there will be a split out of some of these common items between test files as well
--
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 #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
Shanedell merged PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371
--
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 pull request #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
Shanedell commented on PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#issuecomment-1352036664
@scholarsmate @stevedlawrence This should be good for a look over now. I apologize for all the commits it took me forever to figure out why the CI kept failing for windows. The issue seemed to be the path to the batch script was too long making it not ever run the 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: 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 #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
Shanedell commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1046095203
##########
src/utils.ts:
##########
@@ -173,6 +173,19 @@ export async function executeScript(
return terminal
}
+export async function stopJavaProcess() {
Review Comment:
No there is not, I am not to sure how common it is to run scripts on the OS through TS/JS like we are doing. But in another PR I have coming it up it split out a lot the OS specific stuff into separate functions to allow for better use of those items. But yeah I couldn't find any package that would be what we want, the closest thing is `run-script-os` but that is for the `package.json` scripts to make OS specific commands that way but nothing like what we would need.
--
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 a diff in pull request #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
scholarsmate commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1049011916
##########
src/utils.ts:
##########
@@ -173,6 +173,19 @@ export async function executeScript(
return terminal
}
+export async function stopJavaProcess(processName: string) {
+ // Stop debugger if running
+ if (os.platform() === 'win32') {
+ // Windows stop debugger if already running
+ child_process.execSync('tskill java 2>nul 1>nul || echo "Java not running"')
+ } else {
+ // Linux/Mac stop debugger if already running and make sure script is executable
+ child_process.exec(
+ `kill -9 $(ps -ef | grep '${processName}' | grep 'jar' | awk '{ print $2 }') || return 0`
+ ) // ensure debugger server not running and
+ }
+}
Review Comment:
If the extension is _managing_ these server processes, what it starts up, it shall also shut down.
If there is _already_ a daffodil-debugger service running _on the same port_ as configured in VS Code, then consider it _unmanaged_ and use it as-is, and don't shut it down when the debugging session is done. The same applies to the Ωedit server. This way the extension _cooperates_ with different instances of the servers if they are running and listening to the configured port, otherwise it will use our internal versions as managed processes.
--
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 #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
Shanedell commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1048997760
##########
src/utils.ts:
##########
@@ -173,6 +173,19 @@ export async function executeScript(
return terminal
}
+export async function stopJavaProcess(processName: string) {
+ // Stop debugger if running
+ if (os.platform() === 'win32') {
+ // Windows stop debugger if already running
+ child_process.execSync('tskill java 2>nul 1>nul || echo "Java not running"')
+ } else {
+ // Linux/Mac stop debugger if already running and make sure script is executable
+ child_process.exec(
+ `kill -9 $(ps -ef | grep '${processName}' | grep 'jar' | awk '{ print $2 }') || return 0`
+ ) // ensure debugger server not running and
+ }
+}
Review Comment:
Yeah this was mostly used for the daffodil-debugger to make sure another instance of it is running, but you are correct with the addition of omega-edit this could possibly trample that omega-edit instance if running. I would be also for removing this. @mbeckerle @stevedlawrence Do you think this would be good to actually remove?
--
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 a diff in pull request #371: omega-edit updates
Posted by GitBox <gi...@apache.org>.
scholarsmate commented on code in PR #371:
URL: https://github.com/apache/daffodil-vscode/pull/371#discussion_r1048990984
##########
src/tests/suite/omegaEdit.test.ts:
##########
@@ -23,30 +23,30 @@ import { Artifact, Backend } from '../../classes/artifact'
import * as omegaEditClient from '../../omega_edit/client'
import { unzipFile, runScript, killProcess } from '../../utils'
import { before, after } from 'mocha'
+import * as fs from 'fs'
+const PROJECT_ROOT = path.join(__dirname, '../../../')
Review Comment:
Instead of having this relative link, resolve it via `path.resolve()`, which should also shorten the path names.
--
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