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

Broken running forked livescript #916

Closed
fansgit opened this Issue Jul 31, 2016 · 2 comments

Comments

Projects
None yet
3 participants
@fansgit

fansgit commented Jul 31, 2016

lsc 1.5.0 would be broken when running the following directly.

require! cluster

if cluster.is-master
  console.log \master
  cluster.fork!
else
  console.log \worker
  process.disconnect!
require! cluster
       ^
SyntaxError: Unexpected token !
@bumxu

This comment has been minimized.

bumxu commented Jul 31, 2016

This has been introduced by #562.
process.execPath now points to Node, so fork is executed as JS.

I can't think a clear solution, sometimes we need execPath pointing node, sometimes pointing lsc.

😲

@rhendric

This comment has been minimized.

Collaborator

rhendric commented Mar 29, 2017

As I see it, the problem here is not that execPath needs to point to lsc; it's that lsc shouldn't be messing around with process.argv. If I remove the code from command.ls and node.ls that does so, this example works fine, and I bet the shelljs functionality fixed in #562 also continues to work.

The node.ls behavior was introduced in commit 9090c59, way back in the coco days. The motivation stated in that commit was to make relative require work with --eval and --stdin. Removing the process.argv assignment from that code doesn't seem to break relative require with --eval or --stdin when I tested it.

The command.ls behavior was introduced in response to #569, in commit c959444. This is partly a case of piling a hack on top of the previous hack, and partly a case of asking for the wrong thing: the reporter asked for [ 'node', '/home/derk/test.ls', 'hello' ] but should have asked for [ 'node', 'lsc', '/home/derk/test.ls', 'hello' ] (or [ 'lsc', '/home/derk/test.ls', 'hello' ], but we know thanks to #562 that this has undesirable effects).

Basically, I think both of those hacks should be removed and process.argv should be left as node intended.

rhendric added a commit to rhendric/LiveScript that referenced this issue Mar 29, 2017

remove process.argv mutation
Fix gkz#916. See that issue for commentary.

@rhendric rhendric closed this in #959 May 8, 2017

rhendric added a commit that referenced this issue May 8, 2017

remove process.argv mutation
Fix #916. See that issue for commentary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment