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

fix: hooking SIGPROF was interfering with profilers see #21 #24

Merged
merged 3 commits into from
Jun 13, 2016
Merged

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Jun 10, 2016

BREAKING CHANGE: signal-exit no longer wires into SIGPROF

Stop wiring into SIGPROF, this was causing issues discussed in #21 and npm/npmlog#40

@coveralls
Copy link

coveralls commented Jun 10, 2016

Coverage Status

Coverage remained the same at 96.512% when pulling 0734aaf on 21-fix into 6f3eda8 on master.

@bcoe bcoe merged commit 1248a4c into master Jun 13, 2016
@bcoe bcoe deleted the 21-fix branch June 13, 2016 22:32
@@ -77,7 +79,7 @@ function listener (code, signal) {
function run (opt) {
console.error(opt)
var shell = process.platform === 'win32' ? null : { shell: '/bin/bash' }
exec(process.execPath + ' ' + __filename + ' ' + opt, shell, function (err, stdout, stderr) {
exec(join(process.execPath, ' ', __filename, ' ' + opt), shell, function (err, stdout, stderr) {
Copy link
Member

Choose a reason for hiding this comment

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

This is a weird use of path.join. Won't that result in something like /path/to/node/ with a slash at the end?

Copy link
Member Author

Choose a reason for hiding this comment

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

100% agree this is a weird new standard nit that I was working around -- if it sees something that looks like a path being execed, it yells at you until you use path.resolve or path.join.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds like a bug in standard. Especially since you can still throw spaces in there anyway.

Would it be ok with you doing exec(path.resolve(process.execPath) + ' ' + __filename + ' ' + opt)? Ie, just resolve the execPath, not the whole command line.

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