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

Feature: more flexible mirror URL by a command #1793

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

uzxmx
Copy link
Contributor

@uzxmx uzxmx commented Aug 10, 2021

This PR provides a more flexible way to specify the mirror URL by using a shell function than #1457.

For #1457, users need to know the complete mirror URL, and another drawback is users need to change the RUBY_BUILD_MIRROR_PACKAGE_URL env every time they install a different version.

This PR resolves these two drawbacks. Users can build the final mirror URL by any shell commands.

Thanks to the author of #1032, the idea comes from that thread.

README.md Outdated Show resolved Hide resolved
@uzxmx
Copy link
Contributor Author

uzxmx commented Aug 11, 2021

@eregon I've updated the example to a POSIX compatible one. Please have a review. Thanks.

@eregon
Copy link
Member

eregon commented Aug 11, 2021

Thanks, that variant avoids relying on exporting shell functions which seems far more portable.

I feel like an env var to set the mirror base URL could be simpler, but one issue is e.g.

https://cache.ruby-lang.org/pub/ruby/3.0/ruby-3.0.0.tar.gz
is mirrored as
https://mirror.cyberbits.eu/ruby/3.0/ruby-3.0.0.tar.gz
https://cache.ruby-china.com/pub/ruby/3.0/ruby-3.0.0.tar.gz

CRuby mirror list
BTW many of these mirrors don't have recent releases, I wonder which are actually used.
The China 2 (Ruby China) mirror at least has the same structure as cache.ruby-lang.org/ so for those we could just reuse the URL path and just change the domain.

@hsbt What do you think?

@uzxmx
Copy link
Contributor Author

uzxmx commented Aug 12, 2021

IMO, just changing the domain is definitely the simplest solution. But so far, ruby-build actually doesn't support this way, some codes need to be added. For example, for RUBY_BUILD_MIRROR_URL, the final mirror URL is built by concatenating RUBY_BUILD_MIRROR_URL with the checksum, so we cannot rely on it.

ruby-build/bin/ruby-build

Lines 364 to 371 in 57c397d

if [ -n "$RUBY_BUILD_MIRROR_URL" ]; then
if [[ -z "$RUBY_BUILD_DEFAULT_MIRROR" || $package_url != */cache.ruby-lang.org/* ]]; then
mirror_url="${RUBY_BUILD_MIRROR_URL}/$checksum"
fi
elif [ -n "$RUBY_BUILD_MIRROR_PACKAGE_URL" ]; then
mirror_url="$RUBY_BUILD_MIRROR_PACKAGE_URL"
fi
fi

Just changing the domain may also not work for other ruby variants, like jruby (e.g. one url is https://s3.amazonaws.com/jruby.org/downloads/1.7.22/jruby-bin-1.7.22.tar.gz#554da042087bd4a787c73626c81fa354c9ce1168735032f7d954cffec85f5a4a).

@jasonkarns
Copy link
Member

This looks mostly compatible with the implementation that node-build uses from nodenv/node-build#210

As @uzxmx mentioned different mirror urls, it's the same reason node-build opted to accept the _CMD env var: to be flexible enough to handle any mirror and not be limited to those that preserve identical path formats, or that accept the checksum in different ways.

Copy link
Member

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Thank you for your contributions to ruby-build and helping make it more usable in China!

As a late reviewer to this whole extended mirror feature, I would like to voice my concerns about the complexity of the feature (incl. the original PR that already got merged).

First of all, I must admit I do not understand RUBY_BUILD_MIRROR_PACKAGE_URL. The value is the static URL from which a Ruby distribution is to be downloaded, right? But a single Ruby install can attempt to download more archives than just the Ruby version. For example, some installs will also need to fetch openssl from a remote server. Does RUBY_BUILD_MIRROR_PACKAGE_URL affects all the downloads initiated within ruby-build, or just the Ruby-related one? If it doesn't affect all downloads, then I fear that this mirror approach isn't adequate.

Second, I am confused about the internal variable RUBY_BUILD_DEFAULT_MIRROR. Why would we have separate logic for the default mirror and for user-provided mirrors? That feels like a code smell to me. Our mirroring logic should be flexible enough to handle either our own mirror or user-provided mirrors without conditionals.

Lastly, I do dig the new RUBY_BUILD_MIRROR_CMD option because it seems the most flexible one. If that gets merged, I would vote to eliminate RUBY_BUILD_MIRROR_PACKAGE_URL and RUBY_BUILD_DEFAULT_MIRROR.

bin/ruby-build Outdated Show resolved Hide resolved
@uzxmx
Copy link
Contributor Author

uzxmx commented Aug 14, 2021

@mislav Thanks for the suggestion. It's truly useful. Already adopted.

I agree the combination of RUBY_BUILD_MIRROR_PACKAGE_URL with RUBY_BUILD_DEFAULT_MIRROR is a bit confusing unless we read the whole logic carefully. Those names might mislead new users. If the old compatibility is not desired, I also vote to eliminate them.

@mislav
Copy link
Member

mislav commented Aug 16, 2021

Those names might mislead new users. If the old compatibility is not desired, I also vote to eliminate them.

Ah, I just realized that RUBY_BUILD_MIRROR_PACKAGE_URL was supported since Dec 2020. I wasn't aware of that. In the interest of backwards compatibility, it should keep being supported, but perhaps we should strip it from documentation in favor of RUBY_BUILD_MIRROR_CMD and perhaps print a deprecation warning to stderr when someone attempts to use it.

@uzxmx
Copy link
Contributor Author

uzxmx commented Aug 20, 2021

@eregon @hsbt @mislav

Could you help to make a conclusion about what else should be done? I'm eager to make the according changes to get it merged. 😄

@jasonkarns
Copy link
Member

I just recently (finally) have been getting node-build up to date with ruby build the past few weeks. I recently merged nodenv/node-build#901 which backports the RUBY_BUILD_MIRROR_PACKAGE_URL env var into node-build. Which means I have node-build's _MIRROR_CMD var working in conjunction with _MIRROR_PACKAGE_URL.

I should be able to replicate that into ruby-build if that's still desired.

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.

4 participants