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

Skip debase extconf.rb on non-CRuby #106

Merged
merged 2 commits into from
Dec 19, 2023
Merged

Conversation

eregon
Copy link
Contributor

@eregon eregon commented Sep 15, 2023

* Otherwise:
  ruby-debug-ide-VERSION/lib/ruby-debug-ide.rb:10:in `<top (required)>': uninitialized constant Debugger (NameError)
@eregon
Copy link
Contributor Author

eregon commented Dec 1, 2023

Ping, I'd like to merge this.

@@ -1,5 +1,10 @@
if defined?(RUBY_ENGINE) && 'rbx' == RUBY_ENGINE
require 'debase/rbx'
elsif defined?(RUBY_ENGINE) && RUBY_ENGINE == 'truffleruby'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This backend is not supposed to run with truffleruby

Copy link
Contributor Author

@eregon eregon Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's why this code is skipping it.
It's still valuable that the gem installs, otherwise e.g. ruby-debug-ide cannot be installed either and it leads to more workarounds in Gemfiles (and it's not always possible, e.g., if a gem depends on ruby-debug-ide and that gem is in the Gemfile, and it cannot be skipped because it has other dependencies, which we found in some apps).

Copy link
Contributor Author

@eregon eregon Dec 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the changes in this file are problematic, I can remove them or redo them differently.
The most critical part is that the debase C extensions are not attempted to be compiled on non-CRuby (because that would just fail).

@eregon
Copy link
Contributor Author

eregon commented Dec 18, 2023

@hurricup Could you merge this?
See for instance oracle/truffleruby#3359, that needs both ruby-debug-ide and debase to no-op on TruffleRuby and other non-CRuby.

@hurricup
Copy link
Contributor

I'm sorry, I still don't understand what are you trying to achieve? This gem is not intended to be used with truffleruby as well as ruby-debug-ide.

@eregon
Copy link
Contributor Author

eregon commented Dec 18, 2023

what are you trying to achieve?

To let the gem install fine on truffleruby, so there is no need to tweak the Gemfile.
In some cases it is (AFAIK) not possible to tweak the Gemfile, we had such a case internally at Oracle:
Suppose there is a gem my-dev-tools that depend on ruby-debug-ide (but not only, also other gems which do work on truffleruby) and that's part of the Gemfile.
There is no solution there, because removing my-dev-tools would remove too much and break the app/tests.
Using something like gem 'debase', platforms: :mri does not work.
The only solution is to let ruby-debug-ide (and its dependencies as known by RubyGems, so that's debase) install fine on TruffleRuby and noop at runtime.

It's a fairly common practice to let gems install on TruffleRuby/JRuby/etc even when they are not supported, so at least they don't break bundle install. Some examples are stackprof, bootsnap, etc.

elsif defined?(RUBY_ENGINE) && RUBY_ENGINE == 'truffleruby'
require "debase/version"

Debugger = Module.new
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for this is explained in the commit message: 06bdb3c

But maybe it's the wrong place to do this and instead we should change lib/ruby-debug-ide.rb to noop earlier.

@hurricup hurricup merged commit 551b63d into ruby-debug:master Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants