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

utils/gems: always ensure default Gems are in GEM_PATH. #7681

Merged
merged 1 commit into from
Jun 3, 2020
Merged

utils/gems: always ensure default Gems are in GEM_PATH. #7681

merged 1 commit into from
Jun 3, 2020

Conversation

MikeMcQuaid
Copy link
Member

Fixes #7608.

  • 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 tests with your changes locally?

@MikeMcQuaid MikeMcQuaid merged commit 5fb158f into Homebrew:master Jun 3, 2020
@MikeMcQuaid MikeMcQuaid deleted the gem_default_dir branch June 3, 2020 09:28
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 29, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 29, 2020
@Bo98
Copy link
Member

Bo98 commented Feb 23, 2021

Thoughts on reverting this (I think rubocop no longer hard requires did_you_mean) and potentially applying Homebrew.setup_gem_environment! all the time, instead of only some dev commands?

Every week there's a report of someone with some gem installed in /Library/Ruby/Gems reporting their brew is broken. I think it'll save us a lot of headache if we limit gem usage to anything in-built (as in, not just in the default gem location - actually in-built into Ruby) and anything we pull through bundler. If there's a default gem we absolutely need (I don't think there's any) then we can always just vendor it ourselves.

@MikeMcQuaid
Copy link
Member Author

Thoughts on reverting this (I think rubocop no longer hard requires did_you_mean) and potentially applying Homebrew.setup_gem_environment! all the time, instead of only some dev commands?

We disable rubygems by default so I don't think we should enable this all the time. If we can achieve the same thing without enabling rubygems (or our disabling is not working/does not provide any startup speedup): fair enough.

Every week there's a report of someone with some gem installed in /Library/Ruby/Gems reporting their brew is broken. I think it'll save us a lot of headache if we limit gem usage to anything in-built (as in, not just in the default gem location - actually in-built into Ruby) and anything we pull through bundler.

I'm fine with this.

If there's a default gem we absolutely need (I don't think there's any) then we can always just vendor it ourselves.

I'm not fine with this.


I'm fine with reverting this PR if the issue it previously fixed no longer occurs.

@Bo98
Copy link
Member

Bo98 commented Feb 23, 2021

We disable rubygems by default

We require "rubygems" for all users in global.rb. Isn't that effectively enabling it by default?

@MikeMcQuaid
Copy link
Member Author

MikeMcQuaid commented Feb 23, 2021

We require "rubygems" for all users in global.rb. Isn't that effectively enabling it by default?

It seems so. This didn't always use to be the case. You can probably tweak

# Disable Ruby options we don't need. RubyGems provides a decent speedup.
RUBY_DISABLE_OPTIONS="--disable=gems,did_you_mean,rubyopt"
else
# Don't disable did_you_mean for developers as it's useful.
RUBY_DISABLE_OPTIONS="--disable=gems,rubyopt"
too.

@Bo98
Copy link
Member

Bo98 commented Feb 23, 2021

I tweaked things and got it to a state where the only thing in the path was Bundler's gem path and applied that to all runs.

I even went one step further and switched out the require method to bypass RubyGems', so the gem loader is effectively disabled. This is what Bundler non-standalone mode has always done, as well standalone mode after Bundler 2.2.7 - Bundler wasn't meant to have allowed other gems from being loaded in standalone mode, but that bug went unnoticed for a while.

This worked quite well. Though I did find one thing relying on a system gem, and that was test-unit. Seems like RubyGems hasn't actually been disabled for some time now - it's impossible to have used test-unit without some gem loader.

@MikeMcQuaid
Copy link
Member Author

@Bo98 That sounds great!

This worked quite well. Though I did find one thing relying on a system gem, and that was test-unit. Seems like RubyGems hasn't actually been disabled for some time now - it's impossible to have used test-unit without some gem loader.

Interesting. Could be a good candidate for a vendored dependency.

@Bo98
Copy link
Member

Bo98 commented Feb 24, 2021

I think so.

We can also use minitest which is already in our Gemfile.lock, but is currently gitignored. Will require a few aliases for backwards compatibility (see what Active Support do here: https://github.com/rails/rails/blob/291a3d2ef29a3842d1156ada7526f4ee60dd2b59/activesupport/lib/active_support/test_case.rb#L145-L159).

I'm fine with either way. (I don't actually know which one is preferred nowadays - both are system gems.)

@MikeMcQuaid
Copy link
Member Author

I'm fine with either way. (I don't actually know which one is preferred nowadays - both are system gems.)

I think we need it for end users for brew test (unless we pull in bundler gems for that which would be reasonable for a dev-cmd).

@Bo98
Copy link
Member

Bo98 commented Feb 24, 2021

I was more asking about what's preferred between vendoring test-unit, or just using minitest which is already in our Gemfile. But yeah that's a valid question too: ungitignore or just install_bundler_gems!.

@Bo98
Copy link
Member

Bo98 commented Feb 24, 2021

Actually, after the bootsnap changes, we are now running install_bundler_gems! for anyone who has ever run a dev-cmd.

@MikeMcQuaid
Copy link
Member Author

I was more asking about what's preferred between vendoring test-unit, or just using minitest which is already in our Gemfile. But yeah that's a valid question too: ungitignore or just install_bundler_gems!.

I think just install_bundler_gems!. I don't have any preference between test-unit or minitest.

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.

Usused variable crashes RuboCop in brew style
3 participants