Rhino 1.7.14 not usable on Android any more due to javax.lang.model.SourceVersion #1149
Comments
b3nn0
changed the title
|
Thanks for catching this!
I think that these kinds of things keep happening because the few people
who are regularly contributing to Rhino aren't Android developers and don't
know how to tell whether Rhino is compatible with certain versions of
Android (and, in fact, what versions of Android Rhino should target).
I'm willing to spend some time fixing this, but do you (or anyone else)
have the ability to help come up with some sort of test plan that would
allow us to regularly test for Android compatibility so that future changes
can be discovered before it's too late?
…On Fri, Jan 7, 2022 at 7:16 AM Adrian Batzill ***@***.***> wrote:
Hi,
while previous versions worked just fine on Android in interpreted mode,
Rhino 1.7.14 does not work any more due to this commit: 0fcfb09
<0fcfb09>
because javax.lang is not available on Android: import
javax.lang.model.SourceVersion;
As a workaround I created my own class in javax.lang.model:
public class SourceVersion {
public static SourceVersion latestSupported() {
return new SourceVersion();
}
public final int ordinal() {
return 8;
}
}
It seems to work fine, but also feels very hacky and I'm not sure if it's
even correct.
Is there any hope that this is fixed upstream despite Android not being
supported?
—
Reply to this email directly, view it on GitHub
<#1149>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7I2Y7CDHBZ5K7IN3AKODUU37VLANCNFSM5LPAASLA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
FWIW, Error Prone does have a static checker for these types of issues: |
|
Great -- I'll try that out!
…On Fri, Jan 7, 2022 at 10:34 AM Manu Sridharan ***@***.***> wrote:
FWIW, Error Prone does have a static checker for these types of issues:
http://errorprone.info/bugpattern/AndroidJdkLibsChecker
—
Reply to this email directly, view it on GitHub
<#1149 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7I25MFJ545Y3VYPPQS7TUU4W2HANCNFSM5LPAASLA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Interestingly, ErrorProne (which is hard to make work on Java 8, BTW, which is another reason I'd love to upgrade) found over 200 things to fix, so we'd have our work cut out for us to clean up the code and also suppress spurious warnings. The "AndroidJdkLibsChecker" found problems with: BigInt support (new in 1.7.14) It did not, however, complain about using javax.lang.SourceVersion. Is there some sort of an official list of what is supported and not supported in various Android versions? Thanks! |
|
Not super well documented, but it looks like there is a flag https://github.com/google/error-prone/releases/tag/v2.3.2 Those allowed functionalities should be desugared or otherwise handled by modern Android build tools for software targeting older Android versions. I think it's reasonable for Rhino to use those functionalities. I'm not sure about why |
|
I opened google/error-prone#2817 for the issue with not finding the use of |
|
Back to "how to fix it" -- does this show up as a ClassNotFoundException, or something else? I wonder if we were to surround that call with a try...catch for CNFE, that we could make this same code work on Android- and non-Android platforms. |
|
Stack trace: So I guess, try-catch might be an option. |
|
Thanks for all the attention! Is there anyone in the community who works with Android and Rhino regularly who can create a reproducer for this? It could be a GitHub repo or a Gist that includes code that reproduces it, or an existing app that we can build and test, or just a detailed set of instructions. Without that, we're waiting for someone in the Rhino community, or me, to install Android Studio, learn to build an app, learn how to get Rhino into that app, test it, and fix the problem. No one here actually is paid to work on Rhino full time or even close to it so this second option will probably take a while. |
|
I have now tried to prepare something like this: an easy way to run the unit tests against the Android SDK (on desktop, so no Android device or emulator is needed). Since I'm not very familiar with the Rhino code, I simply excluded a few tests in build.gradle to get it working initially. Some stuff is correct to be excluded (like debugger GUI), some stuff I don't know about... All changes related to the android tests are in a separate directory "android" - only build.gradle and AndroidManifest.xml is needed, really: However, there were some compilation problems. One due to the already mentioned So with my fork, you should be able to just run EDIT: EDIT2: EDIT3: which looks like it's related to the changes I had to do to make it compile. @gbrail How to proceed with this? Should I create a cleaned up pull request to add this self-contained "android" subdirectory? |
|
Thanks a ton for pulling this together -- I'm currently wondering whether it'd be easier to incorporate this into the regular repository, or have a separate repo that just does what you've done here to build Rhino with Android tools, and then run that as part of the CI. |
|
I saw the commit log of Why don't we leave the |
|
First, in order to solve the |
|
Just an udpate -- I haven't had a chance to set up an Android environment to test this, and I'm not sure when I will. If someone can produce a PR that works in the current environment, and can assure us that it also works in Android, then that's a start, and we can quickly move forward. Otherwise we're waiting for someone to have time to learn how to test Android stuff. When we get a fix, I'm willing to create a 1.7.14.1 release that includes this fix. But in the meantime there are other things pending so I plan to merge them in to "master" while we work. |
@b3nn0 yes, a cleaned-up PR with all the test infra structure would be the way forward I think. Personally I don't think we should wire up things to run the android tests automatically when running We should have the option to trigger running the Android tests locally and, once all the kinks are ironed out, add a job to our CI(s) to run the tests against Android on every commit. One question though (not being well-versed in Android development): there are many Andoid versions/API levels: how do those correlate to the test infrastructure you set up? Would that test against all versions/levels? Or just the last one? Or...? Looking at your branch, it looks like it is only testing against API version 31? Also see #1156 which refers to api level 21. Once the testing infra structure is available, work on fixing the issues at hand can commence. @archethic I agree we should have a utility function to test whether running on Android or not. Currently in 2 places in the codebase we have
@archethic I think @b3nn0 just did that to make the tests pass in his branch (just like some of the other things he commented out) If I'm not mistaken, the check on STRICT_REFLECTIVE_ACCESS has to do with modular Java (correct @carlosame?). If so, how relevant is that when running on Android? @b3nn0 The stuff you commented out in XmlProcessor.java: isnt the catch on IllegalArgumentException sufficient? What kind of exception is thrown then? As for the stuff you commented out in JavaToJSONConverters: Looks like Android doesn't support the full java.beans package api, just a small subset. There is a 3rd party library it seems to fills in the missing bits: https://github.com/melix/openbeans. Not exactly sure how to go about fixing this, whether we could adjust out implementation to not rely on that specific api from the java.beans package or if we could somehow instead use https://github.com/melix/openbeans opn Android is its available on the classpath. Hope others have idea's on that |
|
OK, I will prepare a pull request in the next few days. I agree that it makes sense to have the android build separately, since yes, it will download the Android SDK and install it in a temporary subdirectory. wrt. SDK Version: While the "more advanced"/"highlevel" Android features are sometimes not 100% compatible in newer SDK version, the "basic java library" that is used by Rhino is usually only extended, not reduced. So IMO it should be enough to pick a "minimum supported version" and test against that. Let's say something around Android 5.x (API Level 21) should be old enough (?). Regarding exceptions for non-available library features (e.g. XmlProcessor.java): |
k, sounds good to me Regarding your choice of compiling against the Android SDK: how would Android developers use Rhino? I would think they take the Rhino jar file that we provide here and use that, not (try to) compile the Rhino source against the Android SDK, correct? If so, I think the approach to testing Rhino against Android should be the same: take the Rhino jar as provided by us and they, using the Android SDK run the tests against that jar. Given asuch approach, I wonder if we couldn't 'just' add some android task to our existing build.gradle file that we can trigger conditionally and has I'm not well-versed in gradle either, so no idea how to for example dynamically load the andoid plugin only when executing the task to run the testsuite against andoid |
To fix this issue @p-bakker you could try using a different way to determine whether one is on a modular JDK, like attempting a reflective access to a method that only a modular JDK would have. Something like the following (or any variation of it): @@ -21,25 +21,33 @@ import java.security.Permission;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import javax.lang.model.SourceVersion;
/**
* @author Mike Shaver
* @author Norris Boyd
* @see NativeJavaObject
* @see NativeJavaClass
*/
class JavaMembers {
- private static final boolean STRICT_REFLECTIVE_ACCESS =
- SourceVersion.latestSupported().ordinal() > 8;
+ private static final boolean STRICT_REFLECTIVE_ACCESS;
private static final Permission allPermission = new AllPermission();
+ static {
+ boolean getModule;
+ try {
+ getModule = Class.class.getMethod("getModule") != null;
+ } catch (NoSuchMethodException e) {
+ getModule = false;
+ }
+ STRICT_REFLECTIVE_ACCESS = getModule;
+ }
+
JavaMembers(Scriptable scope, Class<?> cl) {
this(scope, cl, false);
}
JavaMembers(Scriptable scope, Class<?> cl, boolean includeProtected) {
P.S.: Line 181 in 1cb6002 In fact you should remove the entire Moreover, I'm not sure why Rhino needs to explicitly declare an |
|
tnx for the input @carlosame |
Unfortunately the code snippet in my previous post would be flaky. Turns out that the I got it to work in my |
|
Hello. Any news on this? I downgraded to the previous version for now. Thanks. |
|
Should be resolved by #1385 |
I‘m android developer too, I think many people work on android need this lib. too |
Hi,
while previous versions worked just fine on Android in interpreted mode, Rhino 1.7.14 does not work any more due to this commit: 0fcfb09
because javax.lang is not available on Android:
import javax.lang.model.SourceVersion;As a workaround I created my own class in javax.lang.model:
It seems to work fine, but also feels very hacky and I'm not sure if it's even correct.
Is there any hope that this is fixed upstream despite Android not being supported?
The text was updated successfully, but these errors were encountered: