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

improve cross compile compatibility #5451

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@ahorek
Copy link
Contributor

commented Nov 15, 2018

The Java 9 ByteBuffer and CharBuffer classes introduces overloaded methods with covariant return types for the following methods used by the driver:

  • position
  • limit
  • flip
  • clear
  • mark

Without casting, exceptions like this are thrown when executing on Java 8:

java.lang.NoSuchMethodError: java.nio.ByteBuffer.limit(I)Ljava/nio/ByteBuffer

fixes #5450

@ahorek ahorek force-pushed the ahorek:bytebuffer branch from 68f4722 to fcf7642 Nov 15, 2018

pavel

@ahorek ahorek force-pushed the ahorek:bytebuffer branch from fcf7642 to bfa5af0 Nov 15, 2018

@headius

This comment has been minimized.

Copy link
Member

commented Nov 17, 2018

I hate that we have to do this but I guess there's not really any good choice. However, perhaps it would be better if we put the cast + call for each of these into a utility method? Helpers.clearBuffer(buffer) for example?

@kares

This comment has been minimized.

Copy link
Member

commented Nov 17, 2018

my understanding was that only those having a changed return are problematic - so this seems a bit much.
ByteBuffer#limit(int) has an issue but not ByteBuffer#limit() with an int return, same for position()

@headius

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

@kares My concern is having this cast + call pattern all over the place. We would end up having to document every one of these lines so nobody changes it back (most IDEs will show it as an unnecessary cast). If we move them into utility methods and call them that way, we only have to document it there.

And with an import static, "clearBuffer(buf)" is shorter than "((Buffer) buf).clear()" anyway.

@headius

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

FWIW, it's also easier for us when we start depending on Java 11 (maybe in 2020) because we can just have the IDE inline all calls to the method and eliminate the cast.

@headius

This comment has been minimized.

Copy link
Member

commented Nov 19, 2018

@ahorek Feel like revisiting these and making them call utility methods, perhaps in org.jruby.util.io.ChannelHelper?

@ahorek

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

ByteBuffer#limit(int) has an issue but not ByteBuffer#limit() with an int return, same for position()

@kares - the problem is ByteBuffer#limit is overloaded on java 9, but on java 8 this overloaded call-site doesn't exist.

Feel like revisiting these and making them call utility methods, perhaps in org.jruby.util.io.ChannelHelper

@headius - sounds good. I'll do that later today.

@ahorek

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

after some investigation I'll close this one if you don't mind

other methods like java.lang.Math.multiplyExact have the same problem and it'll be a big maintainance burden

it can be fixed by compiling the source in compatibility mode, otherwise the binary compatibility with older java versions isn't guaranteed
base.java.version and base.javac.version

https://stackoverflow.com/questions/51692748/java-compilation-source-target-and-release-supported-versions/51692749#51692749

@ahorek ahorek closed this Nov 20, 2018

@headius

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

@ahorek Actually that post is referring to just the Java code and bytecode resulting from compiling; source specifies the maximum level of Java features allowed and target specifies what minimum JVM the bytecode will work on. The fixes you were making were separate and would require always compiling against Java 8 classes, necessitating having two JDKs installed to build properly.

I think the cast+call fix is still valid but I will look at doing it with a utility method.

@ahorek

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

ok, reopened #5459

@enebo enebo added this to the Invalid or Duplicate milestone Nov 28, 2018

headius added a commit to headius/jruby that referenced this pull request Dec 6, 2018

headius added a commit that referenced this pull request Dec 6, 2018

Fix known binary compat issues with new Java 9 overloads.
Fixes #5451.

Thanks to @ahorek for the original patch (#5459).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.