jbenet / node-subcomandante

taaaaake ooooooon meeee
4 stars 8 forks source link

Relies on launch shell having nodejs in PATH #1

Closed dignifiedquire closed 8 years ago

dignifiedquire commented 8 years ago

From IRC

dignifiedquire: daviddias: whyrusleeping jbenet any of you guys have an idea how this error could be generated https://gist.github.com/Dignifiedquire/9bde0097eb05e4cb8c8f (yes the files in question exist I checked that) [12:01am] whyrusleeping: dignifiedquire: how did you repro that? [12:01am] daviddias: how does that happen? [12:01am] whyrusleeping: looks like nodejs stuff [12:01am] whyrusleeping: node.js.js [12:01am] dignifiedquire: it only happens when I package the electron app [12:02am] whyrusleeping: you have node installed? [12:02am] dignifiedquire: it doesn’t happen without [12:02am] Encrypt left the chat room. (Quit: Sleeping time!) [12:02am] dignifiedquire: only if you use the right shell [12:02am] dignifiedquire: (zsh + nvm) [12:04am] dignifiedquire: so you think the issue is that the subcom file doesn’t find the node binary? [12:07am] dignifiedquire: makes sense, now that I reread the error message -.- [12:10am] noffle: whyrusleeping: alas! [12:10am] dignifiedquire: it seems to me that this is an issue in subcomandante, as it shouldn’t rely on the shell having node linked

It seems the culprit is relying on the hash bang here as that will only work if the enclosing shell does have nodejs in PATH, which we can not guarantee when packaging it as a native app as far as I understand.

daviddias commented 8 years ago

16:20 <+daviddias> dignifiedquire was trying to get it working simply by removing that line 16:20 <+daviddias> it fired up the node 16:21 <+daviddias> we shouldn't have to specify that in the file since it is always run in the node context

jbenet commented 8 years ago

The point of subcom binary is that it needs to run as a separate process. We need to find a way to execute it with node without expecting the standard toolchain to be there.

Can we expect node to be there?

jbenet commented 8 years ago

we could remove that line, and then exec it with node directly, by finding where the current node binary from the parent context is located.

dignifiedquire commented 8 years ago

we could remove that line, and then exec it with node directly, by finding where the current node binary from the parent context is located.

Agreed

Can we expect node to be there?

You can only expect that the process executing the file has a node binary, but not that it is in the PATH.

jbenet commented 8 years ago

@Dignifiedquire is the actual binary guaranteed to be there and executable? or do we have to do something like fork?

dignifiedquire commented 8 years ago

I don't think there is a reliable way to find the path of the executable from inside the running process, so the safest way would be to use child_process.fork I guess.

jbenet commented 8 years ago

@Dignifiedquire isn't process.argv[0] the node path?

dignifiedquire commented 8 years ago

It looks like process.argv[0] should be supported everywhere, I've tested it with forked child processes on OS X and that still works, will check on windows now to be sure that it's available as expected there as well.

dignifiedquire commented 8 years ago

@jbenet windows checks out as well:

screen shot 2015-10-12 at 09 42 24

jbenet commented 8 years ago

Ok great! let's do that

jbenet commented 8 years ago

Published as 1.0.4

jbenet commented 8 years ago

thanks @Dignifiedquire

dignifiedquire commented 8 years ago

Thanks @jbenet for the quick fix, will test it later