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: support win32 #11

Closed
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@waitingsong

waitingsong commented Nov 7, 2017

it should stop with --port if start cluster with custom port number

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change
feat: support win32
it should stop with --port if start cluster with custom port number
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 7, 2017

Codecov Report

Merging #11 into master will decrease coverage by 15.22%.
The diff coverage is 28.2%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #11       +/-   ##
===========================================
- Coverage     100%   84.77%   -15.23%     
===========================================
  Files           6        6               
  Lines         169      197       +28     
===========================================
- Hits          169      167        -2     
- Misses          0       30       +30
Impacted Files Coverage Δ
lib/cmd/stop.js 93.54% <100%> (-6.46%) ⬇️
lib/helper.js 48.78% <16%> (-51.22%) ⬇️
lib/cmd/start.js 93.39% <46.15%> (-6.61%) ⬇️

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 0016e28...9199d58. Read the comment docs.

codecov-io commented Nov 7, 2017

Codecov Report

Merging #11 into master will decrease coverage by 15.22%.
The diff coverage is 28.2%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #11       +/-   ##
===========================================
- Coverage     100%   84.77%   -15.23%     
===========================================
  Files           6        6               
  Lines         169      197       +28     
===========================================
- Hits          169      167        -2     
- Misses          0       30       +30
Impacted Files Coverage Δ
lib/cmd/stop.js 93.54% <100%> (-6.46%) ⬇️
lib/helper.js 48.78% <16%> (-51.22%) ⬇️
lib/cmd/start.js 93.39% <46.15%> (-6.61%) ⬇️

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 0016e28...9199d58. Read the comment docs.

waitingsong added some commits Nov 7, 2017

@fengmk2

This comment has been minimized.

Show comment
Hide comment
@fengmk2

fengmk2 Nov 7, 2017

Member

@atian25 add appveyor tests

Member

fengmk2 commented Nov 7, 2017

@atian25 add appveyor tests

@atian25

This comment has been minimized.

Show comment
Hide comment
@atian25

atian25 Nov 7, 2017

Member

but egg-scripts start is not support windows yet, so appveyor will break, should we still open it?

Member

atian25 commented Nov 7, 2017

but egg-scripts start is not support windows yet, so appveyor will break, should we still open it?

@waitingsong

This comment has been minimized.

Show comment
Hide comment
@waitingsong

waitingsong Nov 7, 2017

I run test local all passed under win7. not support ?

waitingsong commented Nov 7, 2017

I run test local all passed under win7. not support ?

@atian25

This comment has been minimized.

Show comment
Hide comment
@atian25

atian25 Nov 7, 2017

Member

https://github.com/eggjs/egg-scripts/blob/master/lib/cmd/start.js#L180-L196

tails is support at win? maybe you had install some tool such as git bin (with an options to install bash tools)

Member

atian25 commented Nov 7, 2017

https://github.com/eggjs/egg-scripts/blob/master/lib/cmd/start.js#L180-L196

tails is support at win? maybe you had install some tool such as git bin (with an options to install bash tools)

@waitingsong

This comment has been minimized.

Show comment
Hide comment
@waitingsong

waitingsong Nov 7, 2017

oh, git installed .maybe can be fixed for win32

waitingsong commented Nov 7, 2017

oh, git installed .maybe can be fixed for win32

waitingsong added some commits Nov 7, 2017

@waitingsong

This comment has been minimized.

Show comment
Hide comment
@waitingsong

waitingsong Nov 7, 2017

 stdout = stdout.split(/\n+/g).splice(0, 100);
 +            this.logger.error(stdout);
 +            this.logger.error('Start failed, see %s', stderr);

Whether need convert stdout to string by stdout.join('\n') ?

waitingsong commented Nov 7, 2017

 stdout = stdout.split(/\n+/g).splice(0, 100);
 +            this.logger.error(stdout);
 +            this.logger.error('Start failed, see %s', stderr);

Whether need convert stdout to string by stdout.join('\n') ?

Show outdated Hide outdated lib/cmd/start.js Outdated
const { argv } = context;
const port = argv.port || (process.env && process.env.PORT);

This comment has been minimized.

@popomore

popomore Nov 7, 2017

Member

Is there other way to stop without port

@popomore

popomore Nov 7, 2017

Member

Is there other way to stop without port

This comment has been minimized.

@waitingsong

waitingsong Nov 7, 2017

有其他方法。但相当复杂
1、修改egg-cluster, 在启动 worker 进程时,更新窗口的 title
2、在关闭服务时借助node-ffi 找到任何一个匹配 title 的 worker 窗口获取其pid
3、借助node-ffi 调用 ntdll.NtQueryInformationProcess() 方法,根据pid找到父级pid(即master进程),然后kill。

我计划实现(1, 2),并且可根据参数隐藏worker的窗口。
因为master进程没有窗口,也就无法设置其窗口 title,所以只有在 worker 上面设置了。 否则就可以跳过(3)直接找到 master 进程窗口然后 kill 之。

(3)这个 NtQueryInformationProcess 需要的 参数结构Struct极其复杂,目前搞不定。

@waitingsong

waitingsong Nov 7, 2017

有其他方法。但相当复杂
1、修改egg-cluster, 在启动 worker 进程时,更新窗口的 title
2、在关闭服务时借助node-ffi 找到任何一个匹配 title 的 worker 窗口获取其pid
3、借助node-ffi 调用 ntdll.NtQueryInformationProcess() 方法,根据pid找到父级pid(即master进程),然后kill。

我计划实现(1, 2),并且可根据参数隐藏worker的窗口。
因为master进程没有窗口,也就无法设置其窗口 title,所以只有在 worker 上面设置了。 否则就可以跳过(3)直接找到 master 进程窗口然后 kill 之。

(3)这个 NtQueryInformationProcess 需要的 参数结构Struct极其复杂,目前搞不定。

This comment has been minimized.

@popomore

popomore Nov 7, 2017

Member

Or we can store port when app starts, so it’s unnecessary to give the port argument.

@popomore

popomore Nov 7, 2017

Member

Or we can store port when app starts, so it’s unnecessary to give the port argument.

This comment has been minimized.

@waitingsong

waitingsong Nov 7, 2017

能记录所有进程 pid 那当然就方便了

@waitingsong

waitingsong Nov 7, 2017

能记录所有进程 pid 那当然就方便了

@atian25

This comment has been minimized.

Show comment
Hide comment
@atian25

atian25 Nov 7, 2017

Member
Member

atian25 commented Nov 7, 2017

@atian25

This comment has been minimized.

Show comment
Hide comment
@atian25

atian25 Nov 8, 2017

Member

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

had enabled, need an empty commit to trigger it.

btw, the cov is low.

Member

atian25 commented Nov 8, 2017

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

had enabled, need an empty commit to trigger it.

btw, the cov is low.

@waitingsong

This comment has been minimized.

Show comment
Hide comment
@waitingsong

waitingsong Nov 8, 2017

commit one, but seems not triggered. only master branch?

waitingsong commented Nov 8, 2017

commit one, but seems not triggered. only master branch?

@atian25

This comment has been minimized.

Show comment
Hide comment
@atian25

atian25 Nov 8, 2017

Member

try again and see, just delete and recreate it.

Member

atian25 commented Nov 8, 2017

try again and see, just delete and recreate it.

waitingsong added some commits Nov 8, 2017

Show outdated Hide outdated lib/cmd/start.js Outdated

@popomore popomore referenced this pull request Nov 14, 2017

Merged

fix: should stop app when baseDir is symlink #12

1 of 4 tasks complete

@waitingsong waitingsong closed this Apr 4, 2018

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