[CS2] Add #! support for executable scripts on Linux. #3946

cosmicexplorer · 2015-04-16T08:24:48Z

Pass arguments to executable script unchanged if using "#!/usr/bin/env
coffee". (Previously, "./test.coffee -abck" would be turned into "-a -b -c -k",
for example.)

Fixes #1752.

Pass arguments to executable script unchanged if using "#!/usr/bin/env coffee". (Previously, "./test.coffee -abck" would be turned into "-a -b -c -k", for example.) Fixes jashkenas#1752.

dbushong · 2016-02-23T22:24:28Z

👍 to this. The current behavior also has it so that you can't pass -- through to your coffee command-line program:

% echo 'console.log process.argv' > foo.coffee
% coffee -c foo.coffee
% coffee foo.coffee -a -- -b
[ 'coffee', '/Users/dbushong/foo.coffee', '-a', '-b' ]
% node foo.js -a -- -b
['/path/to/node', '/Users/dbushong/foo.js', '-a', '--', '-b']

While it works correctly if you say coffee foo.coffee -- -a -- -b, that's not on option on the linux shebang line.

GeoffreyBooth · 2017-04-18T17:55:35Z

src/optparse.coffee

+    # systems, which do not parse the '--' argument in the first line correctly.
+    if (args.indexOf '--' is -1) and
+       (args.length > 0) and
+       (isCoffee args[0])


I would rewrite this is as

if args.length isnt 0 and '--' in args and isCoffee(args[0])

GeoffreyBooth · 2017-04-18T17:56:00Z

Is there any chance that this could be a breaking change? If so please target the 2 branch.

cosmicexplorer · 2017-04-19T14:23:59Z

I think so, for linux hosts; the situation I described at the top won't matter if the script follows standard command line conventions (where -abck is equivalent to -a -b -c -k), but it'll mysteriously break for users who weren't aware of this. I can fix this to work off the 2 branch sometime this week.

GeoffreyBooth · 2017-04-19T14:28:02Z

How would it break? Would -a -b -c -k no longer be possible?

cosmicexplorer · 2017-04-19T16:33:15Z

Sorry, the other way around; -abck is always split to -a -b -c -k (not just these letters, others too) unless you use a workaround. There is no way to make a standalone script executable on Linux using an env shebang line so that it can accept multiple arguments; some discussion is here.

Invoking a script either like ./script.coffee -- -abck or coffee script.coffee -- -abck both have an argv of [ 'coffee', '/path/to/script.coffee', '-abck'], so it is possible, but requires a wrapper script. This is a problem because these arguments are consumed by the script, but still processed by the coffeescript executable's own argument parsing. On non-linux boxes you can have the line #!/usr/bin/env coffee --, but in linux that argv is parsed by the kernel as a two-element array ['/usr/bin/env', 'coffee --'], which obviously fails unless you add a symlink at "/usr/bin/coffee --" or something else ridiculous.

GeoffreyBooth · 2017-04-19T16:50:34Z

How will this affect traditional users running coffee from the command line?

cosmicexplorer · 2017-04-19T23:05:27Z

isCoffee checks if the script to be evaluated was invoked with a shebang (as the first argument) or as an argument to the coffee executable. Anyone using a command line like coffee [args...] will be unaffected. Users who may be affected currently invoke a script as argv[0], e.g. ./test.coffee [args...], where test.coffee has a shebang line and is executable. If test.coffee has:

#!<SHEBANG LINE>

console.log process.argv

then there are about two and a half cases it differs:

Scripts with the shebang #!/usr/bin/env coffee -- (which does not work on Linux) and are invoked as ./test.coffee -abck will see a -- after the script path: [ 'coffee', '/path/to/test.coffee', '--', '-abck' ] (where before there was no --). Users who invoke the script with shebang #!/usr/bin/env coffee -- (not on Linux) as ./test.coffee -- -abck intentionally to pass -- as an argument to their own script will see the -- argument disappear: [ 'coffee', '/home/cosmicexplorer/test.coffee', '-abck' ].
Users on any platform (probably on Linux because it was the only way it would work, although not necessarily) who had used the shebang #!/usr/bin/env coffee or #!/path/to/some/executable/coffee and invoked it as ./test.coffee -- -abck to avoid the argument splitting into -a -b -c -k will see an extra --, just as above: [ 'coffee', '/path/to/test.coffee', '--', '-abck' ], where before there was none.

The solution above won't fix this, but we can detect the case where the user invokes the script with -- in the shebang line and not in the script invocation. I'll push my (small) change in a second which does this.

GeoffreyBooth · 2017-04-19T23:36:28Z

We should add some tests around this. I think it’s testable within our current framework, via stuff like execSync('./imports/cli_test.coffee -a -b -c') or similar. There should also be tests for more traditional commands, like execSync('coffee -v'), so that the current behavior is covered against regressions. The tests should make sure to use the coffee binary in the repo, not whatever global coffee might be in the user’s $PATH.

clean up parsing code and in the process fix oustanding bug where coffeescript modified arguments meant for an executable script

cosmicexplorer · 2017-04-20T01:35:26Z

I think the previous option parsing code was convoluted and made it too easy to make mistakes like the -- processing (which this commit also fixes), especially for platform-specific stuff. With respect to the (possible) breaking changes I mentioned in my previous post, invoking the coffee interpreter is still the exact same. When invoking a shebang script ./test.coffee [args...]:

If the script begins with a shebang line of #!/usr/bin/env coffee -- or #!/usr/bin/env coffee (or just a full path to the executable), it will not modify any arguments passed on the command line to the script. This part breaks nothing and makes scripts work on Linux.
There may be users (most probably on Linux, if they exist) who used a shebang script of #!/usr/bin/env coffee and passed -- as an argument to their script, like ./test.coffee -- [intended-args...] to work around the previous behavior (which would have only passed intended-args... to their script). These users will see an "extra" --, which is a breaking change, but I think easily spotted.

I can absolutely add some tests. I do not have time today, but will try to do so tomorrow. Let me know if there are any style changes I can make, I tried to follow the rest of the code as much as possible.

GeoffreyBooth · 2017-04-20T03:55:51Z

src/optparse.coffee

+    state =
+      argsLeft: args[..]
+      options: {}
+    while (arg = state.argsLeft.shift())?


I’ve been trying to avoid assignments inside conditionals. Can we just use for arg in args?

Now that I think about it, I think we can just do a single pass over the arguments beforehand to 1. group arguments with values (-o [dir]) 2. explode combined options (e.g. -abck) 3. identify the first non-optional argument, then use a for. I'll do that.

GeoffreyBooth · 2017-04-20T03:59:04Z

src/optparse.coffee

+        normalized = "-#{multiArg}" for multiArg in multiMatch[1].split ''
+        state.argsLeft.unshift(normalized...)
+      else
+        # the CS option parser is a little odd; options after the first


The comments become the annotated source, so please use sentence case (capitalize first word, use periods).

Got it, will fix. The first two lines are actually from the original source, so I wasn't sure whether I should change them.

Yeah. Never too late to fix things 😄

GeoffreyBooth · 2017-04-20T04:01:01Z

src/optparse.coffee

+    if arg in [rule.shortFlag, rule.longFlag]
+      if rule.hasArgument
+        value = state.argsLeft.shift()
+        if not value?


Got it, will change.

GeoffreyBooth · 2017-04-20T04:01:28Z

src/optparse.coffee

+      else
+        value = true
+
+      addArgument(rule, state.options, value)


Avoid parentheses when possible.

Got it, will fix.

GeoffreyBooth · 2017-04-20T04:02:01Z

src/optparse.coffee

+        value = true
+
+      addArgument(rule, state.options, value)
+      return


Is there a reason to avoid the implicit return?

If not, it would eventually leave the for loop and hit the throw statement. This was dealt with previously by keeping a boolean testing whether or not the rule was matched; I thought this was cleaner here since we don't then loop over the rest of the rules if we successfully match one.

GeoffreyBooth · 2017-04-20T04:31:30Z

Do you mind please submitting the “current state” tests of the CLI as it works now as a separate PR? So that we can see that they work before trying them against your refactor.

cosmicexplorer · 2017-04-20T10:25:40Z

Yes! I'll make a separate PR for those sometime tomorrow. Thanks for your responsiveness.

GeoffreyBooth

Sorry to give such minor notes. I would make the changes myself but I lack access to your branch.

GeoffreyBooth · 2017-06-30T01:25:45Z

test/importing/shebang-extra-args.coffee

@@ -0,0 +1,3 @@
+#!/usr/bin/env coffee --


The convention in this repo is to use underscores in filenames.

I have changed all the files I have added to use underscores. The file test/argument-parsing.coffee was one I made in the previous PR to test v1's command-line argument parsing, and I have changed that one to use underscores too.

GeoffreyBooth · 2017-06-30T01:26:18Z

documentation/sections/breaking_changes_argument_parsing_and_shebang_lines.md

+If called without options, `coffee` will run your script.
+
+
+```


Why the blank lines before and after the text?

I actually screwed up the help text (see the most recent commit -- needed to change the help method to use @rules.ruleList instead of just @rules). I made that fix, changed this text, and have also added tests in test/option_parser.coffee in the most recent commit to ensure the help text will always display the banner and the flags.

GeoffreyBooth · 2017-06-30T01:28:11Z

documentation/sections/breaking_changes_argument_parsing_and_shebang_lines.md

+4
+```
+
+Due to a bug in the argument parsing of previous CoffeeScript versions, this used to fail when trying to pass arguments to the script. Some users on OSX worked around the problem by using `#!/usr/bin/env coffee --` at the top of the file instead. However, that won't work on Linux, which cannot parse shebang lines with more than a single argument. While these scripts will still run on OSX, CoffeeScript will now display a warning before compiling or evaluating files that begin with a too-long shebang line:


It’s “OS X”. Also the documentation uses curly quotes, e.g. “won’t”.

Fixed both of those.

also add tests for help text

GeoffreyBooth

Also please merge in develop and resolve conflicts.

GeoffreyBooth · 2017-07-06T01:53:40Z

documentation/sections/breaking_changes_argument_parsing_and_shebang_lines.md

+  -l, --literate     treat stdio as literate style coffeescript
+  -t, --tokens       print out the tokens that the lexer/rewriter produce
+  -v, --version      display the version number
+  -w, --watch        watch scripts for changes and rerun commands


This is already documented in http://coffeescript.org/v2/#usage. The Breaking Changes section should only discuss breaking changes from 1.x.

That makes sense. I have noted only the changed usage line now and left an ... to denote the rest of the help text.

GeoffreyBooth · 2017-07-06T01:54:15Z

documentation/sections/breaking_changes_argument_parsing_and_shebang_lines.md

@@ -45,7 +64,7 @@ console.log x
 4
 ```

-Due to a bug in the argument parsing of previous CoffeeScript versions, this used to fail when trying to pass arguments to the script. Some users on OSX worked around the problem by using `#!/usr/bin/env coffee --` at the top of the file instead. However, that won't work on Linux, which cannot parse shebang lines with more than a single argument. While these scripts will still run on OSX, CoffeeScript will now display a warning before compiling or evaluating files that begin with a too-long shebang line:
+Due to a bug in the argument parsing of previous CoffeeScript versions, this used to fail when trying to pass arguments to the script. Some users on OS X worked around the problem by using `#!/usr/bin/env coffee --` at the top of the file instead. However, that won’t work on Linux, which cannot parse shebang lines with more than a single argument. While these scripts will still run on OSX, CoffeeScript will now display a warning before compiling or evaluating files that begin with a too-long shebang line:


Another “OS X”.

GeoffreyBooth · 2017-07-06T02:00:41Z

test/option_parser.coffee

+  ok bannerHelp.match bannerPattern
+  for flag in flags
+    linePat = flagPattern flag
+    ok bannerHelp.match linePat


This tests that the option parser correctly outputs a banner text string, but not necessarily that coffee -h actually outputs the correct help text. For that, you need to actually test opt.help() against the help text, which should be written here in the test. I know it doesn’t feel DRY to repeat a string that is elsewhere in the codebase, but if that string changes in the main code, we want this test to fail—either to inform us that we unexpectedly changed the help text, or to remind us to update the test, too, so that we preserve this check for the future. The same applies to the list of switches.

Ok, that makes sense. I'll make that change.

cosmicexplorer · 2017-07-07T22:58:40Z

I have responded to your comments and merged in the 2 branch (the conflict was with the file argument-parsing.md, which I renamed with an underscore instead).
The tests for the help text now check the exact help text string. I have also added tests in option_parser.coffee which checks that the correct help text is output by the OptionParser class, not just the instance of it used in the coffee command.
I have slightly edited breaking_changes_argument_parsing_and_shebang_lines.md to make it more clear.

Let me know if there are any issues with these changes.

GeoffreyBooth

If you’d like to give me access to your branch, I could fix a bunch of these tiny issues.

GeoffreyBooth · 2017-07-07T23:05:00Z

test/option_parser.coffee

+    '  -o, --optional     desc optional'
+    '  -l, --list         desc list'
+    ''
+  ].join('\n')


This could just be a ''' string.

Possibly, except that ''' strings don't allow leading spaces unless one of the lines (e.g. the banner text) is not indented, so I don't think this would work as a ''' string.

GeoffreyBooth · 2017-07-07T23:05:07Z

test/option_parser.coffee

+test "outputs expected help text", ->
+  expectedBanner = '''
+
+banner text


Why is this here?

This tests that the OptionParser class outputs the kind of help text that we expect, and not just the OptionParser instance that is used in command.coffee. It also tests the output without a banner.

GeoffreyBooth · 2017-07-07T23:05:31Z

documentation/sections/breaking_changes_argument_parsing_and_shebang_lines.md

-Due to a bug in the argument parsing of previous CoffeeScript versions, this used to fail when trying to pass arguments to the script. Some users on OS X worked around the problem by using `#!/usr/bin/env coffee --` at the top of the file instead. However, that won’t work on Linux, which cannot parse shebang lines with more than a single argument. While these scripts will still run on OSX, CoffeeScript will now display a warning before compiling or evaluating files that begin with a too-long shebang line:
+##### Incorrect Example
+
+Due to a bug in the argument parsing of previous CoffeeScript versions, this used to fail when trying to pass arguments to the script. Some users on OSX worked around the problem by using `#!/usr/bin/env coffee --` at the top of the file instead. However, that won’t work on Linux, which cannot parse shebang lines with more than a single argument. While these scripts will still run on OSX, CoffeeScript will now display a warning before compiling or evaluating files that begin with a too-long shebang line:


I don't understand what the issue is -- should I add quotes around OSX? The space between OS and X was removed everywhere.

cosmicexplorer · 2017-07-08T00:00:37Z

I have added you as a collaborator to my branch -- let me know if you have push access.

…sidebar and body; add new output

GeoffreyBooth · 2017-07-09T05:19:34Z

Okay, I think we’re good now. I shortened the docs considerably, can you let me know if I took out anything important? We try to keep the docs as concise as possible. The generated output is here:
http://rawgit.com/cosmicexplorer/coffeescript/master/docs/v2/#breaking-changes-argument-parsing-and-shebang-lines

Anyone else have any final notes before this gets merged in?

cosmicexplorer · 2017-07-09T06:37:35Z

src/command.coffee

@@ -25,7 +25,7 @@ hidden = (file) -> /^\.|~$/.test file

 # The help banner that is printed in conjunction with `-h`/`--help`.
 BANNER = '''
-  Usage: coffee [options] [--] path/to/script.coffee [args]


I don't like removing [--], the new version does not say anything about how -- is parsed as an argument. gawk --help uses [--] in a very similar way.

cosmicexplorer · 2017-07-09T06:38:07Z

test/argument_parsing.coffee

@@ -115,7 +115,7 @@ test "throw on invalid options", ->
 test "has expected help text", ->
  ok optionParser.help() is '''

-Usage: coffee [options] [--] path/to/script.coffee [args]


As in src/command.coffee.

cosmicexplorer · 2017-07-09T06:51:13Z

I really like the changes. My only concern is in the usage text, as noted inline.

GeoffreyBooth · 2017-07-09T07:30:20Z

Why mention -- in the help text if we don't want it to be used? Does it serve some purpose?

cosmicexplorer · 2017-07-10T02:41:48Z

After thinking about it a bit, I think the main reason I wanted the [--] to stay is because awk's usage text uses [--] and it seems more precise. I completely agree that since we don't want it to be used (even though the command can handle it), it shouldn't be mentioned.

GeoffreyBooth · 2017-07-10T02:56:39Z

I think it’s confusing to include the reference to it if we don’t define what it does (especially if the only thing it “does” is print a warning). If it did anything useful, then sure, let’s include it and describe its function like the other flags.

@lydell or anyone else, any final notes? This is an old PR, thank you @cosmicexplorer for sticking with us for so long!

cosmicexplorer · 2017-07-10T03:31:51Z

I completely 100% agree with that. Let me know if anything weird happens with these changes -- I feel comfortable after all the test cases, but I'm still wary. Glad this change finally saw light, turned out to be much, much harder than I expected.

jashkenas/coffeescript#3946 (comment)

Add #! support for executable scripts on Linux.

6c48af3

Pass arguments to executable script unchanged if using "#!/usr/bin/env coffee". (Previously, "./test.coffee -abck" would be turned into "-a -b -c -k", for example.) Fixes jashkenas#1752.

GeoffreyBooth reviewed Apr 18, 2017

View reviewed changes

cosmicexplorer force-pushed the master branch 3 times, most recently from 74f9a2e to 65f1e73 Compare April 20, 2017 01:23

refactor option parsing

221dfc4

clean up parsing code and in the process fix oustanding bug where coffeescript modified arguments meant for an executable script

cosmicexplorer force-pushed the master branch from 65f1e73 to 221dfc4 Compare April 20, 2017 01:23

GeoffreyBooth requested changes Apr 20, 2017

View reviewed changes

cosmicexplorer force-pushed the master branch 2 times, most recently from baaf924 to 124d228 Compare April 20, 2017 10:25

cosmicexplorer force-pushed the master branch 6 times, most recently from e2d7fe7 to 5f3edbe Compare April 20, 2017 11:17

address comments

988f2af

cosmicexplorer force-pushed the master branch from 5f3edbe to 988f2af Compare April 20, 2017 11:23

GeoffreyBooth changed the title ~~Add #! support for executable scripts on Linux.~~ [WIP] Add #! support for executable scripts on Linux. Apr 22, 2017

GeoffreyBooth requested changes Jun 30, 2017

View reviewed changes

GeoffreyBooth mentioned this pull request Jun 30, 2017

2.0.0-beta3 #4594

Merged

respond to review comments

b9291ae

also add tests for help text

GeoffreyBooth mentioned this pull request Jun 30, 2017

CS2 Discussion: Project: Progress as of June 2017: 2.0.0-beta3! coffeescript6/discuss#86

Closed

GeoffreyBooth requested changes Jul 6, 2017

View reviewed changes

Danny McClanahan added 3 commits July 7, 2017 17:48

respond to review comments

f306859

Merge branch '2' of github.com:jashkenas/coffeescript

4093574

fix example output

4856fd6

GeoffreyBooth requested changes Jul 7, 2017

View reviewed changes

GeoffreyBooth added 2 commits July 8, 2017 19:30

Rewrite argument parsing documentation to be more concise; add it to …

f7e8c2b

…sidebar and body; add new output

Don’t mention deprecated syntax; clean up variable names

6fb80c1

GeoffreyBooth approved these changes Jul 9, 2017

View reviewed changes

cosmicexplorer commented Jul 9, 2017

View reviewed changes

GeoffreyBooth merged commit 4e57ca6 into jashkenas:2 Jul 19, 2017

GeoffreyBooth mentioned this pull request Aug 4, 2017

CS2 Discussion: Project: Progress as of August 2017: 2.0.0-beta4! coffeescript6/discuss#87

Closed

GeoffreyBooth mentioned this pull request Sep 29, 2017

Tests fail in Windows #4727

Closed

This was referenced Feb 19, 2018

CS2 Discussion: Project: Progress as of June 2017: 2.0.0-beta3! #4979

Closed

CS2 Discussion: Project: Progress as of August 2017: 2.0.0-beta4! #4980

Closed

cosmicexplorer mentioned this pull request Oct 25, 2018

Fall back to most recent known osx version for bootstrap binaries pantsbuild/pants#6681

Merged

JoshCheek added a commit to JoshCheek/language-sampler-for-fullpath that referenced this pull request Mar 12, 2020

*sigh* invoke in a wrapper script

e9a65a9

jashkenas/coffeescript#3946 (comment)

cosmicexplorer mentioned this pull request Oct 31, 2024

Discussion: TypeScript Output #5307

Open

		If called without options, `coffee` will run your script.


		```

[CS2] Add #! support for executable scripts on Linux. #3946

[CS2] Add #! support for executable scripts on Linux. #3946

Conversation

cosmicexplorer commented Apr 16, 2015

dbushong commented Feb 23, 2016

Choose a reason for hiding this comment

GeoffreyBooth commented Apr 18, 2017

cosmicexplorer commented Apr 19, 2017

GeoffreyBooth commented Apr 19, 2017

cosmicexplorer commented Apr 19, 2017

GeoffreyBooth commented Apr 19, 2017

cosmicexplorer commented Apr 19, 2017

GeoffreyBooth commented Apr 19, 2017

cosmicexplorer commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeoffreyBooth commented Apr 20, 2017

cosmicexplorer commented Apr 20, 2017

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicexplorer Jun 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicexplorer Jun 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicexplorer commented Jul 7, 2017

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicexplorer commented Jul 8, 2017

GeoffreyBooth commented Jul 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicexplorer commented Jul 9, 2017

GeoffreyBooth commented Jul 9, 2017

cosmicexplorer commented Jul 10, 2017

GeoffreyBooth commented Jul 10, 2017

cosmicexplorer commented Jul 10, 2017

cosmicexplorer Jun 30, 2017 •

edited

Loading

cosmicexplorer Jun 30, 2017 •

edited

Loading