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

Fix Gem::Specification#to_ruby without OpenSSL #3059

Closed
wants to merge 1 commit into from

Conversation

alyssais
Copy link
Contributor

Description:

OpenSSL is an optional dependency of Ruby. Requiring openssl
unconditionally means that Rubies that don't have the OpenSSL library
won't be able to use RubyGems at all.

In such a Ruby, an OpenSSL::PKey::RSA object can't exist, so we can
just skip the check for those objects if there is no OpenSSL.

Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

OpenSSL is an optional dependency of Ruby.  Requiring openssl
unconditionally means that Rubies that don't have the OpenSSL library
won't be able to use RubyGems at all.

In such a Ruby, an OpenSSL::PKey::RSA object can't exist, so we can
just skip the check for those objects if there is no OpenSSL.
@simi
Copy link
Member

simi commented Dec 29, 2019

This require was added at #2937. Is Ruby without openssl supported by rubygems? Simple search for openssl at github (https://github.com/rubygems/rubygems/search?p=1&q=OpenSSL&unscoped_q=OpenSSL) shows different approaches how to handle OpenSSL presence.

Copy link

@zimbatm zimbatm left a comment

Choose a reason for hiding this comment

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

an idea to make the relationship between both code blocks more explicit

@@ -2414,7 +2414,10 @@ def test_files # :nodoc:
# still have their default values are omitted.

def to_ruby
require 'openssl'
begin
require 'openssl'
Copy link

Choose a reason for hiding this comment

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

Suggested change
require 'openssl'
require 'openssl'
has_openssl = true

require 'openssl'
begin
require 'openssl'
rescue LoadError
Copy link

Choose a reason for hiding this comment

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

Suggested change
rescue LoadError
rescue LoadError
has_openssl = false

@@ -2454,7 +2457,9 @@ def to_ruby
next if handled.include? attr_name
current_value = self.send(attr_name)
if current_value != default_value(attr_name) || self.class.required_attribute?(attr_name)
result << " s.#{attr_name} = #{ruby_code current_value}" unless current_value.is_a?(OpenSSL::PKey::RSA)
unless defined?(OpenSSL) && current_value.is_a?(OpenSSL::PKey::RSA)
Copy link

Choose a reason for hiding this comment

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

Suggested change
unless defined?(OpenSSL) && current_value.is_a?(OpenSSL::PKey::RSA)
unless has_openssl && current_value.is_a?(OpenSSL::PKey::RSA)

Copy link
Member

Choose a reason for hiding this comment

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

I like this approach

Copy link
Member

Choose a reason for hiding this comment

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

i prefer keeping consistent with the library by checking for the constant

@zimbatm
Copy link

zimbatm commented Dec 30, 2019

@simi ruby is shipping with rubygems right? If ruby can have openssl disabled then rubygems should probably also support it at least for basic functionality.

@alyssais
Copy link
Contributor Author

alyssais commented Dec 31, 2019 via email

@mame
Copy link
Contributor

mame commented Jan 7, 2020

See https://bugs.ruby-lang.org/issues/16475

I think that rubygems does not have to seriously support an environment without openssl, but I want make test-all of ruby to work.

@zimbatm
Copy link

zimbatm commented Jan 7, 2020

Just to give a use-case: a small ruby script that I want to distribute with a minimal installation of ruby. The script just uses the ruby stdlib and doesn't need network access so OpenSSL is not needed.

@deivid-rodriguez
Copy link
Member

I'm introducing this fix as part of #3816, so closing this!

Thanks @alyssais and sorry for the delay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants