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

installer upgrades #512

Merged
merged 40 commits into from Jun 20, 2019

Conversation

3 participants
@bartlomieju
Copy link
Contributor

commented Jun 19, 2019

Closes #497.
Closes #501

Blocked on #509.

This PR:

  • splits installer into separate files leaving flag parsing in installer/mod.ts
  • removes uninstall command
  • adds --reload to deno fetch - this ensures that subsequent installation will upgrade executable
  • fixes shebang missing !
  • fixes prompt on subsequent installation
  • supports -d/--dir parameter for custom installation directory

After we land this PR I'll upgrade deno install.

axetroy and others added some commits Jun 19, 2019

fmt

@bartlomieju bartlomieju changed the title chore: split installer installer upgrades Jun 19, 2019

bartlomieju added some commits Jun 19, 2019

@bartlomieju bartlomieju force-pushed the bartlomieju:split_installer branch 3 times, most recently from 1ccca91 to cc4ff74 Jun 19, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

After some thought and especially In light of denoland/deno#2546 I think that uninstall command is superfluous.

If we add ability for custom installation directory we'd need to pass the same directory to uninstall (deno_installer uninstall -d /usr/local/bin file_server). Since uninstalling is just removing a single file it makes no sense to pass installation dir there. So there are two options:
a) remove uninstall command and leave it to user to remove the file
b) try to locale installed file (which file_server or where file_server on Windows) and verify that it's file installed via installer (probably by matching content of the file) and then remove it.

Opinions?

@ry

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

I think that uninstall command is superfluous.

I agree. For myself at least “rm $(which file_server)” works

(Brevity due to phone)

@axetroy

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

I think that uninstall command is superfluous.

agree with this.

but I don't think allow different installation directories is a good idea.

This may cause some module to induce users to install in /usr/local/bin instead of the default $HOME/.deno/bin

It makes difficult to manage the installed module.

example1: if the user reinstalls in different directories. There are two identical scripts in the $PATH, how to deal with this?

example2: if there is a conflict with the native command, for example, there is a file ls under /usr/local/bin. At this time, the user installs the ls module to /usr/local/bin, what should I do?

At least for the current stage, I disapprove.

The solution to allowing different installation directories

If the user wants to install in /usr/local/bin, he can move the script manually.

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

This may cause some module to induce users to install in /usr/local/bin instead of the default $HOME/.deno/bin

It makes difficult to manage the installed module.

You have to explicitly tell installer that you want to install somewhere else (eg. --dir /usr/local/bin), installed modules won't have ability to decide where to install themselves.

example1: if the user reinstalls in different directories. There are two identical scripts in the $PATH, how to deal with this?

example2: if there is a conflict with the native command, for example, there is a file ls under /usr/local/bin. At this time, the user installs the ls module to /usr/local/bin, what should I do?

Your shell deals with that, nothing prevents you from doing exactly these things right now.

Some people might want to keep all executable files in single place and don't want to add ~/.deno/bin to their PATH

@axetroy

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

My point is to keep the computer as simple as possible.

When I need deno, I will go to $HOME/.deno to find the file.
When I don't need deno or want to uninstall it, just remove $HOME/.deno, except for the cache. No residual files.

make it clean and don't mess up your computer.

@ry

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

@axetroy I'm of your opinion too I prefer everything located in ~/.deno, but I think a flag to specify the directory is fine...

(I kind of wish the cache was still there too...)

bartlomieju added some commits Jun 20, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Okay so I added -d/--dir flag - it tells where to put executable. This said, installed executable still uses default deno cache directory.

(I kind of wish the cache was still there too...)

Maybe we should add DENO_DIR to generated executable file - then each installed module is isolated and can be simply upgraded. WDYT?

@ry

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

Maybe we should add DENO_DIR to generated executable file - then each installed module is isolated and can be simply upgraded. WDYT?

Eh.. Idk. I'd prefer the DENO_DIR be controlled by the caller.

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Eh.. Idk. I'd prefer the DENO_DIR be controlled by the caller.

Let's tackle that later. This PR should be landable as is. PTAL and review

fmt
@ry
Copy link
Contributor

left a comment

Looks good - just one thing...

OPTIONS:
-d, --dir <PATH> Installation directory path (defaults to ~/.deno/bin)
`);

This comment has been minimized.

Copy link
@ry

ry Jun 20, 2019

Contributor

I wish the flags module was more like Go's... This would be generated automatically.

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jun 20, 2019

Author Contributor

Agreed, I was looking at porting one of more advanced CLI modules from node:

But I might as well look into https://golang.org/pkg/flag/

Show resolved Hide resolved installer/mod.ts Outdated

bartlomieju added some commits Jun 20, 2019

@ry

This comment has been minimized.

Copy link
Contributor

commented on installer/test.ts in 571354b Jun 20, 2019

👍

@ry

ry approved these changes Jun 20, 2019

Copy link
Contributor

left a comment

LGTM

@ry

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

@bartlomieju Can you draft a commit message please?

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

@ry

Upgrade installer

  • remove uninstall command
  • add --reload to deno fetch - to ensure subsequent installation upgrades script and deps
  • fix executable shebang
  • fix prompt for subsequent installation
  • support custom installation dir via -d/--dir flag

@ry ry merged commit b13441f into denoland:master Jun 20, 2019

5 checks passed

denoland.deno_std Build #20190620.12 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:split_installer branch Jun 20, 2019

@bartlomieju bartlomieju referenced this pull request Jun 20, 2019

Closed

Installer: missing real test in different platforms #500

0 of 3 tasks complete

axetroy added a commit to axetroy/deno_std that referenced this pull request Jun 24, 2019

remove from-stdin
add test case for reading code from stdin and format

update

update

update

update

update

update

update

update

update

update

update

update

update

update

update

improve installer (denoland#512)

- remove uninstall command
- add --reload to deno fetch - to ensure subsequent installation
  upgrades script and deps
- fix executable shebang
- fix prompt for subsequent installation
- support custom installation dir via -d/--dir flag

typo (denoland#515)

bundle/run handles Deno.args better. (denoland#514)

fix: pin eslint version for CI (denoland#518)

typescript-eslint/typescript-eslint#637

feat: add catjson example (denoland#517)

file server should order filenames (denoland#511)

update

update

update

update

update

update

update

update

update

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