Skip to content
This repository was archived by the owner on Jun 17, 2024. It is now read-only.

Bug 1870050 - Upgrade AGP and kotlin dsl version. #4797

Merged
merged 8 commits into from
Feb 5, 2024

Conversation

mcarare
Copy link
Contributor

@mcarare mcarare commented Dec 11, 2023

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Breaking Changes: If this is a breaking Android Components change, please push a draft PR on Reference Browser to address the breaking issues.

To download an APK when reviewing a PR (after all CI tasks finished running):

  1. Click on Checks at the top of the PR page.
  2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
  3. Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
  4. Click on View task in Taskcluster in the new DETAILS section.
  5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1870050

@mcarare
Copy link
Contributor Author

mcarare commented Dec 11, 2023

🧪

@github-actions github-actions bot added the work in progress Not ready to land yet. Work in progress (WIP). label Dec 11, 2023
@mozilla-mobile mozilla-mobile deleted a comment from github-actions bot Dec 12, 2023
@mozilla-mobile mozilla-mobile deleted a comment from github-actions bot Dec 12, 2023
@mozilla-mobile mozilla-mobile deleted a comment from github-actions bot Dec 12, 2023
@mozilla-mobile mozilla-mobile deleted a comment from github-actions bot Dec 12, 2023
@mcarare mcarare force-pushed the agp branch 3 times, most recently from c9dfc79 to 4994045 Compare December 14, 2023 14:51
@mcarare mcarare changed the title Upgrade AGP and kotlin dsl version. Bug 1870050 - Upgrade AGP and kotlin dsl version. Dec 14, 2023
@mozilla-mobile mozilla-mobile deleted a comment from github-actions bot Dec 14, 2023
@mozilla-mobile mozilla-mobile deleted a comment from github-actions bot Dec 14, 2023
@mozilla-mobile mozilla-mobile deleted a comment from github-actions bot Dec 14, 2023
@mcarare mcarare force-pushed the agp branch 3 times, most recently from c433151 to 7bb1c4a Compare December 15, 2023 13:35
@mcarare mcarare force-pushed the agp branch 2 times, most recently from f73c063 to 84e35cd Compare December 28, 2023 11:47
@mcarare mcarare force-pushed the agp branch 4 times, most recently from a9286ae to f7b9821 Compare January 25, 2024 15:35
@mcarare mcarare marked this pull request as ready for review January 31, 2024 12:17
@github-actions github-actions bot added 🕵️‍♀️ needs review PRs that need to be reviewed and removed work in progress Not ready to land yet. Work in progress (WIP). labels Jan 31, 2024
Copy link
Collaborator

@jonalmeida jonalmeida left a comment

Choose a reason for hiding this comment

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

These changes look fine to me on the surface. I haven't run it locally, but nothing here looks suspicious enough to warrant that.

@@ -3,7 +3,7 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

plugins {
id "org.gradle.kotlin.kotlin-dsl" version "4.1.2"
id "org.gradle.kotlin.kotlin-dsl" version "4.2.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Woohoo! Goodbye warnings!

Is there a way to move this version to the DependenciesPlugin or catalogs in the future? I wouldn't change your PR today, maybe in a future revision so we can land this sooner.

@@ -121,7 +121,7 @@ class FenixSnackbar private constructor(
* Suppressing ComplexCondition. Yes it's unfortunately complex but that's the nature
* of the snackbar handling by Android. It will be simpler once dynamic toolbar is always on.
*/
@Suppress("ComplexCondition")
@Suppress("ComplexCondition", "RestrictedApi")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we file an issue to remove access to what is causing this restricted API?

Same question for the one in fenix/app/src/main/java/org/mozilla/fenix/ext/Toolbar.kt too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -180,11 +180,13 @@
<service
android:name=".downloads.DownloadService"
android:exported="false"
android:foregroundServiceType="dataSync" />
android:foregroundServiceType="dataSync"
tools:ignore="ForegroundServicePermission" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we ignore this here? Is it because we request this permission at runtime? If so, can we leave that as a note here: "Permission requested at runtime."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like a false positive because the required permission is in the merged manifest and not in the Focus manifest. An alternative would be adding the permissions explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it would be better for future references to have the permissions declared explicitly instead of having suppressions ( that would likely be fixed in future lint releases)

@github-actions github-actions bot added the approved PR that has been approved label Feb 1, 2024
@jonalmeida
Copy link
Collaborator

lgtm!

@mcarare
Copy link
Contributor Author

mcarare commented Feb 2, 2024

Merging this on Monday.

@mcarare mcarare added the 🛬 needs landing PRs that are ready to land label Feb 5, 2024
@mergify mergify bot merged commit 5ab7a32 into mozilla-mobile:main Feb 5, 2024
298 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved PR that has been approved 🛬 needs landing PRs that are ready to land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants