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

fix: shebangs #3922

Closed
wants to merge 3 commits into from
Closed

fix: shebangs #3922

wants to merge 3 commits into from

Conversation

@brandonkal
Copy link
Contributor

brandonkal commented Feb 8, 2020

The existing shebangs were not portable to Linux (which has no -S option).
This fix enables the shebangs to function in a more portable way.

@brandonkal

This comment has been minimized.

Copy link
Contributor Author

brandonkal commented Feb 8, 2020

Build failure is unrelated (Python format). Requesting a maintainer to trigger it again.

@brandonkal brandonkal requested a review from bartlomieju Feb 8, 2020
@piscisaureus

This comment has been minimized.

Copy link
Collaborator

piscisaureus commented Feb 8, 2020

@brandonkal I restarted CI but it failed at the same point, so I don't think it's unrelated. Can you run format.py locally?

@brandonkal

This comment has been minimized.

Copy link
Contributor Author

brandonkal commented Feb 8, 2020

My bad. The fact python was calling Prettier threw me off. It should pass now.

@brandonkal

This comment has been minimized.

Copy link
Contributor Author

brandonkal commented Feb 8, 2020

@piscisaureus Note that it is still preferrable to not have a semicolon there.
But at least this works in more places than before.

Some shells will print "./catj.ts: 2: ./catj.ts: //: Permission denied" before executing the script.
There is no good way to add a prettier-ignore there either because not all shells accept "//" comments.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Feb 8, 2020

CC @nayeemrmn I remember you were updating shebangs some time ago. Can you take a look?

cli/js/unit_test_runner.ts Outdated Show resolved Hide resolved
@nayeemrmn

This comment has been minimized.

Copy link
Contributor

nayeemrmn commented Feb 8, 2020

Yeah, the env -S solution is for GNU coreutils 8.30+ or something. But only the relatively newer versions of distros have this. The change here I believe is StackOverflow's solution for full compatibility. I probably should have done this in the first place.

@bartlomieju bartlomieju requested a review from ry Feb 9, 2020
#!/usr/bin/env -S deno run --reload --allow-run
#!/bin/sh
":" //; exec /usr/bin/env deno run --reload --allow-run "$0" "$@"
.charAt(0);

This comment has been minimized.

Copy link
@ry

ry Feb 13, 2020

Collaborator

I can't parse this.. what is happening here?

This comment has been minimized.

Copy link
@brandonkal

brandonkal Feb 14, 2020

Author Contributor
  1. The first line tells unix to parse as a shell script line by line.
  2. The : command which is quoted to be valid JavaScript is the "Do nothing beyond expanding arguments and performing redirections." Its only argument is // which is a valid path.
  3. Then we start a new shell statement after the ; inside the JavaScript comment.
  4. exec replaces the shell process with deno passing the desired deno arguments, the script file "$0" and any arguments the user provides "$@"
  5. The .charAt(0); line is required so that Prettier doesn't insert a semicolon after the ":" JavaScript statement. So instead JavaScript executes ":".charAt(0); which is basically a no-op.
@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Feb 13, 2020

Thanks for the patch but sorry this is too weird. We need another solution.

@ry ry closed this Feb 13, 2020
@brandonkal

This comment has been minimized.

Copy link
Contributor Author

brandonkal commented Feb 14, 2020

@ry It is weird but it is the only current solution that is portable especially as -S isn't universally available for the env command.

In most environments only one argument can be passed to a program by env.
Another solution is to introduce another deno flag -x https://perldoc.perl.org/perlrun.html#*-x*.

Then the shebang could look like this:

#!/usr/bin/env deno -x
// deno run --reload --allow-run "$0" "$@"
@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Feb 14, 2020

I'd rather change how we parse args so we can support all types of env

@brandonkal

This comment has been minimized.

Copy link
Contributor Author

brandonkal commented Feb 14, 2020

@ry So you would say the "-x" solution is good?

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Feb 14, 2020

@brandonkal anything is better than exposing the hack in this patch : )

@brandonkal

This comment has been minimized.

Copy link
Contributor Author

brandonkal commented Feb 14, 2020

Okay. I'll look to add -x if I find the spare cycles.

@brandonkal brandonkal deleted the brandonkal:fix-shebangs branch Feb 14, 2020
@nayeemrmn

This comment has been minimized.

Copy link
Contributor

nayeemrmn commented Feb 14, 2020

#!/usr/bin/env deno -x

Which platforms does this work on? The Linux limitation is one interpreter /usr/bin/env one arg to that interpreter "deno -x" and then the script name is the next arg.

I tried this and it calls /usr/bin/env "deno -x" <script>, which is wrong.

@brandonkal

This comment has been minimized.

Copy link
Contributor Author

brandonkal commented Feb 14, 2020

Oh yes of course. So it looks like this will remain unsolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.