Fix "OS is not a class" crash on Ubuntu 23.10

[alichtman]

This seems to resolve the issue raised at discussion #5050. I have not added new tests, but this seems like something that should have tests associated with it!

Without this patch, on Ubuntu 23.10, you experience the following:

$ brew
/usr/lib/ruby/vendor_ruby/os.rb:7:in `<top (required)>': OS is not a class (TypeError)
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/macos_version.rb:168: previous definition of OS was here
        from /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/global.rb:77:in `require'
        from /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/global.rb:77:in `<top (required)>'
        from /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/brew.rb:18:in `require_relative'
        from /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/brew.rb:18:in `<main>'

• 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?

---

[Bo98]

LGTM in principle.

It does break the load path hacking we do the tests do but I'll handle fixing that later today - it probably will just be a case of pre-inserting the library path on the test side.

[MikeMcQuaid]

It does break the load path hacking we do the tests do but I'll handle fixing that later today - it probably will just be a case of pre-inserting the library path on the test side.

@Bo98 you happy to commit that straight to this PR and merge after?

[Bo98]

Yes, I will do that

[Bo98]

Had to make it a bit more complex to work with the test hacks we have in a few places - it now inserts it after any existing Homebrew paths. Outside of tests and certain other developer commands, this is basically the same thing as unshift so it should functionally work the same for you.

[nirvdrum]

@Bo98 Instead of messing around with $LOAD_PATH, can we use require_relative? It's designed to avoid these sorts of issues. Granted, it's a larger changeset, but I'm curious if that's an acceptable solution.

[Bo98]

That's unfortunately a breaking change for taps which are not located relatively.

Library/Homebrew/etc.rb
Library/Taps/homebrew/homebrew-sometap/lib/test.rb

require "etc" inside that tap would become relative_require "../../../../../Homebrew/etc"

A theoretical homebrew_require would make sense, or better still: having a homebrew/ prefix on every Homebrew require (e.g. require "homebrew/etc"). But would be a bit of work to roll out and would require a lengthy transition period.

[Bo98]

Also to clarify: I wasn't interested in only fixing os.rb specifically here as it could happen to any other file we name as there's a ton of other generic names like utils.rb. I didn't even have a os.rb in my vendor_ruby, but did have some other generic files that we were lucky to avoid.

[nirvdrum]

Thanks for the context. I think it's okay for taps to use require and Core to use require_relative. I see that as in keeping with the spirit of require_relative anyway: load files from your own project/repo without involving the $LOAD_PATH. But, if the taps are counting on Homebrew to set up the $LOAD_PATH then that's a non-starter.

Based on your clarification, I get the impression you thought I was asking about changing the few places that do require 'os'. I was asking about a follow-up task to userequire_relative more broadly. It's a much larger changeset, so I'm happy to get an immediate fix in place.

I worry this issue could regress. It's easy to modify the $LOAD_PATH, inadvertently or otherwise, and the error manifests differently on each system making debugging from an issue kind of hard. On the author's machine it may not induce a conflict at all. In this case, the class vs module mismatch was easy to sort out and we were able to reproduce it on a stock Ubuntu system. But, it obviously wasn't failing on other systems. $LOAD_PATH conflicts could be far more subtle and can make for very confusing situations.

To help mitigate that, it might be good to add a spec that verifies putting a conflicting file earlier in the $LOAD_PATH doesn't impact Homebrew's ability to load itself. I looked through the project, but couldn't find a natural place for such a test since it impacts all commands. I asked about require_relative since it would solve the problem semantically, providing a natural guard against this class of error. There are performance and security considerations as well, but I don't think they apply much in this case. But, if it's a bad fit or otherwise breaks things, no worries.

Thank you again for the fix and for the background information. I'll skip the PR to switch to require_relative and keep this info in mind for future contributions. I hadn't realized there'd be an impact on taps like that. I thought maybe the usage of require was just a Ruby 1.8 holdover.