feat: stop command support windows by BaffinLee · Pull Request #22 · eggjs/scripts

BaffinLee · 2018-07-06T13:56:03Z

Checklist

npm test passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

stop command supports windows now.

Description of change

Adjust find process command and filter logic to support windows, done simple than other pr.

Tested on Windows 7 and Windows 10, works fine.

By the way, the npm run test wouldn't pass even if I don't change anything, which I didn't dig in.

codecov · 2018-07-06T14:06:04Z

Codecov Report

Merging #22 into master will not change coverage.
The diff coverage is 100%.

@@ Coverage Diff @@## master #22 +/- ##
=====================================
Coverage 100% 100% =====================================
Files 6 6 Lines 187 190 +3 =====================================
+ Hits 187 190 +3

Impacted Files	Coverage Δ
lib/cmd/stop.js	`100% <100%> (ø)`	⬆️
lib/helper.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82f4125...908efbe. Read the comment docs.

popomore · 2018-07-06T14:23:57Z

 exports.findNodeProcess = function* (filterFn) {
- const command = 'ps -eo "pid,command"';
+ const command = isWin ?
+ 'wmic Path win32_process Where "Name = \'node.exe\'" Get CommandLine,ProcessId' :


I’m not familiar with Windows, is wmic a buildin command?

Yes, from Microsoft offical docs, but I am not sure about Windows XP.

It seems good

I had try https://www.npmjs.com/package/find-process long long ago.
but got some trouble:
XML invalid at some win10.
backslash convert confuse me at appveyor
so I give up.
@BaffinLee maybe you could help us try again ?
and you need to config package.json for egg-ci to gen appveyor.yml, so this feature could be test.
my commit: https://github.com/eggjs/egg-scripts/pull/1/commits

https://support.microsoft.com/en-us/help/2664203/-invalid-xml-content-error-message-when-you-run-a-wmic-command-in-wind?sd=rss&spid=14019

@atian25 It seems some people have the same issue about Invalid XML content. I can't figure out why, I have tested the script on 3 windows pc, works fine.
what about wmic Path win32_process Where "Name = 'node.exe'" Get CommandLine,ProcessId, output the same error ?
I am going to test it on appveyor.

worker for most of the window user is better than nothing~

BaffinLee · 2018-07-07T22:31:43Z

Passed every test case on appveyor, and travis-ci.

TODO:

find a replacement for tail command on windows. (not absolutly necessary)

By the way, I konw why I couldn't run test locally before, you guys use npminstall (cnpm) , but I use npm or yarn. I ~~will fire~~ a PR latter with detail.

BaffinLee · 2018-07-09T08:24:30Z

@atian25 @popomore Got time to review?

atian25 · 2018-07-09T10:30:20Z

 /* istanbul ignore else */
 if (m) {
- const item = { pid: m[1], cmd: m[2] };
+ const item = isWin ? { pid: m[2], cmd: m[1] } : { pid: m[1], cmd: m[2] };


why not just change Get CommandLine,ProcessId to Get ProcessId,CommandLine ?

+1 after fix this

In my test, ProcessId always show behind CommandLine, despite how you run the command.
So I just put ProcessId behind CommandLine, more clear, less confuse.

how about change the ps of linux to make they the same order ?

It's still confusing, Get CommandLine,ProcessId and Get ProcessId,CommandLine actually output the same thing, maybe I should add a comment in the code ? Got better idea? @atian25

something wrong with the REGEXP in my final commit, I will switch to linux to test it.

It's weird:
ps -eo pid,command works fine
fake@ubuntu$ ps -eo pid,command PID COMMAND 1 /init ro 24 ssh-agent 205 /init ro 206 -bash 2956 /init ro 2957 -bash 5274 npm 5284 sh -c npm run lint -- --fix && npm run pkgfiles && npm run test-local 5325 npm 5335 sh -c egg-bin test 5336 node /mnt/c/Users/fake/code/egg-scripts/node_modules/.bin/egg-bin test 5342 /usr/local/lib/nodejs/node/bin/node /mnt/c/Users/fake/code/egg-scripts/node_modules/egg-bin/node_modules/mocha/bin/_mocha --require=/mnt/c/Users/fake/code/egg-scripts/node_modules/egg-bin/lib/mocha-clean.js --require=/mnt/c/U... 5595 node /mnt/c/Users/fake/code/egg-scripts/lib/start-cluster {"framework":"/mnt/c/Users/fake/code/egg-scripts/test/fixtures/example/node_modules/yadan","workers":2,"title":"egg-server-example","baseDir":"/mnt/c/Users/fake/cod... 5680 /usr/local/lib/nodejs/node/bin/node /mnt/c/Users/fake/code/egg-scripts/bin/egg-scripts.js start --workers=2 --title=egg-test /mnt/c/Users/fake/code/egg-scripts/test/fixtures/example 5690 node /mnt/c/Users/fake/code/egg-scripts/lib/start-cluster {"workers":2,"title":"egg-test","framework":"/mnt/c/Users/fake/code/egg-scripts/test/fixtures/example/node_modules/custom-framework","baseDir":"/mnt/c/Users/fake/co... 5696 /usr/local/lib/nodejs/node/bin/node /mnt/c/Users/fake/code/egg-scripts/node_modules/egg-cluster/lib/agent_worker.js {"framework":"/mnt/c/Users/fake/code/egg-scripts/test/fixtures/example/node_modules/custom-framework","baseDi... 5706 /usr/local/lib/nodejs/node/bin/node /mnt/c/Users/fake/code/egg-scripts/node_modules/egg-cluster/lib/app_worker.js {"framework":"/mnt/c/Users/fake/code/egg-scripts/test/fixtures/example/node_modules/custom-framework","baseDir"... 5707 /usr/local/lib/nodejs/node/bin/node /mnt/c/Users/fake/code/egg-scripts/node_modules/egg-cluster/lib/app_worker.js {"framework":"/mnt/c/Users/fake/code/egg-scripts/test/fixtures/example/node_modules/custom-framework","baseDir"...
ps -eo command,pid faild: command was truncated
fake@ubuntu$ ps -eo command,pid COMMAND PID /init ro 1 ssh-agent 24 /init ro 205 -bash 206 /init ro 2956 -bash 2957 npm 5274 sh -c npm run lint -- --fix 5284 npm 5325 sh -c egg-bin test 5335 node /mnt/c/Users/fake/c 5336 /usr/local/lib/nodejs/node/ 5342 node /mnt/c/Users/fake/c 5595 /usr/local/lib/nodejs/node/ 5680 node /mnt/c/Users/fake/c 5690 /usr/local/lib/nodejs/node/ 5696 /usr/local/lib/nodejs/node/ 5706 /usr/local/lib/nodejs/node/ 5707 ps -eo command,pid 5741
refer to how-to-prevent-ps-from-truncating-the-process-name, I think I should rollback :) ? or set column width like: ps -eo command:1000,pid ?
I'd like to rollback, any idea?

no, just rollback to last review, thanks

@atian25 done. Test on appveyor passed too.

fengmk2 · 2018-07-10T08:37:00Z

should change README.md https://github.com/eggjs/egg-scripts/blob/master/README.md#egg-scripts too.

BaffinLee · 2018-07-10T09:34:30Z

@fengmk2 done.

atian25 · 2018-07-11T01:43:11Z

 deploy tool for egg project.

-**Note: Windows is not supported**
+**Note: Windows is partially supported, see [#22](https://github.com/eggjs/egg-scripts/pull/22)**


egg guide docs of deploy is also need to updated.
could raise anthoer PR to support start for win (if not tail instead, just ignore the checker at win)

https://ci.appveyor.com/project/eggjs/egg-scripts

PR for egg's docs, #2788

popomore · 2018-08-10T03:19:02Z

egg-scripts@2.7.0

waitingsong · 2018-09-04T11:05:19Z

有个现象， vscode 调试 egg 中，wmic 持续出现在系统任务列表中。大概3秒执行一次，每次0.3秒左右。
.vscode/launch.json:

 {
"name": "Egg Debug",
"type": "node",
"request": "launch",
"runtimeExecutable": "npm",
"runtimeArgs": [
"run",
"debug",
"--",
"--inspect-brk"
],
"console": "integratedTerminal",
"restart": true,
"protocol": "auto",
"port": 9229,
"autoAttachChildProcesses": true
},

单独运行 npm run debug 没发现有这状况。这和 vscode 有关？

BaffinLee · 2018-09-04T12:10:31Z

是吧，调试应该没用到 egg-scripts stop

feat: stop command support windows
bfa9c9c

BaffinLee mentioned this pull request Jul 6, 2018
Azure App service底下無法使用 eggjs/egg#2772
Closed

popomore reviewed Jul 6, 2018
View reviewed changes

test: adjust test to support windows
ec2fb5c

popomore approved these changes Jul 9, 2018
View reviewed changes

atian25 reviewed Jul 9, 2018
View reviewed changes

BaffinLee added 2 commits July 9, 2018 19:12

style: change the order of params
0f89f2d

fix: command is too long and will be truncated
9d9c9ce

docs: update readme about windows support
908efbe

fengmk2 approved these changes Jul 10, 2018
View reviewed changes

atian25 merged commit 22faa4c into eggjs:masterJul 11, 2018

atian25 reviewed Jul 11, 2018
View reviewed changes

BaffinLee mentioned this pull request Jul 11, 2018
docs(deployment): egg-scripts support windows eggjs/egg#2788
Merged
4 tasks

popomore mentioned this pull request Aug 10, 2018
feat: npm stop by win32 support #20
Closed
4 tasks

atian25 mentioned this pull request Aug 23, 2018
feat: add ipc channel in nonDaemon mode #28
Merged
3 tasks

Uh oh!

Conversation

BaffinLee commented Jul 6, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

codecovBot commented Jul 6, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atian25Jul 7, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BaffinLee commented Jul 7, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BaffinLee commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fengmk2 commented Jul 10, 2018

Uh oh!

BaffinLee commented Jul 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

popomore commented Aug 10, 2018

Uh oh!

waitingsong commented Sep 4, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BaffinLee commented Sep 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

BaffinLee commented Jul 6, 2018•
edited
Loading

codecovBot commented Jul 6, 2018•
edited
Loading

atian25Jul 7, 2018•
edited
Loading

BaffinLee commented Jul 7, 2018•
edited
Loading

waitingsong commented Sep 4, 2018•
edited
Loading