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

solve #306, and bin in $PATH make sense now #307

Closed
wants to merge 15 commits into from

Conversation

zhiyelee
Copy link
Collaborator

solve #306, and bin in $PATH make sense now #196
Almost revert #173 which lead to the bug in #306

Also remove deprecated customFds option in spawn

Almost revert tj#173 which lead to the bug in tj#306
@zhiyelee
Copy link
Collaborator Author

I have no idea how to compatible with Windows here #142
cc @SomeKittens @yiminghe

@zhiyelee
Copy link
Collaborator Author

Seems on windows, ppl must use node pm-install instead of pm-install cc @yiminghe
But with #306 , we can only work with js bin files. But other types of executable files can not make sense, such as bash php etc

@zhiyelee
Copy link
Collaborator Author

Now windows also make sense. I will write some tests for this later.

var local = path.join(dir, bin);
if (fs.existsSync(local)) bin = local;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's deprecated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rlidwka Not yet heard that, Thanks a lot. Maybe we can replace it with fs.accessSync after v0.12, but it makes sense now.

called "nodejs", and on windows it might be renamed too because of the
conflict with node.exe service from microsoft. thx @rlidwka
@SomeKittens
Copy link
Collaborator

Looks like this fixes more than one edge case. Can we get the tests passing?

var proc = spawn('node', args, { stdio: 'inherit', customFds: [0, 1, 2] });

var proc;
if (process.platform !== 'win32') {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With the commit, bin file must have shebang itself

@zhiyelee
Copy link
Collaborator Author

zhiyelee commented Jan 9, 2015

Closed for too many commits. I'll open another PR

@zhiyelee zhiyelee closed this Jan 9, 2015
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