Skip to content

Conversation

@jonbarlo
Copy link
Contributor

@jonbarlo jonbarlo commented Jul 31, 2025

What was the end-user or developer problem that led to this PR?

There is a comment asking for this change to take place specially because we already missed to do it for previous versions.

Closes issue mentioned here

What is your fix for the problem, implemented in this PR?

just changing the default value to false as requested, i also added deprecation warning message for people using the previous versions to know the default value should not be true any longer (not sure what exactly to do though)

Make sure the following tasks are checked

@jonbarlo jonbarlo mentioned this pull request Jul 31, 2025
21 tasks
@hsbt
Copy link
Member

hsbt commented Jul 31, 2025

@jonbarlo Thanks! Could you look https://github.com/rubygems/rubygems/actions/runs/16638467937/job/47094377312?pr=8889 and fix it?

@deivid-rodriguez I would like to apply this default value change at RubyGems 4.0. It may break the some gems that are relied to locate lib/foo.so via ffi gem. But It should be fixed to use Gem::Specification#extension_dir instead of relative path.

How do you think about that?

@hsbt
Copy link
Member

hsbt commented Jul 31, 2025

I'm okay to extend this change to RubyGems 4.1 and only warn at RubyGems 4.0.

@deivid-rodriguez
Copy link
Contributor

Yes, I'm good with moving this forward, I just want to have a second look at it and try make this fully backwards compatible, perhaps warning affected gems first about what they should be doing instead of what they are actually doing.

@jonbarlo
Copy link
Contributor Author

jonbarlo commented Jul 31, 2025

@deivid-rodriguez

Looking at the current implementation, I think we can improve this by:

  • Detecting affected gems: When a gem is being installed, check if it's using the old pattern (extensions in lib/) and warn about it
  • Providing specific guidance: Instead of just a generic deprecation warning, give specific advice about what the gem should do
  • Warning at the right time: Warn when the gem is actually being processed, not just when the config is loaded

let me add some more warning messages in there

@jonbarlo
Copy link
Contributor Author

hi @hsbt i have removed the extra space so that lint error should be fixed, i already pushed the change, thanks!

@jonbarlo
Copy link
Contributor Author

jonbarlo commented Aug 2, 2025

@deivid-rodriguez @hsbt hi guys i have added some more warnings, mind checking? not sure if this is too much, and not sure about the exact text but you will get the idea, I am adding more specific message explaining what's deprecated, guidance on what users should do, detecting which gem is being affected, warning only when extensions are actually being installed in lib/

@deivid-rodriguez
Copy link
Contributor

deivid-rodriguez commented Aug 6, 2025

@jonbarlo While I think explicit deprecation of Gem.install_extension_in_lib == true is great, I believe the issue is more that some gems actually rely on the compiled extension being placed in the lib folder to work, so once we toggle the default, they will break. If I recall correctly, the problem was gems using ffi-compiler.

So my idea was to:

  • Figure out what ffi-compiler should be doing instead to play nice with RubyGems.
  • Propose that change to ffi-compiler.
  • Figure out how to warn other cases we may be missing, and cases where an old version of ffi-compiler not including our fix is used.

One potential idea I just had is to temporarily keep installing a wrapper file into lib that warns and then loads the extension from the proper location.

@jonbarlo
Copy link
Contributor Author

jonbarlo commented Aug 11, 2025

@deivid-rodriguez Ah, now I understand... i will check and reserach about the ffi-compiler and will check about the wrapper idea

@deivid-rodriguez
Copy link
Contributor

Cool, let me close this PR then, I think it's premature to deprecate Gem.install_extension_in_lib == true when we have not even yet changed the default.

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.

3 participants