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

Making demo app independent #355

Merged
merged 3 commits into from
May 14, 2024

Conversation

LikeTheSalad
Copy link
Contributor

@LikeTheSalad LikeTheSalad commented May 10, 2024

Option to avoid these kinds of issues.

This option extracts the demo app into its own project and adds a dependency onto the agent project by using the composite builds config. This option seems to be more stable for the future, considering that the other option proposed below uses a configuration param that's not properly documented, and also adds a risk of Compose not being able to properly compile due to Kotlin version issues.

Some things to consider with this option are:

  • There's a duplication of all the gradlew-related files for the independent project.
  • Android Studio has to explicitly open the project demo-app in order to be able to compile the app with it.

Another option is: #356

@LikeTheSalad LikeTheSalad requested a review from a team May 10, 2024 10:44
Copy link
Contributor

@bidetofevil bidetofevil left a comment

Choose a reason for hiding this comment

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

LGTM. Just have question about Java usage but non-blocking

Just to add more color to the discussion in #342 , I think a separate demo app project is a better option. Consuming the extension as an external dependency like other apps would would prevent mistakes in accidentally adding a dependency or call that would not fly for folks who are not part of the project.

While this is more annoying when we're developing new features or breaking APIs given we have to change a separate project, but I think that's actually a good thing to consume the extension like everyone else does. This is the setup I'm used to anyway, and publishing and consuming artifacts through local Maven generally makes this not super painful, for me at least, as I'm used to much more painful setup haha.

@@ -42,6 +45,16 @@ android {
composeOptions {
kotlinCompilerExtensionVersion = "1.5.13"
}
val javaVersion = JavaVersion.VERSION_11
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: do we require (desugared) Java 11 features to use the android extension? For a demo app, it'd be nice to just use Kotlin and not having to worry about Java

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're right -- at least right now it looks like the demo app is all kotlin, so I would think we could remove this and see if something else in the build requires it?

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

Thanks @LikeTheSalad! This is straightforward and moves us out of the current kotlin version mismatch hell and soup of build breakages.

When I took an attempt at this, I wanted to try and avoid having to help the demo-app as another IntelliJ window, and that didn't work out at all. I think this is fine tho, if not just a little awkward for otel-android devs.

@breedx-splk breedx-splk merged commit 83c3fed into open-telemetry:main May 14, 2024
4 checks passed
bidetofevil pushed a commit to bidetofevil/opentelemetry-android that referenced this pull request May 18, 2024
* Making demo app independent

* Update demo-app/build.gradle.kts

* Update demo-app/build.gradle.kts

---------

Co-authored-by: jason plumb <75337021+breedx-splk@users.noreply.github.com>
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.

3 participants