Arrow: Implement Android check to prevent regressions

[aballano]

Arrow 0.10.1 is crashing because Arrow uses AtomicReference, from which it uses some functions that were added to Android in a latter API version

_See https://github.com/arrow-kt/arrow/pull/1710_

[jereksel]

Maybe we can add Java 7 to list of JDKs we build arrow with?

[aballano]

@jereksel As far as I know yes, thank you for the idea! Targeting Java 7 instead of 8 would be enough so there's no chance to mess up in the future at compile time 👍

If you would be interested in trying out, we would love to receive a PR, could be an easy task for Hacktoberfest 🙂

[aballano]

It seems there's a gradle plugin to check for android versions directly 😍 Some notes:

• There are different android apis to target, but I think targeting down to API 14 should be enough, and there's a signature for it cc @JorgeCastilloPrz
• We should be careful to which modules this is applied as there are some processors that should be allowed to use Java 8 APIS because they don't run on Android's app runtime, but ideally the plugin should be applied to all new modules by default (unless specifically deactivated) to avoid future issues

[JorgeCastilloPrz]

Did you try it? Doesn't look very maintained / starred. We'd also need to ensure it covers generated code also. Sounds good to test it but, isn't it enough to target JDK7 and run tests with it? If we ensure we have tests to cover all non generated and generated code in the library (I'd say we do already) that should ensure everything is alright.

[aballano]

@JorgeCastilloPrz I did a quick check and it seems to work pretty well, it actually showed a violation in one of the functions from the new atomics

[JorgeCastilloPrz]

Which one? Do we need a hotfix once again?

Back to the plugin: What about generated code? Can you include those sources in the analysis?

[aballano]

Which one? Do we need a hotfix once again?

@JorgeCastilloPrz I'll raise a PR in a moment, no need for hotfix IMO as it's a function that won't be used anyway.

Back to the plugin: What about generated code? Can you include those sources in the analysis?

Yes, you can configure source sets https://github.com/xvik/gradle-animalsniffer-plugin#scope

[JorgeCastilloPrz]

@JorgeCastilloPrz I'll raise a PR in a moment, no need for hotfix IMO as it's a function that won't be used anyway.

👍 Thanks! 👌

Yes, you can configure source sets https://github.com/xvik/gradle-animalsniffer-plugin#scope

Sounds good! What would we potentially get from adding this on top of the safety you'd already get by running tests over JDK7?

[aballano]

Sounds good! What would we potentially get from adding this on top of the safety you'd already get by running tests over JDK7?

The difference is that, for example, some higher APIs do support some Java 8 goodies, so targeting a specific JDK would work, but would also prevent us from using some fancy Java 8 api that we might need, depending also on the target Android version.

An example:

• Below API 24 you can't use java Atomic's updateAndGet
• Below API 21 you can't use ForkJoinPool

[JorgeCastilloPrz]

Oh, so you're talking about Android desugaring for some JDK8 apis. Yep the point would be not to build the project with JDK7, keep targeting JDK8 so you can keep using those. Then we'd just run the tests with JDK7 as it's the lower boundary we need to support. Tests emulate the runtime mimicking how a client project runs the library APIs so they can catch all those things if you run them using JDK7.

Still thinking twice, I can see how the approach could make blurry to know which JDK8 apis we are allowed to use (i.e: can be desugared) and which ones not. That would make the plugin you mention a more productive approach 👍

[aballano]

From the added check we noticed that the retrofit integration module requires android API 28, which could be problematic for most Android apps using retrofit + the integration:

https://github.com/arrow-kt/arrow/pull/1751/#issuecomment-547797475

[aballano]

@pakoito @JorgeCastilloPrz Shall I create a ticket to fix that in retrofit integration? I'm unsure how maintained is or if we plan to do some extra work on it.

[JorgeCastilloPrz]

I'd say we don't wanna have it there if it's not backwards compatible to at least minSdk 21, so yep I'll open an issue to fix it if that's the case.

[aballano]

Done, thanks! https://github.com/arrow-kt/arrow/issues/1761