Skip to content
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

Add windowsHide option to allow Windows popup from pm2 process file #3466

Merged
merged 7 commits into from
Feb 19, 2018

Conversation

natcl
Copy link
Contributor

@natcl natcl commented Feb 14, 2018

Please always submit pull requests on the development branch.

Q A
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #3425
License MIT
Doc PR https://github.com/pm2-hive/pm2-hive.github.io/pulls

@natcl
Copy link
Contributor Author

natcl commented Feb 15, 2018

This build is currently failing with the error below, it doesn't pass the "Call PM2 inside PM2" test and I'm not sure why, complains that windowsHide is not a boolean even though I check if it's undefined and set it to to true explicitly. Any tips ? Thanks !

  Call PM2 inside PM2
[2018-02-14 23:57:05] PM2 log: Launching in no daemon mode
[2018-02-14 23:57:05] PM2 log: Starting execution sequence in -fork mode- for app name:start_inside id:0
[2018-02-14 23:57:05] PM2 log: App name:start_inside id:0 online
[2018-02-14 23:57:06] PM2 log: Starting execution sequence in -fork mode- for app name:echo id:1
[2018-02-14 23:57:06] PM2 error: Trace: TypeError: "windowsHide" must be a boolean
    at normalizeSpawnArguments (child_process.js:438:11)
    at exports.spawn (child_process.js:496:38)
    at /home/travis/build/Unitech/pm2/lib/God/ForkMode.js:98:20
    at /home/travis/build/Unitech/pm2/node_modules/async/dist/async.js:473:16
    at next (/home/travis/build/Unitech/pm2/node_modules/async/dist/async.js:5315:29)
    at /home/travis/build/Unitech/pm2/node_modules/async/dist/async.js:958:16
    at WriteStream.<anonymous> (/home/travis/build/Unitech/pm2/lib/Utility.js:172:13)
    at emitOne (events.js:116:13)
    at WriteStream.emit (events.js:211:7)
    at fs.open (fs.js:2153:10)
    at FSReqWrap.oncomplete (fs.js:135:15)
    at Object.God.logAndGenerateError (/home/travis/build/Unitech/pm2/lib/God/Methods.js:36:15)
    at /home/travis/build/Unitech/pm2/lib/God/ForkMode.js:106:13
    at /home/travis/build/Unitech/pm2/node_modules/async/dist/async.js:473:16
    at next (/home/travis/build/Unitech/pm2/node_modules/async/dist/async.js:5315:29)
    at /home/travis/build/Unitech/pm2/node_modules/async/dist/async.js:958:16
    at WriteStream.<anonymous> (/home/travis/build/Unitech/pm2/lib/Utility.js:172:13)
    at emitOne (events.js:116:13)
    at WriteStream.emit (events.js:211:7)
    at fs.open (fs.js:2153:10)
    at FSReqWrap.oncomplete (fs.js:135:15)

@natcl
Copy link
Contributor Author

natcl commented Feb 15, 2018

The test also runs successfully when I run it from my computer.

@vmarchaud
Copy link
Contributor

I'm not sure why you want it to be configurable since its by default at true ?

@natcl
Copy link
Contributor Author

natcl commented Feb 15, 2018

Because we use it to launch GUI apps so we need to be able to set the setting to false.

@vmarchaud
Copy link
Contributor

Okay i see, i through that it only hide the console window that spawn before the process, it make sense.

@@ -94,7 +94,7 @@ module.exports = function ForkMode(God) {
var cspr = spawn(command, args, {
env : pm2_env,
detached : true,
windowsHide: true,
windowsHide: pm2_env.windowsHide,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should do something like
windowsHide: pm2_env.windowsHide || true

Just in case variable is undefined, it's probably why tests are failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

windowsHide: pm2_env.windowsHide || true wouldn't work though because false || true equals true so it will never be false (which is the goal).
I did try adding the following however to ensure that windowsHide is not undefined but it still failed the build test in travis (but succeeded on my machine):

var windowsHide = true;
if (pm2_env.windowsHide != undefined) {
    windowsHide = pm2_env.windowsHide
}

Copy link
Contributor

@wallet77 wallet77 Feb 16, 2018

Choose a reason for hiding this comment

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

Check undefined is not enough I think, pm2_env.windowsHide can be null, or written as a string "false".
Maybe we should check if variable is a boolean, like so :
typeof(pm2_env.windowHide) === "boolean"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will test that now ! 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.

The fix worked but the tests failed for node4 although it doesn't seem related to those changes, is it possible it's a build error ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes sometimes tests failed on node4. I will rerun the job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks !

@wallet77 wallet77 merged commit c6d7ace into Unitech:development Feb 19, 2018
@natcl
Copy link
Contributor Author

natcl commented Feb 19, 2018

Thanks guys !

inerc pushed a commit to inerc/pm2 that referenced this pull request Feb 11, 2020
Add windowsHide option to allow Windows popup from pm2 process file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants