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

tools: run tick processor without forking #4224

Closed
wants to merge 1 commit into from
Closed

tools: run tick processor without forking #4224

wants to merge 1 commit into from

Conversation

matthewloring
Copy link

Using the tick processor no longer creates temporary files or spawns a
child process.

/cc @bnoordhuis

@mscdex mscdex added the tools Issues and PRs related to the tools directory. label Dec 10, 2015
fs.writeFileSync(tempNm, process.binding('natives')['v8/tools/mac-nm'],
{ mode: 0o555 });
tickArguments.push('--mac', '--nm=' + path.join(process.cwd(), tempNm));
var nm = 'foo() { nm "$@" | (c++filt -p -i || cat) }; foo';
Copy link
Member

Choose a reason for hiding this comment

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

maybe const nm

Copy link
Author

Choose a reason for hiding this comment

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

I've converted as many of the vars in the polyfill and processor to const as I could. Unfortunately, the lack of strict mode prevents the use of let (not sure why const is allowed outside of strict now that I think about it).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, if we are not in strict mode, shouldn't we use var instead. Sloppy mode const is totally different IIRC

Copy link
Author

Choose a reason for hiding this comment

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

The script appears to behave correctly using either const or var and I am unfamiliar with how things operate outside of strict mode. Happy to switch back to var if that is preferred.

@JungMinu
Copy link
Member

Thank you for your update, CI : https://ci.nodejs.org/job/node-test-pull-request/973/

return require('child_process').execFileSync(
name, args, {encoding: 'utf8'});
const cmd = name + ' ' + args.join(' ');
return cp.execSync(cmd, {encoding: 'utf8'});
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use .spawnSync() here? You can pass it arguments as an array and it will take care of shell escaping them.

Copy link
Author

Choose a reason for hiding this comment

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

When I use spawnSync, I get the error Error: spawnSync foo() { nm "$@" | (c++filt -p -i || cat) }; foo ENOENT. I'm not sure exactly how spawnSync and execSync differ but they're not handling this command the same way.

Copy link
Member

Choose a reason for hiding this comment

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

.execSync() executes the command as /bin/sh -c '<cmd> <args>' (cmd.exe /s /c "<cmd> <args>" on Windows) whereas .spawnSync() simply calls execve(). Guess I was wrong saying it takes care of shell escaping because it doesn't need to escape anything.

You could wrap that bit of shell script in a sh -c or bash -c below. Longer term, it might be nice to move normalizeExecArgs() from lib/child_process.js to lib/internal/child_process.js so you can use it here. It's possible some more work needs to be done on the internal module system to make that work, though.

I'll leave it up to you if you want to change this line.

Copy link
Author

Choose a reason for hiding this comment

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

I don't believe wrapping it in sh -c or bash -c will work in this instance because the shell script defines a function and then calls it on arguments that will be provided later at the call site. This is the snippet in question:

foo() { nm "$@" | (c++filt -p -i || cat) }; foo

Later it will be invoked as:

foo() { nm "$@" | (c++filt -p -i || cat) }; foo -n -f /usr/lib/system/example.dylib

Is it possible to perform that wrapping you described in this case?

Copy link
Member

Choose a reason for hiding this comment

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

sh -c 'foo() { ... }; foo $@' -- should work, if I understand you right.

Copy link
Author

Choose a reason for hiding this comment

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

I have this working now. Will spawning /bin/sh work correctly on windows or does that need to be special cased?

Copy link
Member

Choose a reason for hiding this comment

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

On Windows there is no /bin/sh, you'd use cmd.exe /s /c, but I don't think you actually need to spawn a shell. The OS X case is special because you're executing shell script, on other platforms nm is invoked directly.

Copy link
Author

Choose a reason for hiding this comment

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

I can handle the OS X case specially here if that is preferable. What is the motivation of changing execSync to spawnSync?

Copy link
Member

Choose a reason for hiding this comment

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

That you won't have to worry about escaping spaces in paths, for example.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, that makes sense, I've added the special casing here and it's working on my test machine (OS X).

@bnoordhuis
Copy link
Member

LGTM with a suggestion.

@jasnell
Copy link
Member

jasnell commented Dec 18, 2015

LGTM

Using the tick processor no longer creates temporary files or spawns a
child process.
@bnoordhuis
Copy link
Member

ofrobots pushed a commit that referenced this pull request Dec 29, 2015
Using the tick processor no longer creates temporary files or spawns a
child process.

PR-URL: #4224
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
@ofrobots
Copy link
Contributor

Thanks. Landed on master in 3e2a2e6.

@ofrobots ofrobots closed this Dec 29, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
Using the tick processor no longer creates temporary files or spawns a
child process.

PR-URL: nodejs#4224
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

@jasnell should this go to LTS?

@MylesBorins
Copy link
Contributor

relies on #4021

@matthewloring matthewloring deleted the tp-no-cp branch February 25, 2016 19:56
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Mar 1, 2016
Using the tick processor no longer creates temporary files or spawns a
child process.

PR-URL: nodejs#4224
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
Using the tick processor no longer creates temporary files or spawns a
child process.

PR-URL: #4224
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Using the tick processor no longer creates temporary files or spawns a
child process.

PR-URL: #4224
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Using the tick processor no longer creates temporary files or spawns a
child process.

PR-URL: nodejs#4224
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants