Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

module: new switch -x; support multiple #! args #7007

Closed
wants to merge 22 commits into from

Conversation

smikes
Copy link

@smikes smikes commented Jan 30, 2014

module: new switch -x; support multiple #! args

Interpreters invoked via #! see only one
argument on some environments (Linux, Cygwin)

This pull request enables a script to pass multiple
arguments,such as v8 options, to node using a technique
developed for other scripting languages (perl and
ruby).

Detailed justification and analysis (TL;DR) in the attached
file doc/Minux-X-Switch-Proposal.md , link http://bit.ly/1bbkQ2X or
https://github.com/smikes/node/blob/minus-x-switch/doc/Minus-X-Switch-Proposal.md

This pull request DOES NOT make any changes to
the public API of module, which is a locked
subsystem.

lint clean, added unit tests for new function Module._removePrologue
and integration test for node -x invocation, tested on OSX,
Windows, Ubuntu AWS instance.

Interpreters invoked via #! only see one
argument on some environments (Linux, Cygwin)

This pull request enables a script to pass multiple
arguments,such as v8 options, to node using a technique
developed for other scripting languages (perl and
ruby).

Detailed justification and analysis in the attached
file doc/Minux-X-Switch-Proposal.md

This pull request *DOES NOT* make any changes to
the public API of modules, which is a locked
subsystem.
Factor the code that removes '#!' into a function,
Module._removePrologue.

Module._removePrologue removes only the first #! line unless
BOTH a) this is the main module (module.id === '.')
 AND b) the '-x' switch was passed on the command line.

In case both a) and b) are true, Module._removePrologue
removes lines until it finds a line which begins with "#!"
and contains the substring "node".
Detect '-x' switch on command line and set global
strip_until_shebang_node and property
process._stripUntilShebangNode
Add documentation of -x switch:

"-x  ignore text before #!node line"
Explicit 'true' return value omitted when refactoring.
Remove incorrect documentation referring to optional <dir>
arg to proposed -x switch.
@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit smikes/node@f48d166 has the following error(s):

  • Commit message must indicate the subsystem this commit changes

Commit smikes/node@adee60b has the following error(s):

  • Commit message must indicate the subsystem this commit changes

Commit smikes/node@6f26d0d has the following error(s):

  • Commit message must indicate the subsystem this commit changes

Commit smikes/node@0486637 has the following error(s):

  • Commit message must indicate the subsystem this commit changes

The following commiters were not found in the CLA:

  • smikes

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

Factor the code that removes '#!' into a function,
Module._removePrologue.

Module._removePrologue removes only the first #! line unless
BOTH a) this is the main module (module.id === '.')
 AND b) the '-x' switch was passed on the command line.

In case both a) and b) are true, Module._removePrologue
removes lines until it finds a line which begins with "#!"
and contains the substring "node".
Detect '-x' switch on command line and set global
strip_until_shebang_node and property
process._stripUntilShebangNode
Add documentation of -x switch:

"-x  ignore text before #!node line"
Explicit 'true' return value omitted when refactoring.
Remove incorrect documentation referring to optional <dir>
arg to proposed -x switch.
@smikes
Copy link
Author

smikes commented Jan 30, 2014

Signed CLA and edited commit messages to conform to CONTRIBUTING standards

Commits that update the Minus-X-Switch-Proposal.md assigned to module doc:

@litmit
Copy link

litmit commented Jan 31, 2014

IMHO it must be task for NPM and be based on package.json configuration, because #! is not portable across different OS.

@rlidwka
Copy link

rlidwka commented Jan 31, 2014

How would it help with a standard #!/usr/bin/env node header?

I don't believe it should be fixed in node, it's rather OS issue.

@smikes
Copy link
Author

smikes commented Feb 1, 2014

It's absolutely an OS issue, but it's one that has seen two patches rejected in the last ten years (at least in Linux). The standard workarounds are aliases, wrapper scripts, and the -x option to interpreters which allows a script to act as its own wrapper.

Right now #!/usr/bin/env node --noharmony works on OSX but not on Linux:

/usr/bin/env: node --noharmony: No such file or directory

This PR is no longer timely since #6999 has been deferred until after 0.12. As harmony changes come in, though, script authors will find it necessary to control the v8 execution environment and will run into this OS issue.

Adding a -x switch to the interpreter is one way of easing that transition. It is not a complete, once-for-all solution. It's a hack. But it's a hack that's familiar to anyone who has dealt with cross-platform scripting before.

@rlidwka
Copy link

rlidwka commented Feb 1, 2014

If we're talking about hacks, this one with bash scripting seems better, because it doesn't rely on node behaviour.

It's absolutely an OS issue, but it's one that has seen two patches rejected in the last ten years (at least in Linux).

Can you show some links? I'd really want to know why.

@smikes
Copy link
Author

smikes commented Feb 1, 2014

Sure. It's all referenced in the TL;DR document linked above.

but here's the one from 2004:
Original Patch submitted to Linux Kernel Mailing list (2004), rejected https://lkml.org/lkml/2004/2/16/74

And here's the thread from 2008: https://lkml.org/lkml/2008/12/3/175

I hadn't seen that bash hack-around. What happens on Windows?

@litmit
Copy link

litmit commented Feb 1, 2014

How this issue help to manage scripts which requires the different options under different versions of Node?

For example some application require Harmony collections. For Node 0.10.x we need --harmony_collections option.
As proposed in #6999 future version of Node may enable --harmony by default. But this application not compatible with some others harmony features. Therefore for Node 0.12.x we need --no-harmony --harmony_collections options.
How such situation will be handled?

I propose use NPM for this. We need to extend engines property in package.json:

{ "engines" : 
    { "node" : ">=0.10.0 <0.12" , "options":["harmony_collections"]} ,
    { "node" : ">=0.12.0" , "options":["no-harmony","harmony_collections"]} ,
}

During install npm should automatically generate script with valid options for run the application, depending on version of Node and host OS.

@rlidwka
Copy link

rlidwka commented Feb 1, 2014

Maybe forget about command line switches and set v8 options using setflags ?

@litmit
Copy link

litmit commented Feb 1, 2014

I talks about more high level concept then command line switches.
NPM may use package.json options for generate scripts (and options become the command line switches)
module.require may also use package.json options and automatically configure loaded module with appropriate V8 flags.

Module is a black box. When I do require('somemodule') i not need to known that it use some harmony features. harmonyRequire('somemodule') is abstraction leak.

@smikes
Copy link
Author

smikes commented Feb 2, 2014

@rlidwka From setflags: "Also, I do not endorse this as a way to do business, but as a potential stop gap to enable a crucial feature."

I think you're right, the ":" //#; exec hack is a better solution. That's not available to perl or ruby, because they both support # as a comment character. The only downside to using the ":" //; hack is that it doesn't lint clean, but that's a lot less trouble than introducing a new command-line switch, etc.

@litmit In the longer term, rather than specifying flags for engines alone, we may need to specify flags for engines and binaries in package.json.

That's very do-able since npm generates a wrapper script using https://github.com/ForbesLindesay/cmd-shim . Right now on Unix-like systems cmd-shim just creates a symbolic link, but if node_options is set either on the engine or on the binary, it could generate a wrapper script which passes the necessary command options.

Regarding require vs. harmonyRequire, that's going to be a problem for some time. I agree that modules should be black boxes but in practice a module can only use harmony features that are enabled by default; a single module can't dictate to every other program that wants to use it that certain harmony features must be turned on. Then we'd be back in DLL hell, only without version numbers.

@rlidwka
Copy link

rlidwka commented Feb 2, 2014

Also, I do not endorse this as a way to do business, but as a potential stop gap to enable a crucial feature.

So what? People are misusing technologies all the time, for example domains are used to catch errors, and JSON is used as a config file format. So why not use setflags, it doesn't make life significantly uglier. :P

The only downside to using the ":" //; hack is that it doesn't lint clean

If a valid and desirable code doesn't lint clean, it means that your linter is misconfigured. So it is an issue with your linter tool.

There is another downside however. Some editors and code highlighters detect file format with shebang, and #!/bin/sh highlights the entire file as a shell script. Not a big issue though.

@smikes
Copy link
Author

smikes commented Feb 2, 2014

Then those editors and code highlighters are misconfigured. ;-P

@smikes smikes closed this Feb 2, 2014
@rlidwka
Copy link

rlidwka commented Feb 2, 2014

Then those editors and code highlighters are misconfigured. ;-P

Why is that? Shell scripts usually have no extension, so getting info from shebang seems reasonable enough.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants