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

Experiment with checking against *Android* Animal Sniffer signatures #4005

Closed
cpovirk opened this issue Sep 2, 2020 · 7 comments
Closed
Assignees
Labels
P2 package=general type=other Miscellaneous activities not covered by other type= labels

Comments

@cpovirk
Copy link
Member

cpovirk commented Sep 2, 2020

...as opposed to just checking against "Java 6" and "Java 8."

I somewhat doubt that we'd get much out of this (and it might even have drawbacks if it starts flagging types like Unsafe?), but I wanted to record somewhere that I learned of this possibility.

I saw it in action in https://github.com/googleapis/google-http-java-client/blob/e957229fd77c08b7dd6233e2a243f6c675ba32bf/pom.xml#L515-L527

@cpovirk cpovirk added type=other Miscellaneous activities not covered by other type= labels package=general P4 labels Sep 2, 2020
@JakeWharton
Copy link

We use this on a bunch of projects. Okio, OkHttp, Retrofit, etc. Works great. Lets us target Java version independent of a minimum Android API level without incident. We also don't use Unsafe though...

@cpovirk
Copy link
Member Author

cpovirk commented Sep 2, 2020

Good to know.

We do have Animal Sniffer checks in place; they just use the JDK signatures rather than Android.

(And I see that we already have to suppress the error for Unsafe just below the snippet I've linked to :))

Oh, and Jake, you might be the kind of person who is interested in Animal Sniffer's failure to actually fail the build for incompatible changes to the *Buffer classes, as discussed in protocolbuffers/protobuf#7827 and various issues linked from there. (Perhaps you already saw #3990 here.) However, since you're primarily(?) targeting Android, you might be safe, as desugaring (well, at least Bazel's desugaring, but I assume the "normal" toolchain, too) rewrites the references to be compatible with older versions of the classes. But if you hear reports like this from any of those non-Android people, that might be what's up :) (I had looked briefly at OkHttp and noticed that you had conveniently sidestepped this issue in at least one place a couple years ago.)

@cpovirk
Copy link
Member Author

cpovirk commented Oct 9, 2020

@elharo notes that Animal Sniffer's Android "scents" haven't been updated since API Level 27. (As I write this, it looks like we're at API Level 30.)

(As noted in my comment just above this one, we've had other trouble with Animal Sniffer, too.)

It's also possible that someday we'll be able to assume that Android users are using Java 8 library desugaring. At that point, we might start using that subset of Java 8 APIs. I somewhat doubt that Animal Sniffer supports that (though presumably we could generate our own "scents" if we wanted), but Android Lint and/or Error Prone's AndroidJdkLibsChecker may help.

@ogolberg
Copy link

ogolberg commented Oct 9, 2020

Check out https://github.com/open-toast/gummy-bears - it provides android scents for API 19-30 and accounts for APIs "available" via desugaring (vs. android.jar)

@elharo
Copy link
Contributor

elharo commented Nov 9, 2020

To be honest, API level 26 is probably good enough for now. That still only covers ~75% of the installed base. We'd have to go earlier than 26--I'm not sure how far--to bring that up to 95%.

@cpovirk
Copy link
Member Author

cpovirk commented Nov 9, 2020

Agreed: Guava targets API Level 15, so it will be a while before we have any problems.

@cpovirk
Copy link
Member Author

cpovirk commented Nov 10, 2023

              <groupId>net.sf.androidscents.signature</groupId>
              <artifactId>android-api-level-15</artifactId>
              <version>4.0.3_r5</version>

...passes, as does...

              <groupId>net.sf.androidscents.signature</groupId>
              <artifactId>android-api-level-19</artifactId>
              <version>4.4.2_r4</version>

...which is what we might look at soon. In fact, the latter lets me remove the exclusion for java.util.Objects (but not Unsafe). (Perhaps we could get the same effect by advancing to the JDK7 signatures, but using actual Android signatures should be safer.)

Also good (including letting me remove java.util.Objects):

              <groupId>com.toasttab.android</groupId>
              <artifactId>gummy-bears-api-19</artifactId>
              <version>0.6.1</version>

I haven't played around enough with desguared vs. non-desugared APIs to confirm that it's better than net.sf.androidscents, but com.toasttab.android seems like the thing to try. (My playing around did teach me something new about SequencedSet and SequencedCollection from 945ce37, though....)

@cpovirk cpovirk added P2 and removed P4 labels Nov 21, 2023
@cpovirk cpovirk self-assigned this Nov 21, 2023
copybara-service bot pushed a commit to google/truth that referenced this issue Nov 21, 2023
…red APIs](https://github.com/open-toast/gummy-bears#android) but still not opt-in library desugaring) instead of the JDK.

Fixes google/guava#4005

RELNOTES=n/a
PiperOrigin-RevId: 584340129
copybara-service bot pushed a commit to google/truth that referenced this issue Nov 21, 2023
…red APIs](https://github.com/open-toast/gummy-bears#android) but still not opt-in library desugaring) instead of the JDK.

Fixes google/guava#4005

RELNOTES=n/a
PiperOrigin-RevId: 584340129
copybara-service bot pushed a commit that referenced this issue Nov 21, 2023
…red APIs](https://github.com/open-toast/gummy-bears#android) but still not opt-in library desugaring) instead of the JDK.

Fixes #4005

RELNOTES=n/a
PiperOrigin-RevId: 584340129
copybara-service bot pushed a commit to google/truth that referenced this issue Nov 21, 2023
…red APIs](https://github.com/open-toast/gummy-bears#android) but still not opt-in library desugaring) instead of the JDK.

Fixes google/guava#4005

RELNOTES=n/a
PiperOrigin-RevId: 584349750
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment