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

Move to Java 8 #186

Open
winder opened this issue Jul 22, 2020 · 11 comments
Open

Move to Java 8 #186

winder opened this issue Jul 22, 2020 · 11 comments

Comments

@winder
Copy link
Contributor

winder commented Jul 22, 2020

We should deprecate support for Java 7 and move to Java 8.

Java 7 has almost no market share these days, especially in the security conscious crypto space.

If that isn't enough, there are a handful of Android security fixes in Java 8, so requiring that version could protect algorand users.

@winder winder added the FDE label Jul 22, 2020
@algorotem algorotem assigned algorotem and Priegle and unassigned algorotem Oct 5, 2020
@algorotem
Copy link
Contributor

Needed to check if it would break any mobile app. @Priegle Any insight here?

@srayhunter
Copy link

@winder is anyone working on this item currently?

@winder
Copy link
Contributor Author

winder commented Jun 14, 2021

@srayhunter no. Is there a specific reason you would like to see us remove Java7 support?

@richdrich
Copy link

richdrich commented Aug 27, 2021

Just FYI: I've found an issue when you target minSdkVersion = 24 and run on Android 7.

I'm seeing:

com.fasterxml.jackson.databind.JsonMappingException: No virtual method decode(Ljava/lang/String;)[B in class Lorg/apache/commons/codec/binary/Base64; or its super classes (declaration of 'org.apache.commons.codec.binary.Base64' appears in /system/framework/org.apache.http.legacy.boot.jar)
  at [Source: [B@d361c9b; line: 1, column: 340] (through reference chain: com.algorand.algosdk.v2.client.model.TransactionsResponse["transactions"]->java.util.ArrayList[0]->com.algorand.algosdk.v2.client.model.Transaction["genesis-hash"])

which stems from the org.apache.commons.codec.binary.Base64 picking up a broken BaseNCodec from the bootpath.

Using Java 8 Base64 would fix this.

(I cant find a non hacky way to stop my app loading the broken commons code)

@timmolter
Copy link

Please keep it Java 7 compatible. We still need this to run on ancient android 4 runtimes.

@timmolter
Copy link

I suggest we update to use the BouncyCastle Base64 class, since it's already on this projects classpath. It will also solve another issue with namespace clashing I'm having on Android 4.

@richdrich
Copy link

If this works in the old BouncyCastle that's bundled in old Android?
Or, just add a local base64 routine that avoids any library issues - should not be needed I know :-)

@timmolter
Copy link

Here's the specific issue I am having: #271

My option #2 to fix it, is exactly what you suggested.

@timmolter
Copy link

For BouncyCastle you can force the usage of the compile dependency using

            Security.removeProvider(BouncyCastleProvider.PROVIDER_NAME);
            Security.insertProviderAt(new BouncyCastleProvider(), 0);

which guarantees ( I think) it uses the one you want and not the old BouncyCastle that's bundled in old Android. This isn't an issue with Java 8 though because you can force the update during runtime.

However there are OTHER issues that I've run into that updating to Java 8 can cause on Android 4, which there are only painful fixes for:

  • List.sort(Object::compareTo)
  • Long.hashCode(long)
  • org.apache.commons.codec.binary.Base64.decode(Ljava/lang/String;)[B

to name a couple. Once you allow Java8, people start using this stuff and then you get crashes usually in production because of some obscure phone that you don't have in hand to test with.

I'm not trying to start an argument, just providing a counter argument. Too bad we just can't update all the phone out there!

@richdrich
Copy link

I think that changing the provider makes it use the correct provider for javax.security (JCE) APIs but not where the bouncycastle dependency is imported directly. But I may be wrong.

@timmolter
Copy link

Yeah, maybe you're right about that. I'm still learning about this specific issue.... ;)

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

No branches or pull requests

6 participants