Skip to content
This repository was archived by the owner on Jan 18, 2023. It is now read-only.

Ruby 2.2+ compatibility#1

Merged
methodmissing merged 2 commits into
masterfrom
ruby23
Jun 13, 2016
Merged

Ruby 2.2+ compatibility#1
methodmissing merged 2 commits into
masterfrom
ruby23

Conversation

@methodmissing
Copy link
Copy Markdown

@methodmissing methodmissing commented Jun 13, 2016

References issue onelogin#8

Gist of the problem:

Gist of the fixes:

  • Explicitly require openssl on extension init and let OpenSSL::Cipher and OpenSSL::OpenSSLError be references to the constants defined in ext/openssl instead.
  • Fixed misuse of the Ruby convention of modules being prefixed with m and classes prefixed with c ( VALUE mOSSLCipher -> VALUE cOSSLCipher )
  • Support the TypedData system with the secret sauce being the RTYPEDDATA_TYPE(obj) macro for parity for the typed data definition that the aead extension reads with the one that OpenSSL::Cipher allocates objects with.
  • That way we're not setting ourselves up for potential fails on another upgrade with the proposed hacky fix in bytecaster@70a7405

@pallan @csfrancis @jnormore

@pallan
Copy link
Copy Markdown

pallan commented Jun 13, 2016

My C is rusty but this looks okay to me

Comment thread ext/openssl/cipher/aead/aead.c Outdated
rb_define_method(mOSSLCipher, "verify", ossl_cipher_verify, 0);
rb_require("openssl");
VALUE cOSSLCipher = rb_path2class("OpenSSL::Cipher");
VALUE eOSSLError = rb_path2class("OpenSSL::OpenSSLError");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is not being used.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Killed - thx.

@csfrancis
Copy link
Copy Markdown

LGTM 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants