Skip to content

feat: stop command support windows #22

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jul 11, 2018
Merged

feat: stop command support windows #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bfa9c9c
feat: stop command support windows
BaffinLee Jul 6, 2018
ec2fb5c
test: adjust test to support windows
BaffinLee Jul 7, 2018
0f89f2d
style: change the order of params
BaffinLee Jul 9, 2018
9d9c9ce
fix: command is too long and will be truncated
BaffinLee Jul 10, 2018
908efbe
docs: update readme about windows support
BaffinLee Jul 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR for egg's docs, #2788


## Install

Expand Down
18 changes: 9 additions & 9 deletions lib/cmd/stop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@
const path = require('path');
const sleep = require('mz-modules/sleep');
const Command = require('../command');
const isWin = process.platform === 'win32';
const osRelated = {
titlePrefix: isWin ? '\\"title\\":\\"' : '"title":"',
appWorkerPath: isWin ? 'egg-cluster\\lib\\app_worker.js' : 'egg-cluster/lib/app_worker.js',
agentWorkerPath: isWin ? 'egg-cluster\\lib\\agent_worker.js' : 'egg-cluster/lib/agent_worker.js',
};

class StopCommand extends Command {

Expand All @@ -23,12 +29,6 @@ class StopCommand extends Command {
}

* run(context) {
/* istanbul ignore next */
if (process.platform === 'win32') {
this.logger.warn('Windows is not supported, try to kill master process which command contains `start-cluster` or `--type=egg-server` yourself, good luck.');
process.exit(0);
}

const { argv } = context;

this.logger.info(`stopping egg application ${argv.title ? `with --title=${argv.title}` : ''}`);
Expand All @@ -37,7 +37,7 @@ class StopCommand extends Command {
let processList = yield this.helper.findNodeProcess(item => {
const cmd = item.cmd;
return argv.title ?
cmd.includes('start-cluster') && cmd.includes(`"title":"${argv.title}"`) :
cmd.includes('start-cluster') && cmd.includes(`${osRelated.titlePrefix}${argv.title}`) :
cmd.includes('start-cluster');
});
let pids = processList.map(x => x.pid);
Expand All @@ -57,8 +57,8 @@ class StopCommand extends Command {
processList = yield this.helper.findNodeProcess(item => {
const cmd = item.cmd;
return argv.title ?
(cmd.includes('egg-cluster/lib/app_worker.js') || cmd.includes('egg-cluster/lib/agent_worker.js')) && cmd.includes(`"title":"${argv.title}"`) :
(cmd.includes('egg-cluster/lib/app_worker.js') || cmd.includes('egg-cluster/lib/agent_worker.js'));
(cmd.includes(osRelated.appWorkerPath) || cmd.includes(osRelated.agentWorkerPath)) && cmd.includes(`${osRelated.titlePrefix}${argv.title}`) :
(cmd.includes(osRelated.appWorkerPath) || cmd.includes(osRelated.agentWorkerPath));
});
pids = processList.map(x => x.pid);

Expand Down
9 changes: 6 additions & 3 deletions lib/helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
'use strict';

const runScript = require('runscript');
const REGEX = /^\s*(\d+)\s+(.*)/;
const isWin = process.platform === 'win32';
const REGEX = isWin ? /^(.*)\s+(\d+)\s*$/ : /^\s*(\d+)\s+(.*)/;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems good

Copy link
Member

@atian25 atian25 Jul 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'ps -eo "pid,command"';
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] };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 after fix this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atian25

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, just rollback to last review, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atian25 done. Test on appveyor passed too.

if (!filterFn || filterFn(item)) {
arr.push(item);
}
Expand Down
3 changes: 2 additions & 1 deletion test/fixtures/ts-pkg/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"typescript": true
},
"scripts": {
"build": "node ../../../node_modules/.bin/tsc"
"build": "node ../../../node_modules/.bin/tsc",
"windows-build": "call ../../../node_modules/.bin/tsc.cmd"
}
}
3 changes: 2 additions & 1 deletion test/fixtures/ts/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"egg": "^1.0.0"
},
"scripts": {
"build": "node ../../../node_modules/.bin/tsc"
"build": "node ../../../node_modules/.bin/tsc",
"windows-build": "call ../../../node_modules/.bin/tsc.cmd"
}
}
42 changes: 24 additions & 18 deletions test/start.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const coffee = require('coffee');
const httpclient = require('urllib');
const mm = require('mm');
const utils = require('./utils');
const isWin = process.platform === 'win32';

describe('test/start.test.js', () => {
const eggBin = require.resolve('../bin/egg-scripts.js');
Expand Down Expand Up @@ -271,7 +272,8 @@ describe('test/start.test.js', () => {
let result = yield httpclient.request('http://127.0.0.1:7001/env');
assert(result.data.toString() === 'pre, true');
result = yield httpclient.request('http://127.0.0.1:7001/path');
assert(result.data.toString().match(new RegExp(`^${fixturePath}/node_modules/.bin${path.delimiter}`)));
const appBinPath = path.join(fixturePath, 'node_modules/.bin');
assert(result.data.toString().startsWith(`${appBinPath}${path.delimiter}`));
});
});

Expand Down Expand Up @@ -402,23 +404,23 @@ describe('test/start.test.js', () => {
describe('start with daemon', () => {
let cwd;
beforeEach(function* () {
yield utils.cleanup(cwd);
if (cwd) yield utils.cleanup(cwd);
yield rimraf(logDir);
yield mkdirp(logDir);
yield fs.writeFile(path.join(logDir, 'master-stdout.log'), 'just for test');
yield fs.writeFile(path.join(logDir, 'master-stderr.log'), 'just for test');
});
afterEach(function* () {
yield coffee.fork(eggBin, [ 'stop', cwd ])
// .debug()
// .debug()
.end();
yield utils.cleanup(cwd);
});

it('should start custom-framework', function* () {
cwd = fixturePath;
yield coffee.fork(eggBin, [ 'start', '--daemon', '--workers=2', '--port=7002', cwd ])
// .debug()
// .debug()
.expect('stdout', /Starting custom-framework application/)
.expect('stdout', /custom-framework started on http:\/\/127\.0\.0\.1:7002/)
.expect('code', 0)
Expand All @@ -442,7 +444,7 @@ describe('test/start.test.js', () => {
it('should start default egg', function* () {
cwd = path.join(__dirname, 'fixtures/egg-app');
yield coffee.fork(eggBin, [ 'start', '--daemon', '--workers=2', cwd ])
// .debug()
// .debug()
.expect('stdout', /Starting egg application/)
.expect('stdout', /egg started on http:\/\/127\.0\.0\.1:7001/)
.expect('code', 0)
Expand All @@ -455,15 +457,15 @@ describe('test/start.test.js', () => {

after(function* () {
yield coffee.fork(eggBin, [ 'stop', cwd ])
// .debug()
// .debug()
.end();
yield utils.cleanup(cwd);
});

it('should status check success, exit with 0', function* () {
mm(process.env, 'WAIT_TIME', 5000);
yield coffee.fork(eggBin, [ 'start', '--daemon', '--workers=1' ], { cwd })
// .debug()
// .debug()
.expect('stdout', /Wait Start: 5.../)
.expect('stdout', /custom-framework started/)
.expect('code', 0)
Expand All @@ -474,12 +476,14 @@ describe('test/start.test.js', () => {
mm(process.env, 'WAIT_TIME', 5000);
mm(process.env, 'ERROR', 'error message');

const stderr = path.join(homePath, 'logs/master-stderr.log');
let stderr = path.join(homePath, 'logs/master-stderr.log');
if (isWin) stderr = stderr.replace(/\\/g, '\\\\');

yield coffee.fork(eggBin, [ 'start', '--daemon', '--workers=1', '--ignore-stderr' ], { cwd })
// .debug()
.expect('stderr', /nodejs.Error: error message/)
.expect('stderr', new RegExp(`Start got error, see ${stderr}`))
const app = coffee.fork(eggBin, [ 'start', '--daemon', '--workers=1', '--ignore-stderr' ], { cwd });
// app.debug();
// TODO: find a windows replacement for tail command
if (!isWin) app.expect('stderr', /nodejs.Error: error message/);
yield app.expect('stderr', new RegExp(`Start got error, see ${stderr}`))
.expect('code', 0)
.end();
});
Expand All @@ -488,12 +492,14 @@ describe('test/start.test.js', () => {
mm(process.env, 'WAIT_TIME', 5000);
mm(process.env, 'ERROR', 'error message');

const stderr = path.join(homePath, 'logs/master-stderr.log');
let stderr = path.join(homePath, 'logs/master-stderr.log');
if (isWin) stderr = stderr.replace(/\\/g, '\\\\');

yield coffee.fork(eggBin, [ 'start', '--daemon', '--workers=1' ], { cwd })
// .debug()
.expect('stderr', /nodejs.Error: error message/)
.expect('stderr', new RegExp(`Start got error, see ${stderr}`))
const app = coffee.fork(eggBin, [ 'start', '--daemon', '--workers=1' ], { cwd });
// app.debug()
// TODO: find a windows replacement for tail command
if (!isWin) app.expect('stderr', /nodejs.Error: error message/);
yield app.expect('stderr', new RegExp(`Start got error, see ${stderr}`))
.expect('code', 1)
.end();
});
Expand All @@ -502,7 +508,7 @@ describe('test/start.test.js', () => {
mm(process.env, 'WAIT_TIME', 10000);

yield coffee.fork(eggBin, [ 'start', '--daemon', '--workers=1', '--timeout=5000' ], { cwd })
// .debug()
// .debug()
.expect('stdout', /Wait Start: 1.../)
.expect('stderr', /Start failed, 5s timeout/)
.expect('code', 1)
Expand Down
71 changes: 51 additions & 20 deletions test/stop.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const coffee = require('coffee');
const httpclient = require('urllib');
const mm = require('mm');
const utils = require('./utils');
const isWin = process.platform === 'win32';

describe('test/stop.test.js', () => {
const eggBin = require.resolve('../bin/egg-scripts.js');
Expand Down Expand Up @@ -60,10 +61,14 @@ describe('test/stop.test.js', () => {
// make sure is kill not auto exist
assert(!app.stdout.includes('exist by env'));

assert(app.stdout.includes('[master] receive signal SIGTERM, closing'));
assert(app.stdout.includes('[master] exit with code:0'));
assert(app.stdout.includes('[app_worker] exit with code:0'));
// assert(app.stdout.includes('[agent_worker] exit with code:0'));
// no way to handle the SIGTERM signal in windows ?
if (!isWin) {
assert(app.stdout.includes('[master] receive signal SIGTERM, closing'));
assert(app.stdout.includes('[master] exit with code:0'));
assert(app.stdout.includes('[app_worker] exit with code:0'));
// assert(app.stdout.includes('[agent_worker] exit with code:0'));
}

assert(killer.stdout.includes('[egg-scripts] stopping egg application'));
assert(killer.stdout.match(/got master pid \["\d+\"\]/i));
});
Expand Down Expand Up @@ -98,9 +103,12 @@ describe('test/stop.test.js', () => {
// master log
const stdout = yield fs.readFile(path.join(logDir, 'master-stdout.log'), 'utf-8');

assert(stdout.includes('[master] receive signal SIGTERM, closing'));
assert(stdout.includes('[master] exit with code:0'));
assert(stdout.includes('[app_worker] exit with code:0'));
// no way to handle the SIGTERM signal in windows ?
if (!isWin) {
assert(stdout.includes('[master] receive signal SIGTERM, closing'));
assert(stdout.includes('[master] exit with code:0'));
assert(stdout.includes('[app_worker] exit with code:0'));
}

yield coffee.fork(eggBin, [ 'stop', fixturePath ])
.debug()
Expand Down Expand Up @@ -162,10 +170,14 @@ describe('test/stop.test.js', () => {
// make sure is kill not auto exist
assert(!app.stdout.includes('exist by env'));

assert(app.stdout.includes('[master] receive signal SIGTERM, closing'));
assert(app.stdout.includes('[master] exit with code:0'));
assert(app.stdout.includes('[app_worker] exit with code:0'));
// assert(app.stdout.includes('[agent_worker] exit with code:0'));
// no way to handle the SIGTERM signal in windows ?
if (!isWin) {
assert(app.stdout.includes('[master] receive signal SIGTERM, closing'));
assert(app.stdout.includes('[master] exit with code:0'));
assert(app.stdout.includes('[app_worker] exit with code:0'));
// assert(app.stdout.includes('[agent_worker] exit with code:0'));
}

assert(killer.stdout.includes('[egg-scripts] stopping egg application with --title=example'));
assert(killer.stdout.match(/got master pid \["\d+\"\]/i));
});
Expand Down Expand Up @@ -214,31 +226,50 @@ describe('test/stop.test.js', () => {

// make sure is kill not auto exist
assert(!app.stdout.includes('exist by env'));
assert(app.stdout.includes('[master] receive signal SIGTERM, closing'));
assert(app.stdout.includes('[master] exit with code:0'));
assert(app.stdout.includes('[app_worker] exit with code:0'));
// assert(app.stdout.includes('[agent_worker] exit with code:0'));

// no way to handle the SIGTERM signal in windows ?
if (!isWin) {
assert(app.stdout.includes('[master] receive signal SIGTERM, closing'));
assert(app.stdout.includes('[master] exit with code:0'));
assert(app.stdout.includes('[app_worker] exit with code:0'));
// assert(app.stdout.includes('[agent_worker] exit with code:0'));
}

assert(killer.stdout.includes('[egg-scripts] stopping egg application'));
assert(killer.stdout.match(/got master pid \["\d+\","\d+\"\]/i));

assert(!app2.stdout.includes('exist by env'));
assert(app2.stdout.includes('[master] receive signal SIGTERM, closing'));
assert(app2.stdout.includes('[master] exit with code:0'));
assert(app2.stdout.includes('[app_worker] exit with code:0'));

// no way to handle the SIGTERM signal in windows ?
if (!isWin) {
assert(app2.stdout.includes('[master] receive signal SIGTERM, closing'));
assert(app2.stdout.includes('[master] exit with code:0'));
assert(app2.stdout.includes('[app_worker] exit with code:0'));
}
});
});

describe('stop with symlink', () => {
const baseDir = path.join(__dirname, 'fixtures/tmp');

beforeEach(function* () {
yield fs.symlink(fixturePath, baseDir);
// if we can't create a symlink, skip the test
try {
yield fs.symlink(fixturePath, baseDir, 'dir');
} catch (err) {
// may get Error: EPERM: operation not permitted on windows
console.log(`test skiped, can't create symlink: ${err}`);
this.skip();
}

// *unix get the real path of symlink, but windows wouldn't
const appPathInRegexp = isWin ? baseDir.replace(/\\/g, '\\\\') : fixturePath;

yield utils.cleanup(fixturePath);
yield rimraf(logDir);
yield coffee.fork(eggBin, [ 'start', '--daemon', '--workers=2' ], { cwd: baseDir })
.debug()
.expect('stdout', new RegExp(`Starting custom-framework application at ${fixturePath}`))
.expect('stdout', new RegExp(`Starting custom-framework application at ${appPathInRegexp}`))
.expect('code', 0)
.end();

Expand Down
Loading