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

Add additional Android requirements to README #68

Merged
merged 3 commits into from
May 20, 2024

Conversation

bidetofevil
Copy link
Contributor

Modify README to reference an additional requirement for Android projects that support Android versions that require desugaring. Details are on this Slack thread.

I can't find a style guide that tells me how this modification should be formatted, so please let me know if this doesn't work and I'll fix it up!

@bidetofevil bidetofevil requested a review from a team May 4, 2024 20:53
README.md Outdated
> built on debug, you must use AGP 8.3.0+ and set the `android.useFullClasspathForDexingTransform`
> property in `gradle.properties` to `true` to ensure desugaring runs properly. For the full
> context for this workaround, please see
> [this issue](https://issuetracker.google.com/issues/230454566#comment18).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-05-06 at 10 20 04 AM

This renders weird. Can you remove the > quoting and change > Android Requirements to ## Android Requirements?

README.md Outdated
@@ -54,6 +54,18 @@ To use these artifacts, you must also depend on `io.opentelemetry:opentelemetry-
See [opentelemetry-java releases](https://github.com/open-telemetry/opentelemetry-java#releases) for
more information.

> Android Requirements
>
> If you are using this on Android and your project's minSdk is lower than 26, you must enable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just link to the core repo section which talks about desugaring? https://github.com/open-telemetry/opentelemetry-java/blob/main/VERSIONING.md#language-version-compatibility

The "minSdk is lower than 26" requirement is the type of thing what will become out of date and I'm not really sure where 26 comes from.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

26 is Android API version code. It's pretty impenetrable unless you're deep in that ecosystem.

I'll make a quick reference to desugaring here and point to the opentelementry-android repo for more details so we can isolate the details in one place at least.

I'll put up a revision in the next couple days.

@bidetofevil
Copy link
Contributor Author

Updated. I linked to the PR instead of the README for details in case the README changes. I can change that if y'all want.

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jack-berg
Copy link
Member

Looks like the build is failing for something unrelated to this PR, which should be fixed in #71. After that is merged, we can merge main into this branch and merge.

@jack-berg jack-berg merged commit 756a10d into open-telemetry:main May 20, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants