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

Upgrade Android gradle plugin #633

Merged
merged 10 commits into from
Oct 9, 2024
Merged

Upgrade Android gradle plugin #633

merged 10 commits into from
Oct 9, 2024

Conversation

tustanivsky
Copy link
Collaborator

This PR is supposed to address Android symbol upload issues that some Sentry self-hosted users faced previously due to the legacy gradle plugin being in use (i.e. #502).

In order to bump the Android gradle plugin version it was necessary to override engine's gradle version as well by replacing gradle-wrapper.properties file in build dir.

@tustanivsky tustanivsky marked this pull request as ready for review September 10, 2024 13:26
Copy link
Contributor

github-actions bot commented Sep 10, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against d7d15e1

CHANGELOG.md Outdated Show resolved Hide resolved
/LinuxIntermediate/Build/Linux/UnrealGame/Shipping/Sentry/*
/LinuxIntermediate/Build/Linux/x64/UnrealGame/Development/Sentry/*
/LinuxIntermediate/Build/Linux/x64/UnrealGame/Shipping/Sentry/*
/Gradle/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that change intentional? The how do the LinuxIntermediate related to Gradle?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, aside from adding Gradle here I've also removed LinuxIntermediate stuff which is now redundant (probably I've missed deleting these entries earlier). Should we tackle this kind of clean-up in a separate PR?

@@ -137,7 +141,7 @@
<buildscriptGradleAdditions>
<insert>
dependencies {
classpath 'com.android.tools.build:gradle:3.5.4'
classpath 'com.android.tools.build:gradle:7.4.2'
Copy link
Contributor

Choose a reason for hiding this comment

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

That's the gradle tool itself, right? And that's a requirement for the sentry-android-gradle-plugin 4.11.0?
Are we going to mess with user's build by messing with the gradle version that comes with the UE installation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the io.sentry.android.gradle >= 3.0.0 requires Android Gradle Plugin >= 7.0.0 according to docs.
I've tested this with a few different engine versions (including pretty old UE 4.27) and the build was successful however there's always a chance that some compatibility issues with 3rd-party plugins can arise.

@tustanivsky
Copy link
Collaborator Author

UPD: Added new plugin setting which allows switching back to legacy Sentry Android Gradle plugin in case of any compatibility issues with other third-party plugins

Copy link
Contributor

@bitsandfoxes bitsandfoxes left a comment

Choose a reason for hiding this comment

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

I'm not sure I see how this fixes the issue linked. The issue was resolved by adding the defaults url, right? How does the newer Android Gradle Plugin help with this? Is there anything else we're getting out of it?

The fallback is great, I really appreciate it, changing the user's Gradle version still makes me nervous.

</insert>
</true>
<false>
<log text="Using Sentry Gradle plugin 4.11.0 for Android debug symbol upload."/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the hardcoded version number going to bite us in the butt later on?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we don't have any means allowing to check how Sentry Gradle plugin performs in our CI probably it'd be safer to keep a hardcoded version here instead of bumping it automatically for the time being.

@tustanivsky
Copy link
Collaborator Author

I'm not sure I see how this fixes the issue linked. The issue was resolved by adding the defaults url, right? How does the newer Android Gradle Plugin help with this? Is there anything else we're getting out of it?

With the newer Gradle plugin version adding defaults.url to sentry.properties when using Sentry self-hosted shouldn't be required any longer. It was a legacy issue but now the right URL can be extracted from the authentication token so users won't face the problem with uploading symbols.

Copy link
Contributor

@bitsandfoxes bitsandfoxes left a comment

Choose a reason for hiding this comment

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

Considering that we cannot offer ootb support for self-hosted and there is a fallback/opt-out for those that need it: LGTM! 🔥

@bitsandfoxes bitsandfoxes merged commit 024ef21 into main Oct 9, 2024
13 checks passed
@bitsandfoxes bitsandfoxes deleted the feat/gradle-plugin-bump branch October 9, 2024 10:05
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