[New] Support `--no-progress` for `nvm install`, close #1079 by PeterDaveHello · Pull Request #1422 · nvm-sh/nvm

PeterDaveHello · 2017-03-09T17:11:27Z

samrocketman · 2017-03-11T07:19:18Z

nvm.sh

  if [ "${FLAVOR}" = 'node' ]; then
    NODE_OR_IOJS="${FLAVOR}"
  fi
+  if [ "${NVM_NO_PROGRESS}" = "1" ]; then


For things like this it's best to test if the environment variable is non-zero length:

if [ -n "${NVM_NO_PROGRESS}" ]; then ... fi

Either it exists and has content or doesn't is how I've seen it typically tested in other projects which are bash-based. I tend to like that style of testing more.

samrocketman · 2017-03-11T07:22:06Z

nvm.sh

    NODE_OR_IOJS="${FLAVOR}"
  fi
+  if [ "${NVM_NO_PROGRESS}" = "1" ]; then
+    PROGRESS_BAR="-s"


Please use:

PROGRESS_BAR="-sS"

-s by itself silences all output (including errors). However, I believe a user would want to know why something failed so -S will still properly surface errors to them. -sS will only output if there's an error.

samrocketman · 2017-03-11T07:23:31Z

nvm.sh

                           -e 's/-L //' \
                           -e 's/-I /--server-response /' \
-                           -e 's/-s /-q /' \
+                           -e 's/-s /-nv -q /' \


Related to other feedback I think this should be (adding -sS and excluding -q):

... -e 's/-sS /-nv /' ...

You don't want -q because it will completely disable output including errors. From the wget man page -nv is non-verbose but will still display errors when they occur (akin to -sS).

samrocketman · 2017-03-11T07:27:20Z

nvm.sh

          fi

-          if nvm_install_source "${FLAVOR}" std "${VERSION}" "${NVM_MAKE_JOBS}" "${ADDITIONAL_PARAMETERS}"; then
+          if NVM_NO_PROGRESS="${noprogress}" nvm_install_source "${FLAVOR}" std "${VERSION}" "${NVM_MAKE_JOBS}" "${ADDITIONAL_PARAMETERS}"; then


Similar to earlier comment:

if NVM_NO_PROGRESS="${NVM_NO_PROGRESS:-${noprogress}}" nvm_install_source ...

samrocketman · 2017-03-11T07:29:43Z

nvm.sh

        # skip binary install if "nobinary" option specified.
        if [ $nobinary -ne 1 ] && nvm_binary_available "$VERSION"; then
-          if nvm_install_binary "${FLAVOR}" std "${VERSION}"; then
+          if NVM_NO_PROGRESS="${noprogress}" nvm_install_binary "${FLAVOR}" std "${VERSION}"; then


Might I make a recommendation here as well?

if NVM_NO_PROGRESS="${NVM_NO_PROGRESS:-${noprogress}}" nvm_install_source ...

If you look up Parameter Expansion in the bash man page you'll find that ${VAR:-default} will only take the default value if $VAR is unset. The reason why this is useful is because it allows a user to set NVM_NO_PROGRESS globally as an environment variable on their system which will always disable progress without having to use the nvm --no-progress option. In a CI-like, environment I feel this would be desirable.

Just my two cents on environment variables and using defaults.

PeterDaveHello · 2017-03-12T12:03:33Z

@ljharb do you agree with all the change requests from @samrocketman? If so, I can start to update the PR, thanks.

ljharb · 2017-03-17T07:36:50Z

@PeterDaveHello the comments look good; I'm confused why NVM_NO_PROGRESS can't just be "empty" or "nonempty" instead of a "noprogress" string?

ljharb · 2017-03-17T07:37:17Z

nvm.sh

      fi

-      local nobinary
+      local nobinary noprogress


please put each local declaration on its own separate line.

PeterDaveHello · 2017-03-17T14:42:23Z

Okay I updated 😄

samrocketman

Looks good to me. Thanks for taking the time.

PeterDaveHello · 2017-03-18T09:05:02Z

Got conflicts ... fixing now.

PeterDaveHello · 2017-03-19T08:45:32Z

Should be fixed now.

ljharb · 2017-03-20T04:52:30Z

This doesn't seem to work for me consistently in local testing.

nvm install --no-progress -s 5.9.0
Downloading https://nodejs.org/dist/v5.9.0/node-v5.9.0.tar.xz...
curl: (3) <url> malformed
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  1 12.6M    1  211k    0     0   355k      0  0:00:36 --:--:--  0:00:36  354k^C

samrocketman · 2017-03-20T06:01:50Z

@ljharb can you elaborate on the versions of software you're using? Considering what is involved in this PR perhaps versions of:

lsb_release -a
#alternatively instead of lsb_release: head -n1 /etc/issue
uname -rms
bash --version
curl --version
wget --version
sed --version

PeterDaveHello · 2017-03-20T08:42:51Z

My mistake, should be fixed now.

ljharb · 2017-03-21T00:56:22Z

@samrocketman should be whatever comes stock on my Mac:

Details

```sh $ lsb_release -a -bash: lsb_release: command not found

$ head -n1 /etc/issue
head: /etc/issue: No such file or directory

$ uname -rms
Darwin 16.4.0 x86_64

$ bash --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin16)
Copyright (C) 2007 Free Software Foundation, Inc.

$ curl --version
curl 7.51.0 (x86_64-apple-darwin16.0) libcurl/7.51.0 SecureTransport zlib/1.2.8
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: AsynchDNS IPv6 Largefile GSS-API Kerberos SPNEGO NTLM NTLM_WB SSL libz UnixSockets

$ wget --version
GNU Wget 1.19.1 built on darwin15.6.0.

-cares +digest -gpgme +https +ipv6 -iri +large-file -metalink -nls
+ntlm +opie -psl +ssl/openssl

Wgetrc:
/usr/local/etc/wgetrc (system)
Compile:
clang -DHAVE_CONFIG_H -DSYSTEM_WGETRC="/usr/local/etc/wgetrc"
-DLOCALEDIR="/usr/local/Cellar/wget/1.19.1/share/locale" -I.
-I../lib -I../lib -I/usr/local/opt/openssl/include -DNDEBUG
Link:
clang -DNDEBUG -liconv -L/usr/local/opt/openssl/lib -lssl -lcrypto
-ldl -lz ftp-opie.o openssl.o http-ntlm.o ../lib/libgnu.a

Copyright (C) 2015 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later
http://www.gnu.org/licenses/gpl.html.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Originally written by Hrvoje Niksic hniksic@xemacs.org.
Please send bug reports and questions to bug-wget@gnu.org.

$ sed --version
sed: illegal option -- -

</details>

ljharb

This now appears to work in my local testing, thanks.

Is there any way we could test this? Perhaps one test could mock out nvm_download and assert on the presence of --progress-bar vs -sS, and then another test for nvm_download itself?

PeterDaveHello · 2017-03-21T05:22:33Z

I'm not sure what kind of assertion would it looks like, any suggestions?

ljharb · 2017-03-22T07:17:01Z

Take a look at test/fast/Unit tests/nvm_get_checksum and a number of other tests in that folder for inspiration.

ljharb · 2017-06-13T07:37:10Z

@PeterDaveHello it'd be great to get this in soon; have you had a chance to work on these tests?

PeterDaveHello · 2017-06-14T02:22:45Z

@ljharb sorry for the delay, I'd like to, but not sure when can I finish it, no offense, but I also wonder if it's a little bit rush, would that be possible that you can directly finish the tests?

PeterDaveHello · 2017-06-23T08:21:44Z

@ljharb please take a look, test added, not sure if it's good enough, thanks.

PeterDaveHello · 2018-04-23T14:16:36Z

Ping... (merge conflicts fixed)

ljharb · 2018-04-23T21:12:27Z

nvm.sh

    NODE_OR_IOJS="${FLAVOR}"
  fi
+  if [ "${NVM_NO_PROGRESS-}" = "1" ]; then
+    PROGRESS_BAR="-sS"


should this be --silent --show-error, or would that make combining them tricky?

Is there a reason we ever don't want --show-error semantics?

I think it's because we use parameters like -L, -I, -C, so it's to maintain consistency to use a short parameter and maybe I should even use -# instead of --progress-bar?

As long as both forms work on the same systems, I'd prefer the long versions for readability.

Maybe revise this somewhere else? Consistency is important.

Fair. We can stick to the short versions now but please add the long ones in a comment.

I mentioned this before (can't remember where). The long option is not always available in some versions of curl. However, the short option definitely is. For compatibility, I recommend sticking to the short option for this specific feature.

Thanks, in that case let's stick with the short options (but comment the long ones, for clarity)

ljharb · 2018-04-23T21:13:00Z

nvm.sh

+  if [ "${NVM_NO_PROGRESS-}" = "1" ]; then
+    PROGRESS_BAR="-sS"
+  else
+    PROGRESS_BAR="--progress-bar"


where is "PROGRESS_BAR", as defined here, referenced?

You mean the definition?

no - I mean that lines 2017-2021 seem useless since nothing ever uses PROGRESS_BAR in this code path.

I think nvm_download_artifact() will use it

It can't use it unless it's passed or exported.

hmmm ... shell's variable passing is always confusing, it looks working. I pass it anyway ;)

ljharb

I'll try this out locally, then merge.

samrocketman · 2018-05-02T07:02:56Z

Thanks for sticking it out @PeterDaveHello

[New] Support --no-progress for nvm install

113d807

Closes nvm-sh#1079.

PeterDaveHello mentioned this pull request Mar 10, 2017

Option to disable curl-like download progress #1079

Open

samrocketman suggested changes Mar 11, 2017

View reviewed changes

ljharb requested changes Mar 17, 2017

View reviewed changes

PeterDaveHello force-pushed the no-progress-bar-support branch from 1eee505 to dc35687 Compare March 17, 2017 14:42

samrocketman approved these changes Mar 17, 2017

View reviewed changes

PeterDaveHello force-pushed the no-progress-bar-support branch from dc35687 to f0689cf Compare March 18, 2017 01:43

ljharb added feature requests I want a new feature in nvm! installing node Issues with installing node/io.js versions. labels Mar 18, 2017

PeterDaveHello force-pushed the no-progress-bar-support branch 2 times, most recently from 6873bf0 to 8195949 Compare March 19, 2017 08:44

PeterDaveHello force-pushed the no-progress-bar-support branch from 8195949 to ddb0a92 Compare March 20, 2017 08:42

ljharb requested changes Mar 21, 2017

View reviewed changes

PeterDaveHello force-pushed the no-progress-bar-support branch from ddb0a92 to 4d5b180 Compare June 23, 2017 07:54

PeterDaveHello force-pushed the no-progress-bar-support branch 2 times, most recently from df45148 to dd238c1 Compare April 23, 2018 11:07

PeterDaveHello force-pushed the no-progress-bar-support branch from dd238c1 to 3b3e368 Compare April 23, 2018 11:15

ljharb reviewed Apr 23, 2018

View reviewed changes

PeterDaveHello force-pushed the no-progress-bar-support branch 4 times, most recently from 5d118ce to d9afdba Compare April 25, 2018 04:21

ljharb approved these changes May 2, 2018

View reviewed changes

ljharb force-pushed the no-progress-bar-support branch from d9afdba to 113d807 Compare May 2, 2018 04:16

ljharb merged commit 113d807 into nvm-sh:master May 2, 2018

PeterDaveHello deleted the no-progress-bar-support branch May 2, 2018 04:36

Uh oh!

Conversation

PeterDaveHello commented Mar 9, 2017 • edited by ljharb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samrocketman Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samrocketman Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samrocketman Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterDaveHello commented Mar 12, 2017

Uh oh!

ljharb commented Mar 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterDaveHello commented Mar 17, 2017

Uh oh!

samrocketman left a comment

Choose a reason for hiding this comment

Uh oh!

PeterDaveHello commented Mar 18, 2017

Uh oh!

PeterDaveHello commented Mar 19, 2017

Uh oh!

ljharb commented Mar 20, 2017

Uh oh!

samrocketman commented Mar 20, 2017

Uh oh!

PeterDaveHello commented Mar 20, 2017

Uh oh!

ljharb commented Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

PeterDaveHello commented Mar 21, 2017

Uh oh!

ljharb commented Mar 22, 2017

Uh oh!

ljharb commented Jun 13, 2017

Uh oh!

PeterDaveHello commented Jun 14, 2017

Uh oh!

PeterDaveHello commented Jun 23, 2017

Uh oh!

PeterDaveHello commented Apr 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterDaveHello commented Mar 9, 2017 •

edited by ljharb

Loading

samrocketman Mar 11, 2017 •

edited

Loading

samrocketman Mar 11, 2017 •

edited

Loading

samrocketman Mar 11, 2017 •

edited

Loading

ljharb commented Mar 21, 2017 •

edited

Loading