Bye `byebug`, hi `debug`! #17010

issyl0 · 2024-04-02T23:10:19Z

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?

Fixes Fix byebug when running tests #16708.
Byebug - added in 2020 by a GSoC student for test debugging - was not working and also old and hadn't had any commits in a while, also only reported support for Ruby 2. The debug gem, from the ruby/debug repository, supports our version of Ruby and is "official".
A slight issue was it has a dependency on psych, which smarter people than me worked out that we don't support - but if we pin it to an earlier version then it works.
Add a bunch of dependencies to .gitignore etc, otherwise this diff was massive and we don't need +80k lines.

But the (minimal, basic) testing I did seems to drop me at a prompt that I can use.

❯ git diff
diff --git a/Library/Homebrew/test/migrator_spec.rb b/Library/Homebrew/test/migrator_spec.rb
index 87fadd5e95..db4700810a 100644
--- a/Library/Homebrew/test/migrator_spec.rb
+++ b/Library/Homebrew/test/migrator_spec.rb
@@ -69,6 +69,8 @@ RSpec.describe Migrator do
       tab.source["tap"] = "homebrew/core"
       tab.write

+      binding.break
+
       expect do
         described_class.new(new_formula, "oldname")
       end.to raise_error(Migrator::MigratorDifferentTapsError)

issyl0 at pictor in /opt/homebrew on bye-byebug
❯ brew tests --only=migrator --debug
Randomized with seed 59158
1 process for 1 spec, ~ 1 spec per process
.==> Relinking newname
.==> Unlinking oldname
...==> Moving oldname versions to /private/tmp/homebrew-tests-20240403-85464-3uogqr/cellar/newname
....==> Migrating formula oldname to newname
==> Unlinking oldname
==> Moving oldname versions to /private/tmp/homebrew-tests-20240403-85464-3uogqr/cellar/newname
==> Relinking newname
....[67, 76] in ~/migrator_spec.rb
    67|       tab = Tab.empty
    68|       tab.tabfile = HOMEBREW_CELLAR/"oldname/0.1/INSTALL_RECEIPT.json"
    69|       tab.source["tap"] = "homebrew/core"
    70|       tab.write
    71|
=>  72|       binding.break
    73|
    74|       expect do
    75|         described_class.new(new_formula, "oldname")
    76|       end.to raise_error(Migrator::MigratorDifferentTapsError)
=>#0    block in <top (required)> (3 levels) at ~/migrator_spec.rb:72
  #1    [C] BasicObject#instance_exec at /opt/homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/rspec-core-3.13.0/lib/rspec/core/example.rb:263
  # and 68 frames (use `bt' command for all frames)
(rdbg@/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/bin/rspec#85464) p tab
(rdbg@/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/bin/rspec#85464) p    # command(rdbg@/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/bin/rspec#85464) p     # command t    # command ta    # command tab    # command(rdbg@/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/bin/rspec#85464) p tab    # command
=> #<Tab:0x0000000107156be0 @Aliases=[], @Arch=nil, @built_as_bottle=false, @built_on={"os"=>"Macintosh", "os_version"=>"macOS 14", "cpu_family"=>"arm_firestorm_icestorm"}, @Compiler=:clang, @homebrew_version="4.2.16-55-gc8f60ec-dirty", @installed_as_dependency=false, @installed_on_request=false, @loaded_from_api=false, @poured_from_bottle=false, @runtime_dependencies=nil, @source={"path"=>nil, "tap"=>"homebrew/core", "tap_git_head"=>nil, "spec"=>"stable", "versions"=>{"stable"=>nil, "head"=>nil, "version_scheme"=>0}}, @source_modified_time=0, @stdlib=nil, @tabfile=#<Pathname:/private/tmp/homebrew-tests-20240403-85464-3uogqr/cellar/oldname/0.1/INSTALL_RECEIPT.json>, @time=nil, @unused_options=[], @used_options=[]>
(rdbg@/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/bin/rspec#85464)

Bo98 · 2024-04-03T01:36:23Z

A slight issue was it has a dependency on psych, which smarter people than me worked out that we don't support - but if we pin it to an earlier version then it works.

Hmm, I wonder if instead of the debug dependency we can do (in standalone/init.rb just before the stuff at line 42):

if RUBY_PATH.to_s.include?("/vendor/portable-ruby/")
  $LOAD_PATH.unshift "#{RbConfig::CONFIG["rubylibprefix"]}/gems/#{RbConfig::CONFIG["ruby_version"]}/gems/debug-1.6.3/lib"
end

(I'll figure out a way to do this more magically rather than a hardcoded version - maybe on the Portable Ruby side)

This does limit it to Portable Ruby only, but for such a niche developer-only feature I'm OK with that given it avoids juggling more native gems.

apainintheneck · 2024-04-03T06:23:25Z

@issyl0 You got to it before I did. This will be helpful when working on #16895.

@Bo98 If we could handle the debug dependency on the portable Ruby side, that would be ideal.

MikeMcQuaid · 2024-04-03T07:23:32Z

Great work @issyl0 and good idea!

This does limit it to Portable Ruby only, but for such a niche developer-only feature I'm OK with that given it avoids juggling more native gems.

Same, as long as it gets a nice error message 👍🏻

dduugg

I realize this in draft mode, I hope the early feedback doesn't come off as pestering.
Love to see this being investigated!

Library/Homebrew/dev-cmd/tests.rb

Library/Homebrew/test/spec_helper.rb

issyl0 · 2024-04-04T22:06:29Z

(I'll figure out a way to do this more magically rather than a hardcoded version - maybe on the Portable Ruby side.)

@Bo98 Did you figure this out? Or can we ship this and do that in the future? Not that it's urgent, of course.

Bo98 · 2024-04-04T22:36:56Z

well, hardcoded version will be fine as it won't change until we update Portable Ruby - the debug gem ships with our Portable Ruby

issyl0 · 2024-04-05T09:29:20Z

Ahh yes I see what you mean. I'll try to get that working later. 👍🏻

issyl0 · 2024-04-10T19:33:12Z

If anyone's got any suggestions for why this doesn't work with the debug from portable Ruby, I'm marking this as non-draft, so please comment and/or push commits!

Bo98 · 2024-04-10T20:11:21Z

Ah in addition to the gems/debug-1.6.3/lib path you will also need extensions/#{RUBY_PLATFORM}/#{ruby_version}-static/debug-1.6.3

issyl0 · 2024-04-10T20:14:31Z

Oooh, because .so is libraries, and those are "extensions"? Thanks again, Bo!

issyl0 · 2024-04-10T20:26:32Z

🤔 I don't have an extensions?

/opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.1.4/lib/ruby/extensions/arm64-darwin20/3.1.0-static/debug-1.6.3: No such file or directory

❯ ls /opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.1.4/lib/ruby/
3.1.0   gems

Bo98 · 2024-04-10T20:27:44Z

portable-ruby/3.1.4/lib/ruby/gems/3.1.0/extensions/

issyl0 · 2024-04-10T20:52:06Z

That's weird...

❯ ls /opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.1.4/lib/ruby/gems/3.1.0/extensions/arm64-darwin20/3.1.0-static/debug-1.6.3
ls: /opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.1.4/lib/ruby/gems/3.1.0/extensions/arm64-darwin20/3.1.0-static/debug-1.6.3: No such file or directory

but

❯ ls /opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.1.4/lib/ruby/gems/3.1.0/extensions/arm64-darwin-20/3.1.0-static           
debug-1.6.3     rbs-2.7.0

So RUBY_PLATFORM is reporting arm64-darwin20 when the path is actually arm64-darwin-20? What am I doing wrong?

I refer once again to the confused dog in the PR description. 😆

Bo98 · 2024-04-10T21:38:57Z

Ah, right. Gem::Platform.local should work instead of RUBY_PLATFORM

issyl0 · 2024-04-10T22:47:11Z

@Bo98 Thanks. Getting there!

Bo98 · 2024-04-17T16:18:24Z

if I add require "debug" to the spec_helper again it gives:
LoadError:
 cannot load such file -- debug

The require in dev-cmd/tests is useless and can go. The one in spec_helper will need to be after the ../global require.

Though we want it to be conditional of --debug being passed (since require "debug" may fail on non-Portable Ruby) so you'll need to pass some tag/variable and make the require conditional on that.

issyl0 · 2024-04-17T21:20:03Z

Thanks for the extra guidance, Bo! I made brew tests --debug set ENV["HOMEBREW_DEBUG"] and then required the "debug" gem in spec_helper.rb based on that. It works!

- Byebug was introduced in [2020](#7577) for hooking into tests for debugging. - It does not work anymore in so far as it does not stop at breakpoints when following the instructions to trigger them in tests.

@Aliases

- This processed that we'd requested a debugger, but didn't drop us into a debugging console until I [stopped the stdin disablement](#16708 (comment)). Usage: ``` ❯ git diff diff --git a/Library/Homebrew/test/migrator_spec.rb b/Library/Homebrew/test/migrator_spec.rb index 87fadd5e95..db4700810a 100644 --- a/Library/Homebrew/test/migrator_spec.rb +++ b/Library/Homebrew/test/migrator_spec.rb @@ -69,6 +69,8 @@ RSpec.describe Migrator do tab.source["tap"] = "homebrew/core" tab.write + binding.break + expect do described_class.new(new_formula, "oldname") end.to raise_error(Migrator::MigratorDifferentTapsError) issyl0 at pictor in /opt/homebrew on bye-byebug ❯ brew tests --only=migrator --debug Randomized with seed 59158 1 process for 1 spec, ~ 1 spec per process .==> Relinking newname .==> Unlinking oldname ...==> Moving oldname versions to /private/tmp/homebrew-tests-20240403-85464-3uogqr/cellar/newname ....==> Migrating formula oldname to newname ==> Unlinking oldname ==> Moving oldname versions to /private/tmp/homebrew-tests-20240403-85464-3uogqr/cellar/newname ==> Relinking newname ....[67, 76] in ~/migrator_spec.rb 67| tab = Tab.empty 68| tab.tabfile = HOMEBREW_CELLAR/"oldname/0.1/INSTALL_RECEIPT.json" 69| tab.source["tap"] = "homebrew/core" 70| tab.write 71| => 72| binding.break 73| 74| expect do 75| described_class.new(new_formula, "oldname") 76| end.to raise_error(Migrator::MigratorDifferentTapsError) =>#0 block in <top (required)> (3 levels) at ~/migrator_spec.rb:72 #1 [C] BasicObject#instance_exec at /opt/homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/rspec-core-3.13.0/lib/rspec/core/example.rb:263 # and 68 frames (use `bt' command for all frames) (rdbg@/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/bin/rspec#85464) p tab (rdbg@/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/bin/rspec#85464) p # command(rdbg@/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/bin/rspec#85464) p # command t # command ta # command tab # command(rdbg@/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/bin/rspec#85464) p tab # command => #<Tab:0x0000000107156be0 @Aliases=[], @arch=nil, @built_as_bottle=false, @built_on={"os"=>"Macintosh", "os_version"=>"macOS 14", "cpu_family"=>"arm_firestorm_icestorm"}, @Compiler=:clang, @homebrew_version="4.2.16-55-gc8f60ec-dirty", @installed_as_dependency=false, @installed_on_request=false, @loaded_from_api=false, @poured_from_bottle=false, @runtime_dependencies=nil, @source={"path"=>nil, "tap"=>"homebrew/core", "tap_git_head"=>nil, "spec"=>"stable", "versions"=>{"stable"=>nil, "head"=>nil, "version_scheme"=>0}}, @source_modified_time=0, @StdLib=nil, @tabfile=#<Pathname:/private/tmp/homebrew-tests-20240403-85464-3uogqr/cellar/oldname/0.1/INSTALL_RECEIPT.json>, @time=nil, @unused_options=[], @used_options=[]> (rdbg@/opt/homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/bin/rspec#85464) ```

- This is cleaner than vendoring a whole bunch of new gems and pinning `psych`. - Thanks for the pointer, Bo! - It doesn't work, though? ``` ❯ brew tests --only=migrator --debug Error: cannot load such file -- debug/debug.so Warning: Removed Sorbet lines from backtrace! Rerun with `--verbose` to see the original backtrace /opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.1.4/lib/ruby/gems/3.1.0/gems/debug-1.6.3/lib/debug/frame_info.rb:16:in `require' /opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.1.4/lib/ruby/gems/3.1.0/gems/debug-1.6.3/lib/debug/frame_info.rb:16:in `rescue in <module:DEBUGGER__>' /opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.1.4/lib/ruby/gems/3.1.0/gems/debug-1.6.3/lib/debug/frame_info.rb:13:in `<module:DEBUGGER__>' /opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.1.4/lib/ruby/gems/3.1.0/gems/debug-1.6.3/lib/debug/frame_info.rb:3:in `<top (required)>' /opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.1.4/lib/ruby/gems/3.1.0/gems/debug-1.6.3/lib/debug/session.rb:31:in `require_relative' /opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.1.4/lib/ruby/gems/3.1.0/gems/debug-1.6.3/lib/debug/session.rb:31:in `<top (required)>' /opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.1.4/lib/ruby/gems/3.1.0/gems/debug-1.6.3/lib/debug.rb:3:in `require_relative' /opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.1.4/lib/ruby/gems/3.1.0/gems/debug-1.6.3/lib/debug.rb:3:in `<top (required)>' /opt/homebrew/Library/Homebrew/dev-cmd/tests.rb:48:in `require' /opt/homebrew/Library/Homebrew/dev-cmd/tests.rb:48:in `run' /opt/homebrew/Library/Homebrew/brew.rb:89:in `<main>' ```

- This doesn't work still, but now for new reasons. ``` NoMethodError: undefined method `b' for #<Binding:0x0000000107a7e088> binding.b ^^ # ./test/migrator_spec.rb:72:in `block (3 levels) in <top (required)>' ``` and if I add `require "debug"` to the spec_helper again it gives: ``` LoadError: cannot load such file -- debug ``` but, doing a `require "debug"; binding.b` in `migrator_spec` _does_ work. Where is the require coming from where it works some of the time but not through all of the layers we have?

- This will cause the "debug" gem to be required in `spec_helper.rb`, so we can do interactive debugging.

MikeMcQuaid

Looks good to me, thanks @issyl0! Only non-blocking feedback is on the hardcoded debug gem version; feels likely we'll forget to update that when portable Ruby is updated.

MikeMcQuaid · 2024-04-18T09:28:53Z

Library/Homebrew/standalone/init.rb

+  ruby_version = RbConfig::CONFIG["ruby_version"]
+  ruby_path = "#{RbConfig::CONFIG["rubylibprefix"]}/gems/#{ruby_version}"
+
+  $LOAD_PATH.unshift "#{ruby_path}/extensions/#{Gem::Platform.local}/#{ruby_version}-static/debug-1.6.3"


Might be nice to glob or use a Gem method here so the version isn't hardcoded.

dduugg · 2024-04-21T23:37:46Z

Library/Homebrew/test/spec_helper.rb

@@ -38,6 +37,8 @@

 require_relative "../global"

+require "debug" if ENV["HOMEBREW_DEBUG"]


i wonder if we should just do this for any command run with --debug?

Unlike brew tests which is a developer-only command, we ask end users to run --debug and we do not want this behaviour for them. Or if we do then it should catch a LoadError and fail silently.

MikeMcQuaid · 2024-04-22T12:31:36Z

Thanks @issyl0, great work!

issyl0 force-pushed the bye-byebug branch from 545db15 to ee95f99 Compare April 3, 2024 10:39

dduugg reviewed Apr 3, 2024

View reviewed changes

Library/Homebrew/dev-cmd/tests.rb Outdated Show resolved Hide resolved

Library/Homebrew/dev-cmd/tests.rb Show resolved Hide resolved

Library/Homebrew/test/spec_helper.rb Outdated Show resolved Hide resolved

issyl0 force-pushed the bye-byebug branch 2 times, most recently from 8fe8515 to b830b3e Compare April 7, 2024 19:27

issyl0 marked this pull request as ready for review April 10, 2024 19:33

issyl0 force-pushed the bye-byebug branch from e993765 to 169dfa7 Compare April 10, 2024 22:48

issyl0 added 5 commits April 17, 2024 22:24

Remove byebug since it doesn't work in tests anymore

11c1960

- Byebug was introduced in [2020](#7577) for hooking into tests for debugging. - It does not work anymore in so far as it does not stop at breakpoints when following the instructions to trigger them in tests.

Add the debug gem for test debugging

58fbaec

Fix RuboCop and exclude psych from RBI generation

f2a64f3

Don't require "debug" in brew tests - it's in our RSpec config

621e51a

issyl0 force-pushed the bye-byebug branch from d549e33 to ab2d06d Compare April 17, 2024 21:26

issyl0 added 2 commits April 17, 2024 23:26

Set HOMEBREW_DEBUG when running brew tests --debug

4c64193

- This will cause the "debug" gem to be required in `spec_helper.rb`, so we can do interactive debugging.

issyl0 force-pushed the bye-byebug branch from ab2d06d to 4c64193 Compare April 17, 2024 22:28

issyl0 enabled auto-merge April 17, 2024 22:57

MikeMcQuaid approved these changes Apr 18, 2024

View reviewed changes

issyl0 merged commit 151e2fa into master Apr 18, 2024
28 checks passed

issyl0 deleted the bye-byebug branch April 18, 2024 09:30

dduugg reviewed Apr 21, 2024

View reviewed changes

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

github-actions bot locked as resolved and limited conversation to collaborators May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bye `byebug`, hi `debug`! #17010

Bye `byebug`, hi `debug`! #17010

issyl0 commented Apr 2, 2024 •

edited

Loading

Bo98 commented Apr 3, 2024 •

edited

Loading

apainintheneck commented Apr 3, 2024

MikeMcQuaid commented Apr 3, 2024

dduugg left a comment

issyl0 commented Apr 4, 2024

Bo98 commented Apr 4, 2024 •

edited

Loading

issyl0 commented Apr 5, 2024

issyl0 commented Apr 10, 2024

Bo98 commented Apr 10, 2024 •

edited

Loading

issyl0 commented Apr 10, 2024

issyl0 commented Apr 10, 2024

Bo98 commented Apr 10, 2024 •

edited

Loading

issyl0 commented Apr 10, 2024

Bo98 commented Apr 10, 2024

issyl0 commented Apr 10, 2024

Bo98 commented Apr 17, 2024 •

edited

Loading

issyl0 commented Apr 17, 2024 •

edited

Loading

MikeMcQuaid left a comment

MikeMcQuaid Apr 18, 2024

dduugg Apr 21, 2024

Bo98 Apr 22, 2024 •

edited

Loading

MikeMcQuaid commented Apr 22, 2024

		@@ -38,6 +37,8 @@

		require_relative "../global"

		require "debug" if ENV["HOMEBREW_DEBUG"]

Bye byebug, hi debug! #17010

Bye byebug, hi debug! #17010

Conversation

issyl0 commented Apr 2, 2024 • edited Loading

Bo98 commented Apr 3, 2024 • edited Loading

apainintheneck commented Apr 3, 2024

MikeMcQuaid commented Apr 3, 2024

dduugg left a comment

Choose a reason for hiding this comment

issyl0 commented Apr 4, 2024

Bo98 commented Apr 4, 2024 • edited Loading

issyl0 commented Apr 5, 2024

issyl0 commented Apr 10, 2024

Bo98 commented Apr 10, 2024 • edited Loading

issyl0 commented Apr 10, 2024

issyl0 commented Apr 10, 2024

Bo98 commented Apr 10, 2024 • edited Loading

issyl0 commented Apr 10, 2024

Bo98 commented Apr 10, 2024

issyl0 commented Apr 10, 2024

Bo98 commented Apr 17, 2024 • edited Loading

issyl0 commented Apr 17, 2024 • edited Loading

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid Apr 18, 2024

Choose a reason for hiding this comment

dduugg Apr 21, 2024

Choose a reason for hiding this comment

Bo98 Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

MikeMcQuaid commented Apr 22, 2024

Bye `byebug`, hi `debug`! #17010

Bye `byebug`, hi `debug`! #17010

issyl0 commented Apr 2, 2024 •

edited

Loading

Bo98 commented Apr 3, 2024 •

edited

Loading

Bo98 commented Apr 4, 2024 •

edited

Loading

Bo98 commented Apr 10, 2024 •

edited

Loading

Bo98 commented Apr 10, 2024 •

edited

Loading

Bo98 commented Apr 17, 2024 •

edited

Loading

issyl0 commented Apr 17, 2024 •

edited

Loading

Bo98 Apr 22, 2024 •

edited

Loading