New sample with min api level #2117

marandaneto · 2022-06-20T19:30:59Z

📜 Description

💡 Motivation and Context

Fixes #1403

💚 How did you test it?

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the docs if needed
No breaking changes

🔮 Next steps

github-actions · 2022-06-20T19:31:34Z

	Fails
🚫	Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- New sample with min api level ([#2117](https://github.com/getsentry/sentry-java/pull/2117))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against f050a46

sentry-android-core/lint-baseline.xml

marandaneto · 2022-06-20T19:32:39Z

sentry-android-okhttp/lint-baseline.xml

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="UTF-8"?>


No issue for this module, but for some reason, it requires the baseline, likely a bug.

marandaneto · 2022-06-20T19:33:28Z

sentry-apollo/build.gradle.kts

@@ -8,6 +8,7 @@ plugins {
    id(Config.QualityPlugins.errorProne)
    id(Config.QualityPlugins.gradleVersions)
    id(Config.BuildPlugins.buildConfig) version Config.BuildPlugins.buildConfigVersion
+    id(Config.QualityPlugins.androidLint)


Java projects can add the Android lint but I could not find a way to configure it using lint {} cus android {} does not exist.

Apollo is also available for non Android, not sure how we would add it

@adinauer what do you mean? This is the Apollo module and I'm adding the lint to it, so it's alright.

Yes I was trying to say that we probably can't just add the android plugin to it to gain android {} in there as this can also be used by non Android applications.

ah gotcha, that's not the problem, lint is already being applied to this module as a non-Android project, and it's being executed, I just can't change the default options apparently.

Could we simply run grep on the logs instead and check for minApi warnings?

But which logs do you want to run grep on? You need to be able to run lint first :)

I thought it works but we can't customize it to fail the build for minSdk violations.

the Linter runs already, but it does not check for newApi.

https://github.com/open-toast/gummy-bears
This can help
have you used/tried this @romtsn @markushi ?

marandaneto · 2022-06-20T19:33:53Z

sentry-samples/sentry-samples-android-minsdk/build.gradle.kts

+    implementation(projects.sentryAndroid)
+    implementation(projects.sentryAndroidFragment)
+    implementation(projects.sentryAndroidTimber)
+    implementation(projects.sentryApollo)


So all projects with lower API is here

marandaneto · 2022-06-20T19:36:09Z

sentry/src/main/java/io/sentry/Baggage.java

@@ -15,7 +16,7 @@
 @ApiStatus.Experimental
 public final class Baggage {

-  static final @NotNull String CHARSET = "UTF-8";
+  static final @NotNull String CHARSET = StandardCharsets.UTF_8.toString();


For some reason this is not being detected, I guess AGP regressed here.

Warning: Lint will treat :sentry as an external dependency and not analyze it.

Recommended Action: Apply the 'com.android.lint' plugin to java library project :sentry. to enable lint to analyze those sources.

The linter is able to analyze and check everything but does not consider the minApi apparently.

I should revert this change and merge it anyway since I found out other issues with the linter.

So this means we'll have to be careful and check manually in Android Studio if there's any minApi breakages we introduced?

If we don't find a fix for this, yes, which sucks, this used to work.
@romtsn ideas?

answered in https://github.com/getsentry/sentry-java/pull/2117/files#r902303328

but it's not a bug, it's a deliberate decision from them not to run this check on non-android projects, because they cannot infer the minSdkVersion from a jvm-lib

This used to work in the past, so I'd consider either a bug or a non-documented breaking change.

hm, maybe, I checked the git history all the way to 2018 it was still there. Dunno how it would work on non-android projects, since they don't know the min sdk. Maybe you could ask on the issue tracker, but I guess we'd need to have some workaround like I mentioned.

Yeah, that's strange, I'm pretty sure that it worked in the past when developing sentry-android v2 unless I'm mistaken, I don't recall leaking NewApi tho so I strongly think that it worked in the past.
We could try this workaround, it's a good idea.
I just quickly tried and it requires a bit of more work, I'd stop here tho, feel free to add this to our backlog and eventually prioritize finishing up.

Sounds good, @adinauer or I could take it from there, thanks 👍

…ntry-java into chore/min-api-level

codecov-commenter · 2022-06-21T08:08:36Z

Codecov Report

Merging #2117 (f050a46) into main (ba9dda7) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #2117      +/-   ##
============================================
- Coverage     80.92%   80.85%   -0.07%     
+ Complexity     3253     2918     -335     
============================================
  Files           231      176      -55     
  Lines         11951    10545    -1406     
  Branches       1586     1433     -153     
============================================
- Hits           9671     8526    -1145     
+ Misses         1700     1509     -191     
+ Partials        580      510      -70

Impacted Files	Coverage Δ
sentry/src/main/java/io/sentry/SentryClient.java	`87.50% <ø> (ø)`
sentry/src/main/java/io/sentry/TracesSampler.java	`100.00% <ø> (ø)`
sentry/src/main/java/io/sentry/Baggage.java	`89.28% <100.00%> (+0.12%)`	⬆️
...ring/tracing/SpringMvcTransactionNameProvider.java
.../java/io/sentry/graphql/SentryInstrumentation.java
...java/io/sentry/spring/webflux/SentryWebFilter.java
.../io/sentry/spring/tracing/SentryTracingFilter.java
...y/spring/HttpServletRequestSentryUserProvider.java
...o/sentry/spring/webflux/SentryRequestResolver.java
...main/java/io/sentry/spring/SentryHubRegistrar.java
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba9dda7...f050a46. Read the comment docs.

romtsn · 2022-06-21T08:01:12Z

build.gradle.kts

-            "sentry-uitest-android",
-            "sentry-uitest-android-benchmark",
+            "sentry-samples-android-minsdk",
+//            "sentry-samples-console",


Revert before merging

romtsn · 2022-06-21T08:19:57Z

sentry-apollo/build.gradle.kts

@@ -8,6 +8,7 @@ plugins {
    id(Config.QualityPlugins.errorProne)
    id(Config.QualityPlugins.gradleVersions)
    id(Config.BuildPlugins.buildConfig) version Config.BuildPlugins.buildConfigVersion
+    id(Config.QualityPlugins.androidLint)


That's the reason I think - they do not run this check for non-android projects
https://cs.android.com/android-studio/platform/tools/base/+/mirror-goog-studio-main:lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/ApiDetector.kt;l=645

I think we'd need to apply the android plugin and set minSdkVersion in order for it to run.

romtsn · 2022-06-21T08:30:21Z

sentry-apollo/build.gradle.kts

@@ -8,6 +8,7 @@ plugins {
    id(Config.QualityPlugins.errorProne)
    id(Config.QualityPlugins.gradleVersions)
    id(Config.BuildPlugins.buildConfig) version Config.BuildPlugins.buildConfigVersion
+    id(Config.QualityPlugins.androidLint)


I think we could apply the com.android.library plugin and the android {} block conditionally (e.g. having a flag on CI) and run lint as a separate build stage/task, but still release the library as a jar (disable the flag for publish task). Wdyt?

bruno-garcia · 2023-06-01T13:07:45Z

👋

marandaneto · 2023-06-01T13:21:46Z

Outdated, a new PR would make more sense.

romtsn · 2023-06-01T13:40:44Z

sry, never got to it. Does it still make sense to have a min-api-level sample or should we rather have an automated test running against the app with the min-api-level? I'm afraid I will never use this sample for my local testing

New sample with min api level

9f3eac4

marandaneto commented Jun 20, 2022

View reviewed changes

sentry-android-core/lint-baseline.xml Outdated Show resolved Hide resolved

marandaneto commented Jun 20, 2022

View reviewed changes

marandaneto requested review from bruno-garcia and adinauer June 20, 2022 19:36

marandaneto and others added 6 commits June 21, 2022 09:23

config lint

3a87193

Format code

9268b77

fix import

86510e2

Merge branch 'chore/min-api-level' of https://github.com/getsentry/se…

1709d3f

…ntry-java into chore/min-api-level

fix config

d587a72

fix lint

f050a46

romtsn reviewed Jun 21, 2022

View reviewed changes

marandaneto closed this Jun 1, 2023

bruno-garcia removed their request for review June 2, 2023 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New sample with min api level #2117

New sample with min api level #2117

marandaneto commented Jun 20, 2022 •

edited

Loading

github-actions bot commented Jun 20, 2022 •

edited

Loading

marandaneto Jun 20, 2022

marandaneto Jun 20, 2022

adinauer Jun 21, 2022

marandaneto Jun 21, 2022

adinauer Jun 21, 2022

marandaneto Jun 21, 2022

adinauer Jun 21, 2022

romtsn Jun 21, 2022

adinauer Jun 21, 2022

marandaneto Jun 21, 2022

marandaneto Oct 25, 2022

marandaneto Jun 20, 2022

marandaneto Jun 20, 2022

marandaneto Jun 20, 2022

adinauer Jun 21, 2022

marandaneto Jun 21, 2022

romtsn Jun 21, 2022 •

edited

Loading

romtsn Jun 21, 2022

marandaneto Jun 21, 2022

romtsn Jun 21, 2022

marandaneto Jun 22, 2022

romtsn Jun 22, 2022

codecov-commenter commented Jun 21, 2022 •

edited

Loading

romtsn Jun 21, 2022

romtsn Jun 21, 2022

romtsn Jun 21, 2022

bruno-garcia commented Jun 1, 2023

marandaneto commented Jun 1, 2023

romtsn commented Jun 1, 2023

New sample with min api level #2117

New sample with min api level #2117

Conversation

marandaneto commented Jun 20, 2022 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Jun 20, 2022 • edited Loading

Instructions and example for changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romtsn Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 21, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-garcia commented Jun 1, 2023

marandaneto commented Jun 1, 2023

romtsn commented Jun 1, 2023

marandaneto commented Jun 20, 2022 •

edited

Loading

github-actions bot commented Jun 20, 2022 •

edited

Loading

romtsn Jun 21, 2022 •

edited

Loading

codecov-commenter commented Jun 21, 2022 •

edited

Loading