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

Correct module lookup when including ffi-module #912

Merged
merged 1 commit into from Aug 14, 2021

Conversation

danielevans
Copy link

The inclusion of @ioquatix's ffi-module gem to a project causes errors loading other ffi gems (such as rbnacl) due to the definition of FFI::Module which makes .kind_of?(Module) equivalent to .kind_of?(FFI::Module).

This change simply changes that lookup to ensure that it is checking against the top-level Module constant.

@ioquatix
Copy link
Contributor

Hmm, I wonder if I should rename the gem. I didn't realise this issue at the time.

@eregon
Copy link
Collaborator

eregon commented Aug 13, 2021

The change is fine (make the check a bit more robust) but IMHO the much bigger issue is to define a class or module named Module which isn't the ::Module of Ruby core.
So +1000 to rename FFI::Module to something else.

@ioquatix
Copy link
Contributor

FFI::SomethingElse

But seriously, I agree, this was kind of an oversight on my part.

That being said, I've encountered this issue in other places.

Sometimes Ruby constant lookup causes pain if someone introduces a new module into an existing one.

@eregon eregon merged commit 7c70b82 into ffi:master Aug 14, 2021
@larskanis
Copy link
Member

Do we need a new release due to this issue?

@danielevans
Copy link
Author

danielevans commented Aug 15, 2021

There is no rush to release the fix from my perspective, the issue is easily worked around and the ffi-module gem is not widely depended upon.

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

Successfully merging this pull request may close these issues.

None yet

4 participants