-
-
Notifications
You must be signed in to change notification settings - Fork 36
feat: Auto-install distribution dependency when enabled for variant #1001
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
base: main
Are you sure you want to change the base?
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % question around AutoInstall.kt
): TaskProvider<GenerateDistributionPropertiesTask>? { | ||
val variantName = name | ||
if (extension.distribution.enabledVariants.get().contains(variantName)) { | ||
// Add the build distribution dependency if autoInstallation is enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see all the machinery in plugin-build/src/main/kotlin/io/sentry/android/gradle/autoinstall/AutoInstall.kt
& plugin-build/src/main/kotlin/io/sentry/android/gradle/autoinstall/*/*Strategy.kt
I assume we don't want to use that because we want to condition on extension.distribution.enabledVariants
? It might be worth a comment either here and/or in plugin-build/src/main/kotlin/io/sentry/android/gradle/autoinstall/AutoInstall.kt
explaining why distribution has a different implementation to the others
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that was my thought, it’s just easier to include it in here although it does make the task do two things instead of one. I’ll add a comment.
content { | ||
includeGroup("com.android.tools") | ||
} | ||
content { includeGroup("com.android.tools") } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You ran some linter/formatter over this and/or sentry-java
right as a big one off change before right? We should try get whatever you used added to the CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is weird. i just ran spotlessApply and it did this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think have a theory: We run spotless on all the modules but perhaps we don’t call spotlessCheck on the root project. I will open a separate PR once I investigate further.
When a variant has distribution enabled and autoInstallation is true, automatically add sentry-android-distribution dependency to that variant. Refs: EME-278 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add comments explaining why sentry-android-distribution uses a custom auto-install implementation instead of the standard InstallStrategy approach. Distribution requires variant-specific installation based on extension.distribution.enabledVariants, whereas other integrations are installed globally when their dependencies are detected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
92155d7
to
7e74c6a
Compare
Summary
When build distribution is enabled for a variant (via
distribution.enabledVariants
) andautoInstallation.enabled = true
, the plugin now automatically adds thesentry-android-distribution
dependency to that specific variant.Note: the library is only released as an alpha 8.24.0-alpha.1. You'll need to set
sentry.autoInstallation.sentryVersion="8.24.0-alpha.1"
for this to work.Changes
SENTRY_ANDROID_DISTRIBUTION
module toSentryModules
configureDistributionPropertiesTask
to add the distribution dependency per variant when both distribution and autoInstallation are enabledRefs: EME-278
#skip-changelog as this is part of the unreleased build distribution feature
🤖 Generated with Claude Code