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: installer #489

Merged
merged 28 commits into from
Jun 14, 2019
Merged

feat: installer #489

merged 28 commits into from
Jun 14, 2019

Conversation

bartlomieju
Copy link
Member

Closes #471

This PR builds on #488 by @syumai

I've managed to remove call to wget

@syumai syumai mentioned this pull request Jun 13, 2019
2 tasks
@bartlomieju
Copy link
Member Author

bartlomieju commented Jun 13, 2019

Current status:

  • ask to overwrite already existing script
  • download script to sniff for shebang
  • sniff shebang and display prompt to use shebang's permissions
  • display info about adding directory to PATH
  • handle Windows - gonna need help with this one I have no Windows machine
  • check if ~/.deno/bin is in PATH and skip info about adding it to PATH
  • use DENO_DIR instead of `~/.deno/bin - not sure about this one
  • allow installing local modules
  • add uninstall command
  • add upgrade command - not sure about this one
  • somehow allow to pass --reload flag to script
  • handle situation with both shebang and passed flags
20:07 $ deno -A ./installer/mod.ts https://deno.land/std/http/file_server.ts
[1/1] Compiling file:https:///Users/biwanczuk/dev/deno_std/installer/mod.ts
⚠️  file_server is already installed, do you want to overwrite it? [yN]
y

Downloading: https://deno.land/std/http/file_server.ts

ℹ️  Detected shebang:

   #!/usr/bin/env deno --allow-net

   Requested permissions:

	--allow-net

⚠️  Grant? [yN]
y

✅  Successfully installed file_server.

ℹ️  Add ~/.deno/bin to PATH
   echo 'export PATH="$HOME/.deno/bin:$PATH"' >> ~/.bashrc # change this to your shell

@ry
Copy link
Member

ry commented Jun 13, 2019

ℹ️  Detected shebang:

   #!/usr/bin/env deno --allow-net

   Requested permissions:

	--allow-net

⚠️  Grant? [yN]
y

How does a program request permissions?

@bartlomieju
Copy link
Member Author

How does an installer request permissions?

Not sure what you mean

@ry
Copy link
Member

ry commented Jun 13, 2019

I mean if a user installs something like this

deno install --allow-net https://foo.bar/blah.ts

Then there is no prompt necessary because the user has already explicitly given permission.

So I don't understand when Requested permissions: --allow-net ⚠️ Grant? [yN] is displayed?

@ry
Copy link
Member

ry commented Jun 13, 2019

Ooh it's based on the shebang?

Can we omit this feature for V1 ? shebang is quite unix specific, and it seems strange that it would have meaning on windows.

@bartlomieju
Copy link
Member Author

Ooh it's based on the shebang?

Yes, @syumai implemented it.

Can we omit this feature for V1 ? shebang is quite unix specific, and it seems strange that it would have meaning on windows.

Sure, I wasn't sure about it anyway.

@bartlomieju
Copy link
Member Author

@ry this should be ready for another review - I haven't tested it on Windows

@bartlomieju
Copy link
Member Author

bartlomieju commented Jun 14, 2019

Okay, there's one more important feature to add:

deno_install file_server https://deno.land/std/http/file_server.ts --allow-write --allow-net

First argument (file_server) denotes how installed script should be named - currently it is done automatically by stripping name from installed script. I'll update that shortly

$ deno -A ./installer/mod.ts
> deno installer
  Install remote or local script as executables.

USAGE:
  deno https://deno.land/std/installer/mod.ts EXE_NAME SCRIPT_URL [FLAGS...]

ARGS:
  EXE_NAME  Name for executable
  SCRIPT_URL  Local or remote URL of script to install
  [FLAGS...]  List of flags for script, both Deno permission and script specific flag can be used.

installer/README.md Outdated Show resolved Hide resolved
installer/README.md Outdated Show resolved Hide resolved
EXE_NAME Name for executable
SCRIPT_URL Local or remote URL of script to install
[FLAGS...] List of flags for script, both Deno permission and script specific flag can be used.
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems when I run it with no arguments, there's no error message or help text:

~/src/deno_std> deno -A installer/deno_installer.ts
[1/1] Compiling file:https:///Users/rld/src/deno_std/installer/deno_installer.ts
~/src/deno_std>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, can we just drop installer/deno_installer.ts and leave installer/mod.ts?

Removed installer/deno_installer.ts it's not needed anymore - previously it was discovering module name from path, but now it's explicitly passed as an arg.

Please try deno -A installer/mod.ts

@ry
Copy link
Member

ry commented Jun 14, 2019

Cool. It's working for me.

So this doesn't need to be solved now, but something to think about. I did this:

> deno -A ./installer/mod.ts file_server2 https://deno.land/std/http/file_server.ts -A
Downloading: https://deno.land/std/http/file_server.ts

✅ Successfully installed file_server2.
> which file_server2
/Users/rld/.deno/bin/file_server2
> file_server2 ../deno/website/
HTTP server listening on http:https://0.0.0.0:4500/
[2019-06-14 15:21:32] "GET / HTTP/1.1" 200

That worked correctly.

But now I want to perhaps re-download the file_server2 program. I want to be able to do

file_server2 --reload

and have it operate as if I did

deno --reload https://deno.land/std/http/file_server.ts

but due to the placement of $@ it won't pick up the --reload flag.

@bartlomieju
Copy link
Member Author

@ry yep, I know about this caveat, but haven't figured it out yet.

@ry
Copy link
Member

ry commented Jun 14, 2019

I mean it would be nice if for all deno programs, the argument ordering wasn't so necessary. It would be great if you could do "deno https://deno.land/std/http/file_server.ts --reload"

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks @bartlomieju and @syumai

Is there any other changes you want to make before landing?

@bartlomieju
Copy link
Member Author

bartlomieju commented Jun 14, 2019

I mean it would be nice if for all deno programs, the argument ordering wasn't so necessary. It would be great if you could do "deno https://deno.land/std/http/file_server.ts --reload"

I'll see what we can do about that.

Is there any other changes you want to make before landing?

I added uninstall function but it's currently not exposed. I'm fine with landing as is.

@syumai
Copy link
Contributor

syumai commented Jun 14, 2019

LGTM to me too. Thanks @bartlomieju and @ry !

@ry ry merged commit a3015be into denoland:master Jun 14, 2019
@bartlomieju bartlomieju deleted the feat-installer branch June 14, 2019 15:44
ry pushed a commit to ry/deno that referenced this pull request Oct 9, 2019
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.

Wanted: installer
3 participants