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

Remove ActiveSupport dependency #16190

Closed
1 task done
dduugg opened this issue Nov 7, 2023 · 13 comments · Fixed by #16510
Closed
1 task done

Remove ActiveSupport dependency #16190

dduugg opened this issue Nov 7, 2023 · 13 comments · Fixed by #16510
Assignees
Labels
features New features in progress Maintainers are working on this outdated PR was locked due to age

Comments

@dduugg
Copy link
Member

dduugg commented Nov 7, 2023

Verification

Provide a detailed description of the proposed feature

Remove the activesupport gem dependency. For simple components with downstream dependencies, we can directly incorporate the monkey-patches. Other components should just be replaced with Ruby methods (much of the good stuff in ActiveSupport has been incorporated into recent Ruby releases anyway).

What is the motivation for the feature?

ActiveSupport has been a source of performance issues (as it's arguably intended for server lifecycles, not CLI tools). It lacks some type optimizations that sorbet knows about with core methods. Monkey-patches can make for subtle bugs, as libraries and Ruby core may disagree over which versions they consider canonical.

How will the feature be relevant to at least 90% of Homebrew users?

Maybe some tiny perf improvements (though possibly a less user-friendly interface at the edges as well).

What alternatives to the feature have been considered?

We could maintain the status quo, and be vigilant about keeping out the less performant components (e.g. inflections)

@dduugg dduugg added the features New features label Nov 7, 2023
@dduugg dduugg self-assigned this Nov 7, 2023
@MikeMcQuaid
Copy link
Member

Remove the activesupport gem dependency. For simple components with downstream dependencies, we can directly incorporate the monkey-patches. Other components should just be replaced with Ruby methods (much of the good stuff in ActiveSupport has been incorporated into recent Ruby releases anyway).

Agreed, this makes sense to me. It might make sense to have a list of the stuff we're (still) using so we can discuss each one and whether it makes sense to:

  • keep it around and support it indefinitely as a Homebrew DSL/monkeypatch
  • keep it around for now but deprecate it as a Homebrew DSL/monkeypatch
  • remove it now because it's not used

@dduugg if you have the bandwidth and desire: would you be able to create such a list? 🙇🏻

@dduugg
Copy link
Member Author

dduugg commented Nov 7, 2023

@MikeMcQuaid yep, i can own this own this ticket, if you're ok with a slow rollout as bandwidth allows me (probably circa end-of-year)

@MikeMcQuaid
Copy link
Member

@dduugg Works for me, thanks!

@Bo98
Copy link
Member

Bo98 commented Nov 21, 2023

For any deprecations, it would be good to get those by mid-December (minor bump will ideally be before the holidays, but no earlier than the 11th). Not a blocker though, as we can instead punt the deprecations until April (minor bump for Ruby 3.2 maybe?).

Assuming it's still around by then, I'm also considering moving what is left of ActiveSupport to the mechanize-style gem vendoring instead of bundler vendoring it for Ruby 3.1. To do this however, we first need to remove our dependency on active_support/core_ext/object/blank, which depends on concurrent-ruby. We were planning on reimplementing this ourselves I think so we at least already had a plan for that one.

@MikeMcQuaid
Copy link
Member

Assuming it's still around by then, I'm also considering moving what is left of ActiveSupport to the mechanize-style gem vendoring instead of bundler vendoring it for Ruby 3.1

I'd really rather keep everything in Bundler we can as it makes version management way easier.

We were planning on reimplementing this ourselves I think so we at least already had a plan for that one.

Yup 👍🏻

I'd think we can probably reimplement most other stuff by that timescale 🤞🏻

@dduugg
Copy link
Member Author

dduugg commented Nov 26, 2023

Assuming it's still around by then, I'm also considering moving what is left of ActiveSupport to the mechanize-style gem vendoring instead of bundler vendoring it for Ruby 3.1. To do this however, we first need to remove our dependency on active_support/core_ext/object/blank, which depends on concurrent-ruby. We were planning on reimplementing this ourselves I think so we at least already had a plan for that one.

Happy to take care of this separately from the refactoring of the rest of the ActiveSupport code. PR incoming.

@Bo98
Copy link
Member

Bo98 commented Dec 11, 2023

4.2.0 changes are landing now. Let me know if there's anything here that should land with 4.2.0 and needs help with.

@Bo98
Copy link
Member

Bo98 commented Dec 13, 2023

One question is what we do with rubocop-rails, given that has a ActiveSupport dependency on its own. Going through what we have enabled:

  • Rails/ActiveSupportAliases - not needed if we remove ActiveSupport as these methods won't exist anymore and will be errors instead
  • Rails/Blank - only NilOrEmpty/NotNilAndNotEmpty is useful (only a few lines) - other bits can be replaced by more standard Style/InverseMethods and Style/InvertibleUnlessCondition
  • Rails/CompactBlank - TBC if we're going to support this
  • Rails/DelegateAllowBlank - not needed, not supported
  • Rails/DurationArithmetic - no longer supported: Remove active_support Time extensions #14502
  • Rails/ExpandedDateRange - ditto, no longer supported
  • Rails/Inquiry - not needed if we remove ActiveSupport as these methods won't exist anymore and will be errors instead
  • Rails/NegateInclude - can be replaced by Style/InverseMethods
  • Rails/PluralizationGrammar - linked to time, no longer supported
  • Rails/Presence - probably the most useful one here
  • Rails/Present - only NilOrEmpty/NotNilAndNotEmpty is useful (only a few lines) - other bits can be replaced by more standard Style/InverseMethods and Style/InvertibleUnlessCondition
  • Rails/RelativeDateConstant - linked to time, no longer supported
  • Rails/SafeNavigation - no longer supported: Remove use of ActiveSupport try #16184
  • Rails/SafeNavigationWithBlank - useful, extremely small (couple lines)
  • Rails/StripHeredoc - not needed if we remove ActiveSupport as these methods won't exist anymore and will be errors instead
  • Rails/ToFormattedS - will not be supported

@MikeMcQuaid
Copy link
Member

One question is what we do with rubocop-rails, given that has a ActiveSupport dependency on its own.

Feels like by the time we end up vendoring ActiveSupport changes we should probably do the same with the specific cops that make sense. Has always been a bit of a cludge in here.

Agreed with your takes about what's useful/not.

Copy link

github-actions bot commented Jan 4, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Jan 4, 2024
@MikeMcQuaid MikeMcQuaid added in progress Maintainers are working on this and removed stale No recent activity labels Jan 4, 2024
@dduugg
Copy link
Member Author

dduugg commented Jan 8, 2024

I poked around for where we're still relying on ActiveSupport and came up with the table below. Since we mostly require these from global.rb, there's always the possibility that we have downstream dependencies in formulae (as i learned in #16189).

ActiveSupport core_ext path Method Approx. # of uses in Library/Homebrew Add'l comments
array/access.rb Array#second 26 Trivial (one-line) implementation
Array#third 3 Trivial (one-line) implementation
Array#second_to_last 1 Trivial (one-line) implementation
enumerable.rb Enumerable#index_by 2 both in formula_installer
Enumerable#exclude <= 49 Trivial (one-line) implementation. Collides with String#exclude?, hence the upper bound
Enumerable#compact_blank 3 Trivial (one-line) implementation
file/atomic.rb File.atomic_write 1 in Pathname#atomic_write
hash/deep_merge.rb Hash#deep_merge 2 in dev-cmd/bottle.rb and dev-cmd/pr-upload.rb
hash/deep_transform_values.rb Hash#deep_transform_values 1 Formulary.load_formula_from_api
Hash#deep_transform_values! 1 Formulary.load_formula_from_api
hash/keys.rb Hash#assert_valid_keys 7 all in Library/Homebrew/cask, seems like we could replace with type checking
Hash#deep_transform_keys 1 Formulary.load_formula_from_api
Hash#deep_transform_keys! 1 Formulary.load_formula_from_api
Hash#deep_stringify_keys 1 GitHubPackages#validate_schema
Hash#deep_symbolize_keys 1 Cask::CaskLoader::FromAPILoader#load
object/deep_dup.rb Object#deep_dup 1 Cask::Artifact::AbstractArtifact.new
object/duplicable.rb Object#duplicable 0 Dependency of Object#deep_dup that we don't need
string/exclude.rb String#exclude? <= 49 Trivial (one-line) implementation. Collides with Array#exclude?, hence the upper bound
string/filters.rb String#remove 1 Removed in #16426
string/indent.rb String#indent 8 test files & Utils::AST

I think this is useful for folks to identify which methods we want to vendor, and which we want to refactor away, but let me know if y'all were wanting other metrics in order to guide us from here.

@Bo98
Copy link
Member

Bo98 commented Jan 8, 2024

Array#second et all

I've found these arbitrary a bit but do get why they are sometimes used. I'd like to see alternatives to it. For example instead of .split("/").second we could do .partition("/").last, or instead of args.named.first and args.named.second do arg1, arg2, = args.named. "second" etc is often non-descriptive of why it's special and you usually need to read more code to follow what's happening.

This is however used in homebrew-core and at least one third-party tap so should be considered public API and go through any relevant deprecation process.


In terms of the rest:

  • *#exclude? definitely considered public API at this point
  • File.atomic_write isn't used externally but our wrapper Pathname#atomic_write is public API. Could just implement it directly in the Pathname wrapper
  • Enumerable#compact_blank used by the qt-creator cask and at least two third-party taps so while it is low usage it probably needs to be considered public API.
  • Hash#deep_stringify_keys and Hash#deep_symbolize_keys is used by Homebrew/bundle, but I'm not aware of any other usage externally.

@MikeMcQuaid
Copy link
Member

Thanks @dduugg!

I think this is useful for folks to identify which methods we want to vendor, and which we want to refactor away, but let me know if y'all were wanting other metrics in order to guide us from here.

I think anything with a trivial one-line implementation and/or >=3 uses by us or any third-party tap usage should be vendored.

Also agreed with @Bo98's assessment of what should be considered public API/not.

@github-actions github-actions bot added the outdated PR was locked due to age label Mar 1, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
features New features in progress Maintainers are working on this outdated PR was locked due to age
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants