-
Notifications
You must be signed in to change notification settings - Fork 779
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
Add RUBY_BUILD_TARBALL_OVERRIDE to override the ruby tarball URL #2258
Conversation
d5bbda7
to
fe97440
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm skeptical of this feature because I already wasn't a fan of the original RUBY_BUILD_MIRROR_PACKAGE_URL feature, and this adds even more features around it #1793
And finally I would also like a way to skip the checksum check, for instance when trying a variant of a release which therefore has a different URL and contents but yet I want to reuse the existing definition.
From everything you described so far, it sounds like what you're doing isn't at all mirroring existing releases, but dynamically modifying the target that a build definition downloads and installs. If you need to modify a truffleruby definition slightly, would it be an option for you to create a custom definition and provide ruby-build
with a path to your custom definition?
bin/ruby-build
Outdated
@@ -1194,7 +1198,7 @@ isolated_gem_install() { | |||
apply_ruby_patch() { | |||
local patchfile | |||
case "$1" in | |||
ruby-* | jruby-* | rubinius-* | truffleruby-* ) | |||
ruby-* | jruby-* | mruby-* | picoruby-* | truffleruby-* | truffleruby+graalvm-* ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it belongs in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a cleanup I noticed while working on this and already its own commit, but sure I can split it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with this change being bundled here, apart from being wholly unrelated, is that it won't get its own changelog entry automatically upon cutting a release with script/release
(nor when creating a release using the web interface and choosing "Generate release notes"). I believe that all feature changes deserve a changelog entry.
Also, rubinius-
should stay (for backwards compatibility).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the updated PR it's fully related and necessary.
Right, this is indeed not exactly mirroring. (then I saw RUBY_BUILD_MIRROR_PACKAGE_URL already existed and it seemed nicer to extend it) |
I take this as custom build definitions not being a suitable workaround for you? |
Yeah they would be too annoying to be of any real use. |
My immediate/main use case at hand is @nirvdrum found an issue with the latest TruffleRuby release, and I'd like to make it easy for him/others to try other builds, such as (release|dev) builds at other URLs, CE, etc. Infrastructure typically uses releases of ruby-build so it's not really convenient to ship your own definition for that and that would likely require more infra changes (based on infra details of course). While setting environment variables is quite easy. |
@mislav What do you think of the above? Which way would be OK to merge? |
bin/ruby-build
Outdated
@@ -1194,7 +1198,7 @@ isolated_gem_install() { | |||
apply_ruby_patch() { | |||
local patchfile | |||
case "$1" in | |||
ruby-* | jruby-* | rubinius-* | truffleruby-* ) | |||
ruby-* | jruby-* | mruby-* | picoruby-* | truffleruby-* | truffleruby+graalvm-* ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with this change being bundled here, apart from being wholly unrelated, is that it won't get its own changelog entry automatically upon cutting a release with script/release
(nor when creating a release using the web interface and choosing "Generate release notes"). I believe that all feature changes deserve a changelog entry.
Also, rubinius-
should stay (for backwards compatibility).
bin/ruby-build
Outdated
fi | ||
elif [ -n "$RUBY_BUILD_MIRROR_PACKAGE_URL" ]; then | ||
mirror_url="$RUBY_BUILD_MIRROR_PACKAGE_URL" | ||
if [ -n "$RUBY_BUILD_IGNORE_CHECKSUM" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not believe we should expose this additional option, since we would not want anyone to actually use it.
Instead, in order to support the use-case that you are describing (testing Ruby releases that do not yet have their own definition), I suggest that we introduce a new environment variable completely orthogonal to the mirror feature (since this is technically not related to mirroring, either). The new environment variable could be called something like RUBY_BUILD_TARBALL_OVERRIDE
, and the value read from it would replace both the URL and checksum values in fetch_tarball
. If the environment variable only supplies a URL, then checksum checks would be off anyway, but a checksum could also be optionally supplied too. How does that sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that sounds great, I'll update the PR with it.
fe97440
to
06a58c4
Compare
06a58c4
to
e87c419
Compare
@mislav Updated, could you review and merge if it's OK? |
@@ -371,6 +371,10 @@ fetch_tarball() { | |||
local checksum | |||
local extracted_dir | |||
|
|||
if is_ruby_package "$1" && [ -n "$RUBY_BUILD_TARBALL_OVERRIDE" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason we need the is_ruby_package check is because fetch_tarball is also used for non-ruby packages like openssl and overriding both URLs with the same URL would make no sense.
e87c419
to
7f03f11
Compare
* Update the check for whether a package is a ruby.
7f03f11
to
216084d
Compare
Rebased on top of master to include #2268, and locally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the brevity of the final solution! Thank you.
Indeed it's beautiful how small it is, I was positively surprised as well :) |
This is useful for instance when testing a truffleruby release which is not published yet.
Then I need to use a custom URL, like:
I also want this to try dev builds at a different URL, hence this change.
(and those might have a different checksum too, see below)
And finally I would also like a way to skip the checksum check, for instance when trying a variant of a release which therefore has a different URL and contents but yet I want to reuse the existing definition.
For example this can be useful if I want to try the CE build instead of GFTC build of truffleruby 23.1.0, like this: