-
Notifications
You must be signed in to change notification settings - Fork 789
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
Automatically link to Homebrew openssl #2275
Conversation
183e5b6
to
15098e5
Compare
d4ac29d
to
8d209e8
Compare
bf25f06
to
2dcfe58
Compare
@hsbt @eregon This is now ready for final review. The change here is that ruby-build will automatically link to a compatible Homebrew OpenSSL on all platforms unless One potential downside of this change is that there is no way to opt out of this mechanism, e.g. if you do have @eregon I noticed that Truffleruby definitions invoke |
It looks like TruffleRuby looks up Homebrew OpenSSL in @eregon With all that in mind, it sounds to me that
I would propose to remove |
Yes, let's remove it. |
I think it would be safer, yes. |
2dcfe58
to
7eb0d9e
Compare
68c1053
to
80a8915
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.
Looks great!
4b59bfd
to
c6429cb
Compare
Update: I went with this syntax in the end, which I find more readable and, most importantly, avoids the use of shell quotes: install_package openssl-1.1.1 "https://..." --if needs_openssl:1.0.2-3.x.x |
Example: install_package openssl-1.1 "https://..." --if needs_openssl:1.0.1-3.1.x In the example, the two values are passed as arguments to the `needs_openssl` function.
If a system OpenSSL version was not found or is at version that is incompatible with a Ruby being installed, ruby-build would typically download and compile a new OpenSSL version scoped to that Ruby installation. Now the `needs_openssl` condition will also check for Homebrew-installed OpenSSL and automatically link to the first one found that satisfies the version requirement. This primarily helps speed up Ruby installation on macOS where the system OpenSSL is never compatible and where Homebrew is a de-facto standard package manager.
c6429cb
to
315373b
Compare
Catching up but is there an opt-out mechanism? |
@brandonfriess-stripe Not yet, but if you open a new issue detailing how the current mechanism (automatically linking to Homebrew) doesn't work for you, we will consider adding an opt-out mechanism. I prefer to first understand potential negative ramifications of the current approach before adding options around it. Thank you! |
If a build definition invokes an openssl requirement, but there is no satisfying system OpenSSL version—e.g. always a state of affairs on macOS—automatically query Homebrew-installed versions with
brew --prefix openssl@<version>
(with a range of versions between "1.1" and "3.1") and automatically link to the latest one found that Ruby is known to be compatible with.The goal is to skip compiling a per-Ruby version of OpenSSL whenever possible, speeding up Ruby installation and reducing the number of compile dependencies for a typical user, hopefully resulting in fewer "build failed" reports in this repository.
We tried this before by always linking to
[email protected]
, but that was neither future-proof (today is more common to haveopenssl@3
installed) nor backward compatible as it sabotaged installations of older Ruby versions. #1375Fixes #2157
Followup to #1974, ref. #1375
Feedback wanted: this PR introduces this syntax:
i.e. there is a single function
needs_openssl
that now takes arbitrary arguments. This replaces the previous set of functions named likeneeds_openssl_102_300
which weren't future-proof. Now, every build definition explicitly and precisely specifies which openssl versions it wants.I'm not in love with the new syntax, especially because quotes are necessary to keep the condition function and its arguments as a single argument in bash. But I do like the added functionality! What do you think?
Possible alternative syntaxes:
--if "needs_openssl 1.0.2 3.x.x"
- I don't like the quotes; it looks like the code in question will geteval
'd from string (not true)--if needs_openssl:1.0.2:3.x.x
--if needs_openssl:1.0.2-3.x.x
--if [email protected]
Keep in mind that the intent here is that this is a more generalized syntax for any
--if
condition, means that it could facilitate passing different arguments than just version numbers in the future.