Remove ActiveSupport from runtime #16463

dduugg · 2024-01-10T20:35:38Z

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 #16190

Removes ActiveSupport from the brew runtime (it will still exist as a transitive dependency of rubocop-rails, for now).

I recommend to squash commits on merge (or I can force-push a squashed commit, not sure what the merge options are here).

I've used just the parts of the ActiveSupport files that we use, with some light modifications (including adding type signatures, and pulling in upstream changes, for which I've added comments). Another option could be to just vendor the existing dependencies, as we do with mechanize. However, ActiveSupport sometimes undergoes refactors and dependency changes, which might be a headache to keep up with (for instance, this refactor of Hash#deep_merge! to introduce another DeepMergeable module).

I haven't added new tests for this functionality, but don't want to dismiss their importance/value (rails uses a different framework, preventing this from being a simple c/p operation). Looks like there a just few spots without coverage though (including some error paths that may be hard to plausibly simulate). LMK if more are desirable.

Library/Homebrew/tab.rb

Library/Homebrew/download_strategy.rb

MikeMcQuaid

Looks good so far!

Library/Homebrew/utils/gems.rb

Library/Homebrew/extend/array.rb

.gitignore

dduugg · 2024-01-12T23:39:58Z

Library/Homebrew/cask/artifact/abstract_uninstall.rb

@@ -6,6 +6,7 @@
 require "utils/user"
 require "cask/artifact/abstract_artifact"
 require "cask/pkg"
+require "extend/hash/keys"


I scoped these to the files that used them, rather than in global

dduugg · 2024-01-13T00:17:58Z

Library/Homebrew/cli/parser.rb

-        @is_dev_cmd = cmd_location.absolute_path.start_with?(Commands::HOMEBREW_DEV_CMD_PATH)
+        end.fetch(1)
+        @command_name = T.must(cmd_location.label).chomp("_args").tr("_", "-")
+        @is_dev_cmd = T.must(cmd_location.absolute_path).start_with?(Commands::HOMEBREW_DEV_CMD_PATH)


Some minor changes that are required when adding sigs to vendored methods

dduugg · 2024-01-13T00:20:25Z

Library/Homebrew/extend/array.rbi

+# typed: strict
+
+class Array
+  sig { returns(T.nilable(Elem)) }


Elem is an upstream RBI creation that isn't available at runtime, so these need to be in RBI files. (And similarly for Enumerable::Enum, Hash::K, Hash::V, etc. below…)

Library/Homebrew/extend/enumerable.rb

dduugg · 2024-01-13T00:29:27Z

Library/Homebrew/extend/enumerable.rbi

+end
+
+class Hash
+  sig { returns(T::Hash[Hash::K, Hash::V]) }


I would like to use a runtime sig with returns(T.self_type), but that's a bug: sorbet/sorbet#7586

dduugg · 2024-01-13T00:31:39Z

Library/Homebrew/extend/object/deep_dup.rb

      end
    end
    hash
  end
 end
+
+class Module


This is sourced from the current upstream implementation

dduugg · 2024-01-13T00:32:41Z

Library/Homebrew/extend/object/duplicable.rb

+
+require "singleton"
+
+module Singleton


another upstream addition

dduugg · 2024-01-13T00:36:05Z

Library/Homebrew/livecheck/strategy/gnu.rb

          ^https?://
          (?:(?:[^/]+?\.)*gnu\.org/(?:gnu|software)/(?<project_name>[^/]+)/
          |(?<project_name>[^/]+)\.gnu\.org/?$)
-        }ix.freeze
+        }ix.freeze, Regexp)


Strict typing and this change somehow resolve a spurious type error on line 42 ¯\(ツ)/¯

dduugg · 2024-01-13T00:38:01Z

Library/Homebrew/standalone/sorbet.rb

@@ -2,6 +2,7 @@
 # frozen_string_literal: true

 require "sorbet-runtime"
+require "extend/module"


This is mostly unrelated, but as of this change, the extension only extends Module to include T::Sig, so IMO it makes the most sense to live here rather than global.

apainintheneck · 2024-01-13T08:36:06Z

Library/Homebrew/download_strategy.rb

-        components[:query] = URI.decode_www_form(uri_query).map(&:second)
+        components[:query] = URI.decode_www_form(uri_query).map { _2 }


I haven't seen this pattern in the wild. Very cool!

Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>

dduugg · 2024-01-19T21:40:22Z

Ready for review

apainintheneck · 2024-01-20T23:12:38Z

Library/Homebrew/extend/enumerable.rb

+      each { |elem| result[yield(elem)] = elem }
+      result
+    else
+      T.unsafe(self).to_enum(:index_by) { T.unsafe(self).size if respond_to?(:size) }


This is really ugly.

I'm honestly not entirely sure what's going on here. There's only two usages of index_by so if we're wanting to keep it then we don't really need to keep the nil-block case which has a behaviour that's undocumented anyway.

The whole thing can probably be simpified to each_with_object({}) { |elem, hash| hash[yield(elem)] = elem }, or replace the two usages with that.

I agree that index_by can probably be removed entirely and this code inlined given there's only two usages.

Library/Homebrew/extend/enumerable.rb

Bo98 · 2024-01-21T03:48:53Z

Looks ok, though some cleanup would be nice afterwards. deep_dup and friends can probably also go - duplicable? seems annoying to keep on top of and I'm not fully convinced its one current usage of deep_dup in Cask::Artifact::AbstractArtifact is really necessary anyway (or if so - it can probably be handled better). But given this PR is already quite large I'm ok deferring any further cleanup to a separate PR to reduce risk.

MikeMcQuaid

Thanks @dduugg! As this is a bit of a handful to review, I think it's probably worth merging soon (or as-is, will leave that up-to-you).

I think index_by and perhaps deep_dup could be dropped but that could be a follow-up PR if necessary.

MikeMcQuaid · 2024-01-22T13:03:50Z

Library/Homebrew/extend/enumerable.rb

+      each { |elem| result[yield(elem)] = elem }
+      result
+    else
+      T.unsafe(self).to_enum(:index_by) { T.unsafe(self).size if respond_to?(:size) }


I agree that index_by can probably be removed entirely and this code inlined given there's only two usages.

MikeMcQuaid · 2024-01-22T13:04:30Z

Library/Homebrew/extend/object/deep_dup.rb

@@ -12,6 +13,7 @@ class Object
  #
  #   object.instance_variable_defined?(:@a) # => false
  #   dup.instance_variable_defined?(:@a)    # => true
+  sig { returns(T.self_type) }
  def deep_dup


As @Bo98 mentioned: I think we can drop this.

Naïvely swapping out deep_dup for dup in abstract_artifact leads to test failures in test/cask/cask_spec.rb – could i trouble someone with more context to take a deeper (pun intended) look into this one?

dduugg · 2024-01-22T18:14:25Z

I believe that I've addressed all the feedback except for deep_dup, PTAL

MikeMcQuaid

Thanks again @dduugg!

dduugg added the in progress Maintainers are working on this label Jan 10, 2024

dduugg marked this pull request as draft January 10, 2024 20:36

dduugg changed the title ~~No active support~~ Remove ActiveSupport from runtime Jan 10, 2024

dduugg force-pushed the no-active-support branch 2 times, most recently from b76155c to 4267a7a Compare January 10, 2024 20:57

Bo98 reviewed Jan 10, 2024

View reviewed changes

Library/Homebrew/tab.rb Outdated Show resolved Hide resolved

Library/Homebrew/download_strategy.rb Outdated Show resolved Hide resolved

MikeMcQuaid reviewed Jan 11, 2024

View reviewed changes

Library/Homebrew/utils/gems.rb Outdated Show resolved Hide resolved

MikeMcQuaid reviewed Jan 11, 2024

View reviewed changes

Library/Homebrew/utils/gems.rb Outdated Show resolved Hide resolved

bevanjkay reviewed Jan 11, 2024

View reviewed changes

Library/Homebrew/extend/array.rb Show resolved Hide resolved

dduugg force-pushed the no-active-support branch 7 times, most recently from 3dfce75 to 07f98a9 Compare January 13, 2024 00:26

dduugg commented Jan 13, 2024

View reviewed changes

apainintheneck reviewed Jan 13, 2024

View reviewed changes

dduugg and others added 12 commits January 19, 2024 13:34

Remove ActiveSupport Array access core extensions

08e12b2

Extend Module when requiring sorbet

5546f77

Copy enumerable file

f8de5df

Update from upstream, remove unused methods

b60d951

Add ActiveSupport String#exclude? to extend/

2e21eff

Update Library/Homebrew/utils/gems.rb

7ae087b

Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>

Fix style/type violations

c02520f

Add Array#fourth to resolve cask errors

79e2379

Add File.atomic_write to extend/

8e9d294

Add Hash#deep_merge to extend/

0cdd4ee

Add Hash#deep_transform_values to extend/

a87cdf0

brew vendor-gems: commit updates.

14f8a08

Add hash/keys to extend/

f6c7eb7

dduugg force-pushed the no-active-support branch from 07f98a9 to 4cd6c51 Compare January 19, 2024 21:39

dduugg removed the in progress Maintainers are working on this label Jan 19, 2024

dduugg marked this pull request as ready for review January 19, 2024 21:40

dduugg mentioned this pull request Jan 20, 2024

Vendor remaining Rails cops, remove ActiveSupport #16510

Merged

7 tasks

apainintheneck approved these changes Jan 20, 2024

View reviewed changes

MikeMcQuaid approved these changes Jan 22, 2024

View reviewed changes

dduugg force-pushed the no-active-support branch 2 times, most recently from ccb1c03 to 7036c9d Compare January 22, 2024 18:45

dduugg added 11 commits January 22, 2024 10:52

Add Object#deep_dup to extend/

8efe73b

brew vendor-gems: commit updates.

fa7b8ff

Resolve 'Error: can't add a new key into hash during iteration'

1790904

Improve atomic_write sig

4c25250

revert bundler/setup changes

44e6b48

concise

66aff2a

cleanup

53bba9c

brew style --fix

df140b3

Inline index_by

2f8ad2f

Fix compact_blank style

1a40468

Use each_with_object

e574904

dduugg force-pushed the no-active-support branch from 7036c9d to e574904 Compare January 22, 2024 18:52

MikeMcQuaid approved these changes Jan 22, 2024

View reviewed changes

MikeMcQuaid merged commit 86e1c8a into Homebrew:master Jan 22, 2024
25 checks passed

dduugg deleted the no-active-support branch January 22, 2024 19:31

github-actions bot added the outdated PR was locked due to age label Mar 2, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ActiveSupport from runtime #16463

Remove ActiveSupport from runtime #16463

dduugg commented Jan 10, 2024 •

edited

MikeMcQuaid left a comment

dduugg Jan 12, 2024

dduugg Jan 13, 2024

dduugg Jan 13, 2024

dduugg Jan 13, 2024

dduugg Jan 13, 2024

dduugg Jan 13, 2024

dduugg Jan 13, 2024 •

edited

dduugg Jan 13, 2024 •

edited

apainintheneck Jan 13, 2024

dduugg commented Jan 19, 2024

apainintheneck Jan 20, 2024

Bo98 Jan 21, 2024

MikeMcQuaid Jan 22, 2024

Bo98 commented Jan 21, 2024 •

edited

MikeMcQuaid left a comment

MikeMcQuaid Jan 22, 2024

MikeMcQuaid Jan 22, 2024

dduugg Jan 22, 2024

dduugg commented Jan 22, 2024

MikeMcQuaid left a comment

		components[:query] = URI.decode_www_form(uri_query).map(&:second)
		components[:query] = URI.decode_www_form(uri_query).map { _2 }

Remove ActiveSupport from runtime #16463

Remove ActiveSupport from runtime #16463

Conversation

dduugg commented Jan 10, 2024 • edited

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dduugg Jan 13, 2024 • edited

Choose a reason for hiding this comment

dduugg Jan 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dduugg commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bo98 commented Jan 21, 2024 • edited

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dduugg commented Jan 22, 2024

MikeMcQuaid left a comment

Choose a reason for hiding this comment

dduugg commented Jan 10, 2024 •

edited

dduugg Jan 13, 2024 •

edited

dduugg Jan 13, 2024 •

edited

Bo98 commented Jan 21, 2024 •

edited