Fix `$LOAD_PATH` in rake and ext_conf builder #6490

ntkme · 2023-03-16T03:20:25Z

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

Installing gem with rake based extension which depends on a native gem (e.g. psych) that has more than one copies on the system (one comes default with ruby installation and one user installation) with the system rubygems can fail in a very strange way.

Reproduction: sinatra/sinatra#1911 (comment)
Explanation: sinatra/sinatra#1911 (comment)

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

Per previous PR related to this issue: #4165

I'm not sure using this -I parameter here is a good idea, so since I'm not sure I'm just keeping things as they are for now and fixing the bug.

Removing this flag caused one of the existing tests to be broken: https://github.com/rubygems/rubygems/actions/runs/4433225457/jobs/7778058872

Therefore, this PR removes it conditionally.

Make sure the following tasks are checked

Describe the problem / feature
Write tests for features and bug fixes (existing tests should be enough)
Write code to solve the problem
Make sure you follow the current code style and write meaningful commit messages without tags

deivid-rodriguez · 2023-03-16T20:56:15Z

Hi! Thanks so much! You could've just moved on by upgrading RubyGems, but you fixed the root cause 😍.

Could you add a test for this?

ntkme · 2023-03-16T21:14:26Z

Could you add a test for this?

I'd be happy to. The case is so specific that I'm not sure how to write it / where to put it:

It has to be a default installation of rubygems that comes with ruby, as gem update --system will change the behavior so the bug is no longer reproducible.
It has to install a second copy of a dependency gem (e.g. psych) that already comes with ruby and the version of second installation needs to be different enough so that the linking between the ruby code and native ext could break.
The Rakefile in the gem being installed needs to use that the dependency gem.

@deivid-rodriguez If you can give me some pointer to maybe a similar test case we have currently, I can try come up with a test case.

deivid-rodriguez · 2023-03-16T21:27:59Z

I was thinking of something similar to the one added by #4165, but this seems much more specific, indeed.

For this kind tricky tests I normally use realworld testing like the commands run in .github/workflows/install-rubygems.yml. But that requires manually installing RubyGems first, which does not have the same structure as the RubyGems bundled with a pristine version of Ruby.

How are you testing that the fix works? Directly editing the RubyGems copy included with Ruby?

Anyways, yeah, too hard to get this tested, I think it's fine as is.

ntkme · 2023-03-16T21:31:42Z

How are you testing that the fix works? Directly editing the RubyGems copy included with Ruby?

Exactly, I start with a ruby 3.0 container which I used in the reproduction, manually apply the patch to the default rubygems, and confirmed that it fixed the problem.

deivid-rodriguez · 2023-03-16T21:45:46Z

This is a ruby-core specific thing, not a RubyGems thing, so I found a little discrepancy. On my laptop:

$ find /Users/deivid/.asdf/installs/ruby/3.2.1/lib/ruby/3.2.0/ -type d -depth 1 -name 'arm*'
/Users/deivid/.asdf/installs/ruby/3.2.1/lib/ruby/3.2.0//arm64-darwin22

$ ruby -e 'puts Gem::Platform.local'                               
arm64-darwin-22

$ ruby -e 'puts RUBY_PLATFORM'                                     
arm64-darwin22

So it sounds like we want the latter.

ntkme · 2023-03-16T21:48:33Z

Oh wow. On linux it was the same. Let me update the PR.

ntkme · 2023-03-16T22:00:42Z

Umm, there is actually two different possibilities:

https://github.com/ruby/ruby/blob/5825d7d4a1a53ba84909a01679f4dfab63fa9739/loadpath.c#L25-L27
https://github.com/ruby/ruby/blob/5825d7d4a1a53ba84909a01679f4dfab63fa9739/loadpath.c#L48-L50
https://github.com/ruby/ruby/blob/5825d7d4a1a53ba84909a01679f4dfab63fa9739/loadpath.c#L86-L90

Depends on whether RUBY_PLATFORM_CPU is defined, it will be either RUBY_ARCH or RUBY_THINARCH...

ntkme · 2023-03-16T22:10:07Z

RbConfig::CONFIG['rubyarchdir'] this is it

ntkme · 2023-03-16T23:45:56Z

@deivid-rodriguez Sorry for lots of back and forth changing the PR. I think now I have the most clean and robust solution:

If rubygems' install location is not present in the default $LOAD_PATH, we add it. - This happens in the rubygems development and test.
If rubygems' install location is present in $LOAD_PATH, just don't add it. - This happens for normal users. The $LOAD_PATH order won't change and we're good.

hsbt · 2023-03-17T05:43:59Z

@ntkme Wait to push for re-run tests. We reached to limit of macOS jobs.

see #6504

hsbt · 2023-03-17T05:52:24Z

Can you rebase this from 7e5fbea? Thanks.

deivid-rodriguez · 2023-03-17T16:00:32Z

@ntkme Nice, your solution makes sense to me! Should we move this common logic to the base ext builder?

ntkme · 2023-03-17T17:06:33Z

@deivid-rodriguez Refactored. I also noticed that there is some inconsistency on the usage of Gem.ruby as it maybe a quoted string. The format of Gem.ruby shall be interpreted as a shell command string instead of a path (the documentation is somewhat misleading)... Therefore, we should shellsplit it. - Today, it already uses shellsplit in ExtConfBuilder but not in RakeBuilder.

rubygems/lib/rubygems.rb

Lines 791 to 802 in 06f8526

    
           ## 
        
           # The path to the running Ruby interpreter. 
        
           def self.ruby 
        
             if @ruby.nil? 
        
               @ruby = RbConfig.ruby 
        
               @ruby = "\"#{@ruby}\"" if @ruby =~ /\s/ 
        
             end 
        
             @ruby 
        
           end

deivid-rodriguez

Awesome, thanks for your work!

ntkme · 2023-03-17T21:48:48Z

Looks like merge queue check failed due to flakiness of #6512. Not sure if any action is required.

deivid-rodriguez · 2023-03-20T12:18:59Z

I'm merging the PR fixing the flaky failure and then will merge this one. No action needed @ntkme, thanks for your work!

Fix $LOAD_PATH in rake and ext_conf builder (cherry picked from commit 653ee82)

To address rubygems/rubygems#6611 via rubygems/rubygems#6490 that has been released as 3.4.9 https://github.com/rubygems/rubygems/blob/master/CHANGELOG.md#349--2023-03-20

Ref: ruby#647 Ref: rubygems/rubygems#6490 Older rubygems are executing `extconf.rb` with a broken `$LOAD_PATH` causing the `json` gem native extension to be loaded with the stdlib version of the `.rb` files. This fails with ``` json/common.rb:82:in `initialize': wrong number of arguments (given 1, expected 0) (ArgumentError) ``` Since this is just for `extconf.rb` we can probably just accept that extra argument and ignore it. The bug was fixed in rubygems 3.4.9 / 2023-03-20

Ref: #647 Ref: rubygems/rubygems#6490 Older rubygems are executing `extconf.rb` with a broken `$LOAD_PATH` causing the `json` gem native extension to be loaded with the stdlib version of the `.rb` files. This fails with ``` json/common.rb:82:in `initialize': wrong number of arguments (given 1, expected 0) (ArgumentError) ``` Since this is just for `extconf.rb` we can probably just accept that extra argument and ignore it. The bug was fixed in rubygems 3.4.9 / 2023-03-20

Ref: ruby/json#647 Ref: rubygems/rubygems#6490 Older rubygems are executing `extconf.rb` with a broken `$LOAD_PATH` causing the `json` gem native extension to be loaded with the stdlib version of the `.rb` files. This fails with ``` json/common.rb:82:in `initialize': wrong number of arguments (given 1, expected 0) (ArgumentError) ``` Since this is just for `extconf.rb` we can probably just accept that extra argument and ignore it. The bug was fixed in rubygems 3.4.9 / 2023-03-20 ruby/json@1f5e849fe0

ntkme mentioned this pull request Mar 16, 2023

Add sass support via sass-embedded sinatra/sinatra#1911

Merged

ntkme changed the title ~~Remove rubygems path in rake builder~~ Make sure native extension are loaded correctly in rake builder Mar 16, 2023

ntkme changed the title ~~Make sure native extension are loaded correctly in rake builder~~ Make sure native extensions are loaded correctly in rake builder Mar 16, 2023

ntkme changed the title ~~Make sure native extensions are loaded correctly in rake builder~~ Make sure native extensions are loaded correctly in rake and ext_conf builder Mar 17, 2023

ntkme added 4 commits March 17, 2023 09:49

Make sure native extensions are loaded correctly in rake builder

6276852

Make sure native extensions are loaded correctly in ext_conf builder

d095b47

Move duplicated logic to parent class

e82ed20

Refactor and fix usage of Gem.ruby

9db5e94

ntkme changed the title ~~Make sure native extensions are loaded correctly in rake and ext_conf builder~~ Fix $LOAD_PATH in rake and ext_conf builder Mar 17, 2023

deivid-rodriguez approved these changes Mar 17, 2023

View reviewed changes

deivid-rodriguez added this pull request to the merge queue Mar 17, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 17, 2023

deivid-rodriguez added the rubygems: bug fix label Mar 20, 2023

deivid-rodriguez added this pull request to the merge queue Mar 20, 2023

deivid-rodriguez changed the title ~~Fix $LOAD_PATH in rake and ext_conf builder~~ Fix $LOAD_PATH in rake and ext_conf builder Mar 20, 2023

deivid-rodriguez merged commit 653ee82 into rubygems:master Mar 20, 2023

ntkme deleted the remove-rubygems-path-in-rake-builder branch March 20, 2023 16:04

deivid-rodriguez added a commit that referenced this pull request Mar 20, 2023

Merge pull request #6490 from ntkme/remove-rubygems-path-in-rake-builder

edc5b3f

Fix $LOAD_PATH in rake and ext_conf builder (cherry picked from commit 653ee82)

deivid-rodriguez mentioned this pull request Apr 17, 2023

Having Psych 5 installed raises an error during another gem's C-extension installation when parsing YAML #6611

Closed

tombruijn mentioned this pull request Apr 17, 2023

Compiling native extension fails when having psych >= 5 appsignal/appsignal-ruby#904

Closed

yahonda mentioned this pull request Sep 28, 2023

Use gvenzl/oracle-free:latest to run Oracle Database 23c Free rsim/oracle-enhanced#2352

Merged

flavorjones mentioned this pull request Oct 9, 2023

Native version doesn't build since version 1.6.6 and Ruby 3.1.4 sparklemotion/sqlite3-ruby#411

Closed

gravitystorm mentioned this pull request Nov 22, 2023

Error installing mini_racer: ERROR: Failed to build gem native extension rubyjs/mini_racer#289

Closed

dentarg mentioned this pull request Dec 28, 2023

Fixup 'rubygems: latest' ruby/setup-ruby#551

Merged

This was referenced Oct 25, 2024

Installing to vendor/bundle appears to be broken ruby/json#647

Closed

json 2.7.3 has broken my test environment ruby/json#646

Closed

casperisfine mentioned this pull request Oct 25, 2024

Workaround rubygems $LOAD_PATH bug ruby/json#648

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `$LOAD_PATH` in rake and ext_conf builder #6490

Fix `$LOAD_PATH` in rake and ext_conf builder #6490

ntkme commented Mar 16, 2023 •

edited

Loading

deivid-rodriguez commented Mar 16, 2023

ntkme commented Mar 16, 2023 •

edited

Loading

deivid-rodriguez commented Mar 16, 2023

ntkme commented Mar 16, 2023

deivid-rodriguez commented Mar 16, 2023

ntkme commented Mar 16, 2023

ntkme commented Mar 16, 2023 •

edited

Loading

ntkme commented Mar 16, 2023

ntkme commented Mar 16, 2023 •

edited

Loading

hsbt commented Mar 17, 2023 •

edited

Loading

hsbt commented Mar 17, 2023

deivid-rodriguez commented Mar 17, 2023

ntkme commented Mar 17, 2023

deivid-rodriguez left a comment

ntkme commented Mar 17, 2023

deivid-rodriguez commented Mar 20, 2023

Fix $LOAD_PATH in rake and ext_conf builder #6490

Fix $LOAD_PATH in rake and ext_conf builder #6490

Conversation

ntkme commented Mar 16, 2023 • edited Loading

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

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

Make sure the following tasks are checked

deivid-rodriguez commented Mar 16, 2023

ntkme commented Mar 16, 2023 • edited Loading

deivid-rodriguez commented Mar 16, 2023

ntkme commented Mar 16, 2023

deivid-rodriguez commented Mar 16, 2023

ntkme commented Mar 16, 2023

ntkme commented Mar 16, 2023 • edited Loading

ntkme commented Mar 16, 2023

ntkme commented Mar 16, 2023 • edited Loading

hsbt commented Mar 17, 2023 • edited Loading

hsbt commented Mar 17, 2023

deivid-rodriguez commented Mar 17, 2023

ntkme commented Mar 17, 2023

deivid-rodriguez left a comment

Choose a reason for hiding this comment

ntkme commented Mar 17, 2023

deivid-rodriguez commented Mar 20, 2023

Fix `$LOAD_PATH` in rake and ext_conf builder #6490

Fix `$LOAD_PATH` in rake and ext_conf builder #6490

ntkme commented Mar 16, 2023 •

edited

Loading

ntkme commented Mar 16, 2023 •

edited

Loading

ntkme commented Mar 16, 2023 •

edited

Loading

ntkme commented Mar 16, 2023 •

edited

Loading

hsbt commented Mar 17, 2023 •

edited

Loading