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

feat(flags): script arguments come after '--' #3621

Merged
merged 14 commits into from
Jan 8, 2020
Merged

Conversation

ry
Copy link
Member

@ry ry commented Jan 8, 2020

Fixes #3469

@bartlomieju bartlomieju mentioned this pull request Jan 8, 2020
@ry ry requested a review from bartlomieju January 8, 2020 18:15
@ry ry merged commit 2d5457d into denoland:master Jan 8, 2020
@ry ry deleted the hyphen_hyphen branch January 8, 2020 20:01
@Fenzland
Copy link
Contributor

That's prevent us to build a real command application. Suppose we implement git with deno, we have to use it like these:

git -- --version
git -- add somefile
git -- commit -m 'add somefile'

A new user query helps with git --help will get help of deno.

@lucacasonato
Copy link
Member

That's prevent us to build a real command application. Suppose we implement git with deno, we have to use it like these:

git -- --version
git -- add somefile
git -- commit -m 'add somefile'

A new user query helps with git --help will get help of deno.

Not per se - instead of aliasing git to deno git.ts, alias it to deno git.ts -- and your problem is solved.

@Fenzland
Copy link
Contributor

Not per se - instead of aliasing git to deno git.ts, alias it to deno git.ts -- and your problem is solved.

An alias is not a real command, it only works in current shell, when you switch to another shell, or call with other processes such as Deno.run(), it not work. And it's bellyful to let your users to set alias after download your application.

A reasonable way is write an executable file named as command, with content like this:

#!/usr/bin/env -S deno --allow-something

your user download it and put it into /usr/bin or other place in the PATH.
we cannot add -- after the script name by shebang.

@chibat chibat mentioned this pull request Jan 15, 2020
@ry
Copy link
Member Author

ry commented Jan 15, 2020

@Fenzland This works for me

> cat foo.js
#!/usr/bin/env deno --allow-read --

let f = await Deno.open("/etc/passwd");
console.log("f", f);
console.log("args", Deno.args);
> ./foo.js
f File { rid: 3 }
args [ "asdf", "asasdf" ]
>

@ry
Copy link
Member Author

ry commented Jan 15, 2020

That was on OSX. When I try it on linux I do indeed get an error. Hrm.

Is the issue that env tries to interpret the "--" ?

@nayeemrmn
Copy link
Collaborator

@ry You need the -S after env on Linux. But yes @Fenzland the above works due to the fact that if there are missing unnamed args (like the script name) before the --, they are consumed from after the --. So the following syntax is valid: deno <deno flags> -- <script name> <script args>.

The unnamed arg behaviour which allows this isn't really intuitive and I believe #3011 (wrongly closed 😅) was actually reporting against it as a bug. Confused yet?

@ry
Copy link
Member Author

ry commented Jan 15, 2020

@nayeemrmn -S doesn't work with the GNU coreutil's env that I have.

ry added a commit to ry/deno that referenced this pull request Jan 15, 2020
Due to complaints about ergonomics and because it breaks shebang on
linux.

This reverts commit 2d5457d.
@nayeemrmn
Copy link
Collaborator

@ry Yeah, 8.30+ as I once wrote here denoland/deno_std#545. (It actually never worked with mine either :P) Nevertheless it's the only solution on Linux going forward.

@dubiousjim
Copy link
Contributor

The Linux kernel handles shebangs in a way that's unfriendly to passing more than one explicit argument (the remainder of the line gets passed to the first command as a single argument, together with the name of the file as a second). I guess the Gnu version of env (after a certain version, someone says 8.30+) reparses the shebang line when invoked with -S. Here's a workaround for users who are using Linux kernel and don't have that variant/version of env installed:

Instead of starting your script with:

#!/usr/bin/env deno run --allow-read

(or possibly with a -- appended...) Start it instead with these two lines:

#!/bin/sh
'exec' //"$(type -P deno)" run --allow-read "$0" "$@"; exit 1

What's happening? The first line says to execute the rest of the file as a shell script. When doing so, it will never go further than the second line, so the rest of the file can be JS or TypeScript. When the file is run by deno, the first line will be ignored as a shebang, and the second line (and rest of the file) gets interpreted as JS/TS. So the second line has to be a "polyglot", that is has to make sense both when interpreted as shell and when interpreted as JS/TS.

It makes sense when interpreted as JS/TS because it's just a string 'exec' followed by a comment.

It makes sense when interpreted as shell because the quotes around exec are stripped, and then the rest of the line up to the ; are read as arguments to exec. We include the ; exit 1 at the end in case deno wasn't found or otherwise couldn't be exec'd. We want the shell script to fail rather than try to interpret the rest of the file.

The arguments to exec are:

First, the path to deno (using type -P deno or command -v deno find deno wherever it is in your path, same as running /usr/bin/env deno). We rely on the fact that prefixing the path with two extra //s at the start shouldn't change its meaning. (A different strategy I've seen is to instead start this line with 'true' //; exec ....)

Next we pass arguments run --allow-read to deno. Then "$0" is the name of the script file being executed. The "$@" add in any arguments specified when invoking the script file.

Depending on whether the feature being discussed here is included or reverted (as it is when I write this comment), you might need to insert a -- before the "$@".

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flag order error
6 participants