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

[New] Enable curl auto compression/decompression #1437

Merged
merged 2 commits into from
Mar 23, 2017

Conversation

PeterDaveHello
Copy link
Collaborator

@PeterDaveHello PeterDaveHello commented Mar 19, 2017

Enable --compressed parameter on curl to automatically enable
compression on request content by sending coressponding header, if the
server side supports compression format like deflate or gzip, curl will
also decompress the content automatically, so there is no additional
works need to done manually on client side, but just enjoy the benifits
of bandwidth and time saving!

Take https://nodejs.org/dist/index.tab as an example which is last
modified on Tue, 14 Mar 2017 22:41:05 GMT, the compressed transmission
only take 4829 bytes howevet the not compressed on taks 48000 bytes,
which is about 10 times larger!

This feature can be traced back to Sep 3 2002, in curl commit:

So should be supported on various versions widely.
(even on Ubuntu 08.04, just tested!)

@PeterDaveHello PeterDaveHello force-pushed the curl-compression branch 2 times, most recently from cda970f to 38e564a Compare March 19, 2017 20:53
@ljharb
Copy link
Member

ljharb commented Mar 20, 2017

Upping the required version of curl would be a breaking change.

What happens in an older version of curl that doesn't understand the flag, if the flag is passed?

Is there a reliable way to detect if curl supports the flag, and only pass it when it does?

@ljharb ljharb added the needs followup We need some info or action from whoever filed this issue/PR. label Mar 20, 2017
@PeterDaveHello
Copy link
Collaborator Author

If the user can't get a curl version newer than 2002 Sep released, I wonder how old the system is and may also failed on other dependencies like git v1.7.0, if we really need to test its ability of parameters, I doubt if we should just test every parameters we are using rather than just this one? Anyway, I guess we can test this feature by curl --compressed when nvm being loading.

@ljharb
Copy link
Member

ljharb commented Mar 21, 2017

That's a fair point; and if you can find evidence that git v1.7.0 only works on a system with a curl that supports --compressed, I'm all set.

@PeterDaveHello
Copy link
Collaborator Author

Sorry I didn't understand what do you mean evidence that git v1.7.0 only works on a system with a curl that supports --compressed, can you explain more details? Thanks.

@ljharb
Copy link
Member

ljharb commented Mar 21, 2017

I mean, find out the system requirements for git v1.7.0 - and see if any of those even would support a curl that lacks --compressed. If not (or if all systems that support git v1.7.0+ come stock with a curl that supports --compressed, then we're good to go.

@PeterDaveHello
Copy link
Collaborator Author

PeterDaveHello commented Mar 21, 2017

I'm not sure what exactly I should to do confirm that, test all famous unix distros? A little bit too hard and too harsh IMO. As we choose Ubuntu 12.04 as the signal of git version requirement, and curl --compressed works even on Ubuntu 8.04, we can even trace back to 2002, which means Ubuntu 4.04 may even support it (but I don't have such an old platform to test it), I think we could give it a try.

@PeterDaveHello
Copy link
Collaborator Author

PeterDaveHello commented Mar 21, 2017

Tested on CentOS 5 which has no git package by default, works very well.

@PeterDaveHello
Copy link
Collaborator Author

Tested with Fedora 20, also works well, I think a 15 years feature could be very widely deployed, what do you think?

@ljharb
Copy link
Member

ljharb commented Mar 21, 2017

My concern is that git is not actually required to use nvm - only curl or wget is.

Altering the git requirement just means people can't get a nice git clone of NVM_DIR - altering the curl requirement could mean people can't use nvm whatsoever. That requires a harsh criteria, I think.

@PeterDaveHello
Copy link
Collaborator Author

PeterDaveHello commented Mar 21, 2017

Thanks for reply @ljharb, if both old CentOS and Ubuntu supports it, I also test it on and it can be traced back to 2002, isn't this good enough? BTW, I just test on another old system - Debian 6, works perfectly. It's very not easy to test those out of maintenance systems, they don't even have a valid package repository anymore.

@ljharb
Copy link
Member

ljharb commented Mar 21, 2017

It'd also need to be tested on old Mac OS X versions.

@PeterDaveHello
Copy link
Collaborator Author

That'll be a problem for me, I have no environment for that, any suggestions or would you like to help on this part? Thanks.

@ljharb
Copy link
Member

ljharb commented Mar 22, 2017

Hmm.

I think I'm convinced that the age of the curl version won't be a problem.

However, my googling indicates that curl can be compiled without compression support - what does --compressed do in that case? Could we detect compressed support by grepping curl --help?

@PeterDaveHello
Copy link
Collaborator Author

@ljharb AFAIK the help message is "static", so No, we can't detect from that, just like http/2.0 feature in curl, but I wonder if we should even test all features that can be optional in the compilation? At least I didn't find a real case that a common package repository that provides curl without that feature, just like all the browsers support gzip compression.

@ljharb
Copy link
Member

ljharb commented Mar 22, 2017

@PeterDaveHello ok, let's do it - but let's add a note to the readme noting the new requirement for curl.

@PeterDaveHello
Copy link
Collaborator Author

PeterDaveHello commented Mar 22, 2017

@ljharb I need your review and maybe even reword, thanks

**Note:** `nvm` uses `curl` with `--compressed` feature which was introduced since Sep 3 2002([curl/curl@64bbe9d](https://github.com/curl/curl/commit/64bbe9dfafc6693a96b742f3133c636385835a19)), it's not common but if you have `curl` that compiled without that feature, there may be a problem for you, so please make sure your `curl` works with `--compressed`, we already tested it with all the famous GNU/Linux distros on their old versions, and this works on all of them!

@ljharb
Copy link
Member

ljharb commented Mar 22, 2017

**Note:** `nvm` uses `curl` with the `--compressed` option which was introduced on September 3rd, 2002([curl/curl@64bbe9d](https://github.com/curl/curl/commit/64bbe9dfafc6693a96b742f3133c636385835a19)). It's unlikely, but if you have `curl` compiled without that feature, `nvm` may not be able to install or list remote `node` versions. Please file an issue if your `curl` does not work with `--compressed` (and you didn't explicitly compile `curl` to not have compression support).

@PeterDaveHello
Copy link
Collaborator Author

Update, thanks!

@PeterDaveHello PeterDaveHello force-pushed the curl-compression branch 2 times, most recently from cb77efa to 7613c6e Compare March 23, 2017 05:42
@ljharb ljharb added feature requests I want a new feature in nvm! installing node Issues with installing node/io.js versions. performance This relates to anything regarding the speed of using nvm. and removed needs followup We need some info or action from whoever filed this issue/PR. labels Mar 23, 2017
PeterDaveHello added a commit to PeterDaveHello/nvm that referenced this pull request Mar 23, 2017
@ljharb
Copy link
Member

ljharb commented Mar 23, 2017

Seems like there's some test failures.

@PeterDaveHello
Copy link
Collaborator Author

Not sure if all fixed now, let's see.

@ljharb
Copy link
Member

ljharb commented Mar 23, 2017

Still a failure - looks like you forgot to unload nvm_curl_libz_support?

@PeterDaveHello
Copy link
Collaborator Author

@ljharb seems so, I must messed up some of the progress so I lost this part, added it back :)

@PeterDaveHello
Copy link
Collaborator Author

PeterDaveHello commented Mar 23, 2017

not sure what's the relationship with nvm_get_latest, now digging ...

Enable `--compressed` parameter on curl to automatically enable
compression on request content by sending coressponding header, if the
server side supports compression format like deflate or gzip, curl will
also decompress the content automatically, so there is no additional
works need to done manually on client side, but just enjoy the benifits
of bandwidth and time saving!

Take https://nodejs.org/dist/index.tab as an example which is last
modified on Tue, 14 Mar 2017 22:41:05 GMT, the compressed transmission
only take 4829 bytes howevet the not compressed on taks 48000 bytes,
which is about 10 times larger!

This feature can be traced back to Sep 3 2002, in curl commit:
 - curl/curl@64bbe9d

So should be supported on various versions widely.
@PeterDaveHello
Copy link
Collaborator Author

Fixed now.

@ljharb ljharb merged commit 973dfc6 into nvm-sh:master Mar 23, 2017
ljharb pushed a commit to PeterDaveHello/nvm that referenced this pull request Mar 23, 2017
@PeterDaveHello PeterDaveHello deleted the curl-compression branch March 23, 2017 19:33
edwmurph pushed a commit to edwmurph/nvm that referenced this pull request Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature requests I want a new feature in nvm! installing node Issues with installing node/io.js versions. performance This relates to anything regarding the speed of using nvm.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants