Dushistov / flapigen-rs

Tool for connecting programs or libraries written in Rust with other languages
BSD 3-Clause "New" or "Revised" License
767 stars 59 forks source link

java.util.OptionalDouble not available on Android < 24 #314

Open jellelicht opened 4 years ago

jellelicht commented 4 years ago

This also applies to OptionalLong, OptionalInt and Optional<T>

There is a compat library available for exposing these under java8.util.OptionalX or even java9.util.OptionalX, but these classes will of course not be found under the java.util namespace. Would it be possible to make the namespace where these classes are resolved configurable, similar to how it is already done with the @NonNull annotation?

Dushistov commented 4 years ago

That's why there is option to change java.util. to any package. For example:

    let swig_gen = rust_swig::Generator::new(LanguageConfig::JavaConfig(
        JavaConfig::new(output_dir, "com.company.your_project.core.rust".into())
            .use_null_annotation_from_package("android.support.annotation".into())
            .use_optional_package("com.hadisatrio.optional".into()),
    ))
    .rustfmt_bindings(true)
    .remove_not_generated_files_from_output_directory(true);

As you can see details here: https://docs.rs/rust_swig/0.4.0/rust_swig/struct.JavaConfig.html#method.use_optional_package

Dushistov commented 4 years ago

The idea was that Java compiler report you that java.util.Optional* is not supported, you look at documentation and use use_optional_package. It is hard to tell why you reach #312 and d539a4dd3e354d807749e8b9408ab599fe7300b5 without noticing use_optional_package.

Confusing naming?

jellelicht commented 4 years ago

The compiler chugs along happily, as these classes do exist in newer android versions. It is a runtime error only on the older ones once you do System.loadLibrary, which gave a pretty nondescriptive SIGABRT. To make this explicit: we build an AAR that has to support both android version that do have java.util.OptionalX and version that do not.

EDIT: So to clarify; I never saw an error before deploying my code

jellelicht commented 4 years ago

The commit you linked was not supposed to be linked into the PR. I opened a new one without it.

What I would like is to have a use_optional_package that allows a fallback, as this decision should be deferred to be made at runtime. I briefly considered using Proguard to rewrite the generated Java classes, but this would of course not work for the rust code that actually looks for these classes in JNI_OnLoad.

If this is not feasible, we could also simply create 2 builds, one with the proper java.util.OptionalX and the other with a drop-in replacement for that.

Dushistov commented 4 years ago

I find out why I can catch this type of errors during compilation, because of ./gradlew build run :app:lint task, so CI catches this issues without problem:

GoogleMapActivity.java:90: Error: Call requires API level 24 (current min is 16): java.util.Optional#isPresent [NewApi]
          if (lastPos.isPresent()) {

So if linter is not enough for you for some reason, I suppose you can run gradle task, that parse build.rs and build.gradle and if minSdkVersion doesn't match usage of use_optional_package it triggers error. I suppose this would be cheap in terms of build time and no runtime overhead.

tasn commented 3 years ago

I spent a lot of time debugging this until I finally found this ticket. Using the patch from https://github.com/Dushistov/flapigen-rs/commit/d539a4dd3e354d807749e8b9408ab599fe7300b5 fixed it for me too. What's weird though is that I don't have any Optional use in my code. grep -ri option in the generated Java dir confirms it, and yet it was still failing. The code the above commit removes seems like it should not be called for my code, so why is it? What am I missing? Why does that commit fix it for me?