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

Conversation

3 participants
@bartlomieju
Copy link
Contributor

commented Jun 13, 2019

Closes #471

This PR builds on #488 by @syumai

I've managed to remove call to wget

@syumai syumai referenced this pull request Jun 13, 2019

Closed

Add installer #488

0 of 2 tasks complete

bartlomieju added some commits Jun 13, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

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:///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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

How does an installer request permissions?

Not sure what you mean

@ry

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

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 added some commits Jun 13, 2019

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-installer branch from dd64692 to 0576c4c Jun 14, 2019

bartlomieju added some commits Jun 14, 2019

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-installer branch from 7b291b8 to 29d891e Jun 14, 2019

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-installer branch from 4580cf2 to 25b9ae5 Jun 14, 2019

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-installer branch from 81d36b6 to 77c37db Jun 14, 2019

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-installer branch from 0d1aa43 to 81030d6 Jun 14, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

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

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

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.
Show resolved Hide resolved installer/README.md Outdated
Show resolved Hide resolved installer/README.md Outdated

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-installer branch from b9afed1 to 637a6f3 Jun 14, 2019

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.
```

This comment has been minimized.

Copy link
@ry

ry Jun 14, 2019

Contributor

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:///Users/rld/src/deno_std/installer/deno_installer.ts
~/src/deno_std>

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jun 14, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Contributor

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://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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

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

@ry

This comment has been minimized.

Copy link
Contributor

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"

@ry

ry approved these changes Jun 14, 2019

Copy link
Contributor

left a comment

LGTM - thanks @bartlomieju and @syumai

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

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link

commented Jun 14, 2019

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

@ry ry merged commit a3015be into denoland:master Jun 14, 2019

5 checks passed

denoland.deno_std Build #20190614.25 succeeded
Details
denoland.deno_std (Linux) Linux succeeded
Details
denoland.deno_std (Mac) Mac succeeded
Details
denoland.deno_std (Windows) Windows succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@bartlomieju bartlomieju deleted the bartlomieju:feat-installer branch Jun 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.