search

arrow-kt / arrow

Ξ›rrow - Functional companion to Kotlin's Standard Library
http://arrow-kt.io
Other
6.13k stars 442 forks source link

Implement Android check to prevent regressions #1715

Closed aballano closed 4 years ago

aballano commented 4 years ago

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

andrzejressel commented 4 years ago

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

aballano commented 4 years ago

@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 commented 4 years ago

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

JorgeCastilloPrz commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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:

JorgeCastilloPrz commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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