Use prepended modules instead of `undef` for OS-specific code #18305

dduugg · 2024-09-11T02:27:31Z

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?

Relates to #17998

Switches from using undef, which results in (suppressed) sorbet errors, with prepended modules.

The latest proposal on the thread suggested creating an interface with three implementations, rather than using the generic-OS version in the default implementation, with methods undefined in OS-specific code. In building that out, I noticed that there wasn't a consistent, common interface among the OS variations, specifically in the case of Formula.

Thus, I've landed on OS extensions written as modules, which are then prepended to be used ahead of the generic implementations. (Note that in some cases, the modules are prepended to the singleton class, which admittedly gets pretty esoteric.)

dduugg · 2024-09-11T02:28:31Z

Library/Homebrew/cli/args.rb

@@ -19,9 +19,6 @@ class Args < OpenStruct
      sig { returns(T::Array[String]) }
      attr_reader :options_only, :flags_only

-      # undefine tap to allow --tap argument
-      undef tap


IIUC, it shouldn't be necessary to use undef to override a method in a subclass

dduugg · 2024-09-11T02:29:53Z

Library/Homebrew/cli/parser.rb

@@ -24,6 +24,9 @@ class Parser
      }.freeze, T::Hash[Symbol, String])
      private_constant :ArgType, :HIDDEN_DESC_PLACEHOLDER, :SYMBOL_TO_USAGE_MAPPING

+      sig { returns(Args) }
+      attr_reader :args


i found this to be preferred to doing leaps with @args in ParserLinux (also, seems reasonable since args are frozen after parsing).

carlocab

Nice

MikeMcQuaid

This approach looks good and makes sense to me, nice work!

I wonder if there's any way to avoid the need for the require at the bottom of the existing class? That'd be nice if possible.

MikeMcQuaid · 2024-09-11T08:18:33Z

Library/Homebrew/extend/os/linux/cleanup.rb

@@ -2,8 +2,10 @@
 # frozen_string_literal: true

 module Homebrew
-  class Cleanup
-    undef use_system_ruby?
+  module CleanupLinux


Suggested change

module CleanupLinux

module Linux::Cleanup

or

Suggested change

module CleanupLinux

module Cleanup::Linux

or

Suggested change

module CleanupLinux

module Cleanup::LinuxExtensions

or maybe even

Suggested change

module CleanupLinux

module OS::Linux::Cleanup

OS::Linux::Cleanup most closely matches the path, let's go with that one

Welp, one issue with introducing OS into the name space is that breaks resolution of e.g. OS.linux? under Homebrew, including in outside repos, e.g. https://github.com/Homebrew/homebrew-test-bot/blob/cb38bc3/lib/test_bot.rb#L63

We could alias ::OS to point to Homebrew::OS, but wanted to do a sanity check before pursuing this route any further.

I couldn't get a simple alias to work, but this does:

module Homebrew module OS class << self extend Forwardable def_delegators '::OS', :mac?, :linux?, :kernel_version, :kerneal_name, :unsupported_configuration? end end end

We'd presumably then deprecate top-level ::OS, given the desire to move everything under the Homebrew namespace.

We'd presumably then deprecate top-level ::OS, given the desire to move everything under the Homebrew namespace.

Would that deprecate OS.linux? and OS.mac?? If so, then that would break a lot of code, and not just in Homebrew.

OS::Linux::Cleanup most closely matches the path, let's go with that one

@dduugg sounds good, thanks!

I think having a top-level OS even just as an alias to Homebrew::OS would be good. As @carlocab mentioned, it's been an incredibly heavily used public API for a long time to the point that I'd probably rather never deprecate/disable/remove it but keep around the alias indefinitely (like we do with MacOS to OS::Mac).

If the alias isn't possible: feel free to leave this as-is and avoid the Homebrew namespace (or do so just for the extend modules)

Library/Homebrew/extend/os/linux/formula.rb

Library/Homebrew/extend/os/mac/cleaner.rb

dduugg · 2024-09-11T18:14:54Z

This approach looks good and makes sense to me, nice work!

I wonder if there's any way to avoid the need for the require at the bottom of the existing class? That'd be nice if possible.

Yes, that would be very nice, but right now sorbet/sorbet#5025 requires the prepend to come after the methods being replaced.

dduugg · 2024-09-13T19:25:14Z

Library/Homebrew/rubocops/move_to_extend_os.rb

@@ -9,7 +9,7 @@ class MoveToExtendOS < Base
        MSG = "Move `OS.linux?` and `OS.mac?` calls to `extend/os`."

        def_node_matcher :os_check?, <<~PATTERN
-          (send (const nil? :OS) {:mac? | :linux?})
+          (send (const {nil? cbase} :OS) {:mac? | :linux?})


This resolves true negatives when using ::OS (see added tests below).

dduugg marked this pull request as draft September 11, 2024 02:27

dduugg commented Sep 11, 2024

View reviewed changes

carlocab approved these changes Sep 11, 2024

View reviewed changes

MikeMcQuaid reviewed Sep 11, 2024

View reviewed changes

dduugg force-pushed the no-undefs branch from 7d9bcfc to 75369c9 Compare September 13, 2024 19:23

dduugg commented Sep 13, 2024

View reviewed changes

dduugg force-pushed the no-undefs branch from 75369c9 to 2ef6464 Compare September 13, 2024 19:35

dduugg added 4 commits September 13, 2024 12:46

Stop suppressing undef errors

0c446ed

Replace undef with prepended modules

1984f77

Makes args readable

b133e91

Fix namespacing

197447c

dduugg force-pushed the no-undefs branch from 2ef6464 to 197447c Compare September 13, 2024 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use prepended modules instead of `undef` for OS-specific code #18305

Use prepended modules instead of `undef` for OS-specific code #18305

dduugg commented Sep 11, 2024 •

edited

Loading

dduugg Sep 11, 2024 •

edited

Loading

dduugg Sep 11, 2024 •

edited

Loading

carlocab left a comment

MikeMcQuaid left a comment

MikeMcQuaid Sep 11, 2024

dduugg Sep 13, 2024

dduugg Sep 13, 2024

dduugg Sep 15, 2024

carlocab Sep 17, 2024

MikeMcQuaid Sep 17, 2024

dduugg commented Sep 11, 2024

dduugg Sep 13, 2024

Use prepended modules instead of undef for OS-specific code #18305

Are you sure you want to change the base?

Use prepended modules instead of undef for OS-specific code #18305

Conversation

dduugg commented Sep 11, 2024 • edited Loading

dduugg Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

dduugg Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

carlocab left a comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid Sep 11, 2024

Choose a reason for hiding this comment

dduugg Sep 13, 2024

Choose a reason for hiding this comment

dduugg Sep 13, 2024

Choose a reason for hiding this comment

dduugg Sep 15, 2024

Choose a reason for hiding this comment

carlocab Sep 17, 2024

Choose a reason for hiding this comment

MikeMcQuaid Sep 17, 2024

Choose a reason for hiding this comment

dduugg commented Sep 11, 2024

dduugg Sep 13, 2024

Choose a reason for hiding this comment

Use prepended modules instead of `undef` for OS-specific code #18305

Use prepended modules instead of `undef` for OS-specific code #18305

dduugg commented Sep 11, 2024 •

edited

Loading

dduugg Sep 11, 2024 •

edited

Loading

dduugg Sep 11, 2024 •

edited

Loading