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

Change XmlNodeSet#to_a return type to RubyArray #1969

Closed
wants to merge 2 commits into from

Conversation

headius
Copy link
Contributor

@headius headius commented Jan 14, 2020

The existing signature conflicts with one added to JRuby 9.2.9.
Specifically, the new signature in JRuby returns RubyArray, which
causes a compilation error on this line in Nokogiri because it
attempts to use a more general return type.

We would prefer to keep the specific return type in JRuby.

  • If we patch JRuby, then 9.2.9 will never be able to compile any
    version of Nokogiri.
  • If we patch Nokogiri, all versions of JRuby can compile current
    and future Nokogiri. Versions prior to 9.2.9 will be able to
    compile all existing releases of Nokogiri.

I do not believe the change in 9.2.9 breaks anything at runtime,
since the JVM does not care about this particular return type
mismatch unless someone actually returns a non-RubyArray object.

Fixes #1968

The existing signature conflicts with one added to JRuby 9.2.9.
Specifically, the new signature in JRuby returns RubyArray, which
causes a compilation error on this line in Nokogiri because it
attempts to use a more general return type.

We would prefer to keep the specific return type in JRuby.

* If we patch JRuby, then 9.2.9 will never be able to compile any
  version of Nokogiri.
* If we patch Nokogiri, all versions of JRuby can compile current
  and future Nokogiri. Versions prior to 9.2.9 will be able to
  compile all existing releases of Nokogiri.

I do not believe the change in 9.2.9 breaks anything at runtime,
since the JVM does not care about this particular return type
mismatch unless someone actually returns a non-RubyArray object.

Fixes sparklemotion#1968
@headius headius changed the title Change return type to RubyArray Change XmlNodeSet#to_a return type to RubyArray Jan 14, 2020
@jvshahid
Copy link
Member

The changes look good to me. I don't think the failures are related to the change. The failure was in the MRI valgrind tests. I am waiting to see the tests pass before I merge the PR.

@flavorjones
Copy link
Member

Yeah, looks like some optimizations were backported to ruby 2.5, so I just added a valgrind suppression and will kick off again.

@flavorjones flavorjones added this to the v1.11.0 milestone Jan 15, 2020
@flavorjones
Copy link
Member

flavorjones commented Jan 15, 2020

@headius I see you based this commit on the v1.10.x branch. For future reference, we ask that people send PRs against master, and then we can cherry-pick onto the v1.10.x branch if we need to cut a release.

I'm not totally familiar with the JRuby 9.2.9 change that is driving this commit, but am I to understand that you're requesting a bug fix release in the v1.10.x series?

@flavorjones
Copy link
Member

Remaining CI failure is due to #1971, looking into it. We could merge this PR, but I'd like to leave it open for a bit so I can ensure the CI job is stable.

@headius
Copy link
Contributor Author

headius commented Jan 15, 2020

@flavorjones In JRuby 9.2.8, the only to_a defined on our RubyBasicObject class was RubyArray to_a(). In JRuby 9.2.9, we added RubyArray to_a(ThreadContext). Nokogiri's XmlNodeSet previously defined a IRubyObject to_a(ThreadContext) signature. Since XmlNodeSet descends from RubyBasicObject, method signatures it overrides need to be compatible. In this case, because IRubyObject is a the more generic interface type than RubyArray, it fails to compile.

So the addition of the new to_a in JRuby broke compilation of Nokogiri because Java. At runtime it probably doesn't matter since the XmlNodeSet#to_a still returns a RubyArray, but the compiler doesn't like the signatures not matching.

This fix should go into any active Nokogiri branches going forward. I did not do the PR against master because it appear to be behind the branch.

@flavorjones
Copy link
Member

@headius Going forward please submit PRs based on master, thanks.

@flavorjones
Copy link
Member

Merged manually onto master. Will be in v1.11.0. Thank so much, @headius!

@flavorjones
Copy link
Member

Side note: this actually went out in v1.10.9, see https://my.diffend.io/gems/nokogiri/1.10.8-java/1.10.9-java

@headius
Copy link
Contributor Author

headius commented Oct 21, 2020

@headius Going forward please submit PRs based on master, thanks.

A suggestion: put this in the project PR template, perhaps?

Thanks for merge and release!

@flavorjones
Copy link
Member

@headius Great minds think alike: 787fed8 🤣

Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants