Only do RubyVM patches if class exists

[headius]

This class does not exist in any implementation except CRuby.

I would recommend moving this code somewhere else, like a separate
file loaded only on CRuby or into CRuby itself. For now this
change is sufficient to load the library on other implementations.

[headius]

I looked into the failures.

One of them is due to requiring etc.so which does not exist in JRuby. This should simply require 'etc' since it might be loaded from pure-Ruby or some other way in non-CRuby impls.

One of them is a mismatch in recursive inspect output. The test may be too restrictive, but I will look into why it differs on JRuby.

The rest of the failures are all due to using Ruby 2.7 features. JRuby 9.3.0.0, the latest release, supports Ruby 2.6.8 features. Is it intended that pp can no longer be used by Ruby 2.6 users, less than three years after 2.6 was released and less than three months after Ruby 2.6.8 was released? I think that is a bit unreasonable.

[headius]

I have incorporated backport versions of bind_call and the ruby2_keywords methods to allow pp to work under Ruby 2.6. The latter was copied from https://github.com/ruby/ruby2_keywords, which is not ideal

The sole remaining failure is in the recursive inspect output.

[eregon]

pp is a default gem only since 3.0, so I guess that's why it uses recent features.
The gemspec currently says >= 2.7.

So maybe testing JRuby should only be done when JRuby supports 2.7 or more recent?
We could add truffleruby in CI to check there is no CRuby-specific code used like RubyVM.

I think it's bad if pp which is stdlib/default gem might define (many) ruby2_keywords methods.
It seems to have rather few usages of it, so maybe it can just use if respond_to?(:ruby2_keywords, true) like most code out there needing compat with 2.6 and 2.7.
Maybe pp can depend on the ruby2_keywords gem? But that might be problematic for the CRuby 2.7 which does not include ruby2_keywords currently.

bind_call can probably be replaced by .bind.call or some private helper which uses what is available.

Anyway, it's up to pp maintainers whether they are OK to support 2.6.

[headius]

Maybe pp can depend on the ruby2_keywords gem?

I took a closer look and it's only used in the test, so I modified my commit to define UnboundMethod#bind_call in pp.rb and use the ruby2_keywords gem in testing. Perhaps changing bind_call calls to bind.call would be better than monkey-patching, but I will wait to hear from maintainer about how to proceed.

[headius]

@nurse @hsbt Who can decide the 2.6 vs 2.7 question?

If this gem only supports 2.7, it will not be possible to test on JRuby until we have a 2.7+ release (JRuby 9.4 will support 3.0) or 2.7+ features on JRuby master (requiring a jruby-head or nightly build in CI).

[headius]

FWIW this prevents us using the gem for our 3.0 work, since the RubyVM changes go back before the initial 0.1.0 gem shipped with Ruby 3.0:

dc90655

Can we get something merged in and released soon?

[headius]

Pinging again. Please let me know what I can do to get this merged and released.

[eregon]

I think defining bind_call in a stdlib/default gem is unexpected.
Could you either use a JRuby version (even if head) which has bind_call, or implement bind_call in one of the JRuby releases?
Or define it in tests only with a comment why it's needed.

The rest seems straightforward so I would feel OK to approve and merge it.
Although of course if the maintainer @akr would review, that'd be even better.

[headius]

As with other gem fixes, I'm fine pinning this to 2.7 if it is acceptable that the gem will never work on 2.6.

There's no released version of JRuby that has bind_call and we will not add it to a release that should not have it. Defining the method in the tests might be acceptable for now so we can test against a stable release of JRuby rather than against the unstable work-in-progress on master.

[eregon]

Given the current gemspec specifying >= 2.7, it seems safe to assume the gem is not meant to work on 2.6 (it can't be installed on 2.6). (It's a default gem from CRuby 3.0)
In general, I think lowering the min version is risky as the code might depend on other features available only on 2.7+, and of course it tends to make the code more verbose and harder to maintain (admittedly not a huge deal for pp, but still working around no bind_call won't look good).

There's no released version of JRuby that has bind_call and we will not add it to a release that should not have it.

I have another view on this (IMHO it's harmless to add it, https://github.com/oracle/truffleruby/blob/b6e9f15d97bd1d0c4485a5db81a87297039792ad/doc/contributor/workflow.md#running-specs-for-ruby-31-features) but I'm confident I will not be able to convince you on this.

[eregon]

This looks good to me and it seems completely safe to merge.

[eregon]

(I noticed the added JRuby CI is failing)

[headius]

I repushed the bind_call commit to only define it in the test. All cases pass except for this one, which fails on both CRuby 2.6 and JRuby 9.3 (equivalent to 2.6.8)

$ rake test
Loaded suite /Users/headius/.rvm/gems/ruby-2.6.7/gems/rake-13.0.6/lib/rake/rake_test_loader
Started
.......F
=========================================================================================================================================================================================================================================================================================
Failure: test_withinspect(PPTestModule::PPCycleTest)
/Users/headius/projects/pp/test/test_pp.rb:169:in `test_withinspect'
     166:   def test_withinspect
     167:     a = []
     168:     a << HasInspect.new(a)
  => 169:     assert_equal("[<inspect:[...]>]\n", PP.pp(a, ''.dup))
     170:     assert_equal("#{a.inspect}\n", PP.pp(a, ''.dup))
     171:   end
     172: 
<"[<inspect:[...]>]\n"> expected but was
<"[<inspect:[<inspect:[...]>]>]\n">

I'm confident I will not be able to convince you on this

You are correct. We have had issues from users detecting the presence of a Ruby X.Y feature and using that to assume that other Ruby X.Y features are present. We do not add features to JRuby unless they exist in the equivalent Ruby version, to avoid this confusion. The only exceptions, usually, are for bug fixes that have no simple workaround or otherwise hinder the user experience.

[headius]

(I noticed the added JRuby CI is failing)

This appears to be a behavioral change in inspect behavior that happened after Ruby 2.6. Since JRuby 9.3.2 is equivalent to Ruby 2.6.8, we fail the same way.

[eregon]

Let's skip that test for Ruby < 2.7 then?

[headius]

Let's skip that test for Ruby < 2.7 then?

I will do that or modify the expectation for 2.6 (probably the former, since we'll likely remove these 2.6 test hacks once JRuby 9.4 is available).

I will also add more context for the bind_call definition.

[headius]

The only exceptions

The other exception that comes to mind is when a standard library shipped with JRuby has fixes that are relevant to a user but which have not backported to the equivalent CRuby release. We try to err on the side of user experience, which is simultaneously why we do accept non-backported fixes and don't introduce newer features that do not match the compatiblity level.

[headius]

And additional tweak was necessary to disable the "installlation test" since the gem requires 2.7 and fails to install on JRuby 9.3.

I will do a separate draft PR removing all these hacks, which will eventually be mergeable once JRuby 9.4 is out.

[headius]

FWIW I ran tests against jruby-head and got three failures:

• One is the same inspect failure, behavior which has not been updated on JRuby master yet.
• The other two appear to be issues with passing hashes to keyword-receiving methods, due to our keyword argument support being in flux at the moment.

Everything else passes without the hacks in place, so I will push that draft PR for future use.

[headius]

@eregon Thank you for the suggestions. This PR is ready for merging and release.