-
Notifications
You must be signed in to change notification settings - Fork 787
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
Update so Ruby 1.9.3 thru 2.3, and rbx can be built with OpenSSL 1.0.2u #1397
Conversation
bin/ruby-build
Outdated
rm -rf "$OPENSSLDIR/certs" "$pem_file" | ||
ln -s /etc/ssl/certs "$OPENSSLDIR" | ||
ln -s /etc/ssl/certs/ca-certificates.crt "$pem_file" | ||
fi |
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 logic needs more tweaks, it needs to support other OS, and have some kind of fallback.
We probably want to rename build_package_mac_openssl
to just build_package_openssl
or so.
In general I think it would be better to have 2 PRs:
- One updating OpenSSL and no other changes.
- One building OpenSSL on Linux for Ruby <= 2.3 (since it's hard to get OpenSSL 1.0.2 from Linux distributions).
@MSP-Greg Can you separate the pull-request to update versions of OpenSSL and change the certificate logic at least? |
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.
Hi @MSP-Greg, I think your heart is in the right place, but there are multiple things risky with this proposed change.
It looks like you are suggesting that we change Ruby 2.3 build definitions so that they always install an isolated OpenSSL 1.0 instance unconditionally. Before, we only did this on macOS— on other OS, we never try to set up OpenSSL on behalf of the user.
With this change:
- Ruby 2.3 definitions will behave very differently than Ruby 2.4+ definitions, which might be surprising to our users;
- We would need to handle setting up system certs for the OpenSSL installation, which we so far handle only for macOS and I don't think we would want to maintain this logic for more OSs, as it would be too complicated and potentially controversial;
- When OpenSSL install fails on various platforms, we would need to debug it when users open issues in this repo, and we don't want to go down that road.
I think users on non-macOS platforms should be responsible for setting up an appropriate OpenSSL instance on their machine and linking to that using build flags.
Thanks all. I'm revised the PR, removing the cert logic, leaving the As is, it does work on Ubuntu, but as mentioned, leaving it OS specific is probably best. |
There are instructions in the wiki how to workaround on Linux for old OpenSSL: https://github.com/rbenv/ruby-build/wiki#openssl-usrincludeopensslasn1_mach102-error-error-this-file-is-obsolete-please-update-your-software (last paragraph) |
8a04d6d
to
d7eac84
Compare
The previous commit that updated to 1.0.2q had quite a few files. 1.0.2u should be compatible with 1.0.1, but I thought I'd split the commits up in case there are issues and some need to be reverted. |
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 looks good to me.
d7eac84
to
ca5c0f3
Compare
Fixed commit subject typo 'OpeSSL' => 'OpenSSL' |
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.
Thank you; this looks good to me!
share/ruby-build/2.3.x
files to use OpenSSL 1.0.2uThis is for use in generating Rubies for use on GitHub Actions. A build run is here:
https://github.com/MSP-Greg/ruby-install-builder/runs/384299585