New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: stop command support windows #22

Merged
merged 5 commits into from Jul 11, 2018

Conversation

Projects
None yet
5 participants
@BaffinLee
Contributor

BaffinLee commented Jul 6, 2018

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

This comment has been minimized.

codecov bot commented Jul 6, 2018

Codecov Report

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

Impacted file tree graph

@@          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.

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

This comment has been minimized.

@popomore

popomore Jul 6, 2018

Member

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

This comment has been minimized.

@BaffinLee

BaffinLee Jul 6, 2018

Contributor

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

image

This comment has been minimized.

@popomore

popomore Jul 6, 2018

Member

It seems good

This comment has been minimized.

@atian25

atian25 Jul 7, 2018

Member

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

This comment has been minimized.

@BaffinLee

BaffinLee Jul 7, 2018

Contributor

@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.

This comment has been minimized.

@atian25

atian25 Jul 7, 2018

Member

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

@BaffinLee

This comment has been minimized.

Contributor

BaffinLee commented Jul 7, 2018

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

image

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

This comment has been minimized.

Contributor

BaffinLee commented Jul 9, 2018

@atian25 @popomore Got time to review?

const stdio = yield runScript(command, { stdio: 'pipe' });
const processList = stdio.stdout.toString().split('\n')
.reduce((arr, line) => {
if (!!line && !line.includes('/bin/sh') && line.includes('node')) {
const m = line.match(REGEX);
/* 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] };

This comment has been minimized.

@atian25

atian25 Jul 9, 2018

Member

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

This comment has been minimized.

@atian25

atian25 Jul 9, 2018

Member

+1 after fix this

This comment has been minimized.

@BaffinLee

BaffinLee Jul 9, 2018

Contributor

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.

This comment has been minimized.

@atian25

atian25 Jul 9, 2018

Member

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

This comment has been minimized.

@BaffinLee

BaffinLee Jul 9, 2018

Contributor

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

This comment has been minimized.

@BaffinLee

BaffinLee Jul 9, 2018

Contributor

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

This comment has been minimized.

@BaffinLee

BaffinLee Jul 9, 2018

Contributor

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?

This comment has been minimized.

@BaffinLee

This comment has been minimized.

@atian25

atian25 Jul 10, 2018

Member

no, just rollback to last review, thanks

This comment has been minimized.

@BaffinLee

BaffinLee Jul 10, 2018

Contributor

@atian25 done. Test on appveyor passed too.

@fengmk2

This comment has been minimized.

Member

fengmk2 commented Jul 10, 2018

@BaffinLee

This comment has been minimized.

Contributor

BaffinLee commented Jul 10, 2018

@fengmk2 done.

@atian25 atian25 merged commit 22faa4c into eggjs:master Jul 11, 2018

7 checks passed

WIP ready for review
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 82f4125
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (eggjs) No manifest changes detected
security/snyk - package.json (gxcsoccer) No manifest changes detected
security/snyk - package.json (popomore) No manifest changes detected
@@ -2,7 +2,7 @@
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)**

This comment has been minimized.

@atian25

atian25 Jul 11, 2018

Member
  • 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)

This comment has been minimized.

This comment has been minimized.

@BaffinLee

BaffinLee Jul 11, 2018

Contributor

PR for egg's docs, #2788

@BaffinLee BaffinLee referenced this pull request Jul 11, 2018

Merged

docs(deployment): egg-scripts support windows #2788

4 of 4 tasks complete
@popomore

This comment has been minimized.

Member

popomore commented Aug 10, 2018

  • egg-scripts@2.7.0

@popomore popomore referenced this pull request Aug 10, 2018

Closed

feat: npm stop by win32 support #20

2 of 4 tasks complete

@atian25 atian25 referenced this pull request Aug 23, 2018

Merged

feat: add ipc channel in nonDaemon mode #28

3 of 3 tasks complete
@waitingsong

This comment has been minimized.

waitingsong commented Sep 4, 2018

有个现象, 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

This comment has been minimized.

Contributor

BaffinLee commented Sep 4, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment