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

Prevent loading all non-Bundler gems #10695

Merged
merged 5 commits into from Feb 26, 2021
Merged

Prevent loading all non-Bundler gems #10695

merged 5 commits into from Feb 26, 2021

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Feb 24, 2021

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This prevents random user-installed gems from breaking brew in various ways, including but not limited to:

Prior discussion in #7681.

@BrewTestBot
Copy link
Member

Review period will end on 2021-02-25 at 18:08:29 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 24, 2021
@Bo98
Copy link
Member Author

Bo98 commented Feb 25, 2021

I cannot reproduce that rubocop-ast error. Nor can I think of anything that could cause it.

It should be in the $LOAD_PATH:

$:.unshift "#{path}/../#{ruby_engine}/#{ruby_version}/gems/rubocop-ast-1.4.1/lib"

and therefore unaffected by any GEM_PATH changes.

Library/Homebrew/load_path.rb Show resolved Hide resolved
@@ -7,7 +7,20 @@

$LOAD_PATH.push HOMEBREW_LIBRARY_PATH.to_s

require "utils/gems"
Copy link
Member

Choose a reason for hiding this comment

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

I'd be tempted to move this require to brew.rb instead. This file is also used by rubocops.rb.

@MikeMcQuaid
Copy link
Member

I cannot reproduce that rubocop-ast error. Nor can I think of anything that could cause it.

Bootsnap? The Library/Homebrew/rubocops.rb load_path require?

@Bo98
Copy link
Member Author

Bo98 commented Feb 25, 2021

Bootsnap?

I don't think that actually is being used on Linux:

(ENV["HOMEBREW_PROCESSOR"] == "Intel")

HOMEBREW_PROCESSOR being "Intel" is a macOS-specific quirk.

The Library/Homebrew/rubocops.rb load_path require?

Call path appears to be brew bottle. It doesn't happen in my Linux docker though.

@Bo98
Copy link
Member Author

Bo98 commented Feb 25, 2021

I think I figured it out - we (intentionally) remove non-existent directories from the $LOAD_PATH. But because we Homebrew.install_bundler_gems! mid-run, some of the gitignored gems won't be in the $LOAD_PATH on first install. Removing the "exists" check causes this horrible bug again though:

/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/vendor/portable-ruby/2.6.3_2/lib/ruby/2.6.0/rubygems/core_ext/kernel_require.rb:54: [BUG] Segmentation fault at 0x0000000000000018

I'll play around with it.

@MikeMcQuaid
Copy link
Member

I don't think that actually is being used on Linux:

(ENV["HOMEBREW_PROCESSOR"] == "Intel")

HOMEBREW_PROCESSOR being "Intel" is a macOS-specific quirk.

Whoops, should revert that as it's not intentional.

@Bo98 Bo98 force-pushed the gem_path branch 2 times, most recently from b9bc39c to 9efd67a Compare February 25, 2021 16:29
@Bo98
Copy link
Member Author

Bo98 commented Feb 25, 2021

The last commit should fix the Ruby crash issue once and for all.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

# randomly segfaults on Linux with portable-ruby.
it "tests a given Formula", :integration_test, :needs_macos do
would be good to remove the comment and :needs_macos. This is looking good, though.

Library/Homebrew/formula_assertions.rb Show resolved Hide resolved
Library/Homebrew/test/support/lib/utils/ruby.rb Outdated Show resolved Hide resolved
@BrewTestBot
Copy link
Member

Review period ended.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 25, 2021
@Bo98 Bo98 force-pushed the gem_path branch 2 times, most recently from 9f786ab to d998a51 Compare February 25, 2021 23:33
@Bo98
Copy link
Member Author

Bo98 commented Feb 26, 2021

Enabling that test immediately finds a bug! Fixed that now.

(I've also made the brew test command print the stacktrace to stderr instead of stdout - it makes the integration test for it have much better output when it fails)

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks again @Bo98, looks like this is uncovering a bunch of 🐛 too. If the scope here is getting a little large feel free to extract any of it into a separate PR e.g. just the minitest changes.

Library/Homebrew/test/support/lib/utils/ruby.rb Outdated Show resolved Hide resolved
Library/Homebrew/extend/ENV/shared.rb Show resolved Hide resolved
We are migrating away from using system gems, and we already have minitest in our dependency tree, so we might as well use it.
Portable Ruby crashes if the $LOAD_PATH gets too big.
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks great 👍🏻

@Bo98 Bo98 merged commit a65c5d6 into Homebrew:master Feb 26, 2021
@Bo98 Bo98 deleted the gem_path branch February 26, 2021 21:35
@carlocab
Copy link
Member

Related? Homebrew/discussions#953

@Bo98
Copy link
Member Author

Bo98 commented Feb 27, 2021

We do not ship a rake script (though the gem was there) with portable Ruby, so that's half intentional.

I suspect the user is actually wanting to use system Ruby however, which would make sense for formula build scripts specifically. This probably broke because GEM_PATH and GEM_HOME is bleeding into formulae.

So we can either:

  • Not use those envs and use alternative Gem API.
  • Reset those ENVs in a build environment.

I'll try out the former just now because it looks interesting.

@Bo98
Copy link
Member Author

Bo98 commented Feb 27, 2021

#10726

@reitermarkus
Copy link
Member

Is there a way now to allow specific Gems which are not in the Gemfile? The cask review action broke because it cannot load the git_diff gem.

@Bo98
Copy link
Member Author

Bo98 commented Feb 28, 2021

I have a fix for Homebrew.install_gem! here: #10735

For more complex scenarios, bundler still works (homebrew-formula-analytics).

@Bo98
Copy link
Member Author

Bo98 commented Feb 28, 2021

Merged now:

$ brew irb
==> Interactive Homebrew Shell
Example commands available with: `brew irb --examples`
irb(main):001:0> Homebrew.install_gem! "git_diff"
==> Installing 'git_diff' gem
Fetching git_diff-0.4.3.gem
irb(main):002:0> require "git_diff"
=> true
irb(main):003:0> defined? GitDiff
=> "constant"

Also works on subsequent runs - Homebrew.install_gem! will still set up the path of gems already installed.

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Mar 31, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Mar 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants