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

Update test fixtures to run against AGP 4.1.0-beta04 #236

Merged
merged 24 commits into from
Jul 29, 2020

Conversation

fractalwrench
Copy link
Contributor

@fractalwrench fractalwrench commented Jul 20, 2020

Goal

Updates the latest test fixture to run against AGP 4.1.0-beta04. This changeset uses the new AGP API from 4.1.0 for altering the manifest, which was added in #234 and #240.

Note: as this changeset drops support for APK splits in AGP >=4.1.0 I'll wait for product confirmation before merging it.

Changeset

  • Updated test fixture to use AGP 4.1.0 from AGP 4.0.0
  • Included changes from Support for manifest processing in AGP 4.1.0-alpha04 and above #234 and Fix missing outputfile configuration #240 which use the new android.onVariants API for updating the manifest. This is only applied on AGP 4.1.0+.
  • The bugsnag manifest processing task is now registered for each ApkVariant on AGP 4.1.0, whereas on lower AGP versions it is registered for each ApkVariantOutput. This is because there does not appear to be a good API for supporting APK splits on AGP 4.1.0 as they have been superseded by App Bundles
  • Moved APK split E2E scenarios to only run against AGP 3.4.0.

@fractalwrench
Copy link
Contributor Author

@ZacSweers thought it'd be worth flagging up that the manifest changes seem to be failing when run against an example app using AGP 4.1.0-beta04. The error message seems to suggest that the input/output values haven't been set:

org.gradle.api.InvalidUserDataException: No value has been specified for property 'outputManifest

I'll keep the changes from #234 merged for now and think any fix can be applied using this PR. If you're still happy to contribute, would you have any idea what might be causing this behaviour? If not I can take the time to look at this myself.

@ZacSweers
Copy link
Contributor

Yep just that I forgot to set the output path in my previous PR :). We should just set a path in build/intermediates/bugsnag/... like the other one. I can make a PR and target this branch if that works?

@ZacSweers
Copy link
Contributor

This patch should work

Index: src/main/kotlin/com.bugsnag.android.gradle/BugsnagPlugin.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/main/kotlin/com.bugsnag.android.gradle/BugsnagPlugin.kt	(revision 6de8a1a0395d22280d4d4fb5e5c552423bd5ef5a)
+++ src/main/kotlin/com.bugsnag.android.gradle/BugsnagPlugin.kt	(date 1595223611000)
@@ -122,9 +122,11 @@
         val buildUuidProvider = project.provider { UUID.randomUUID().toString() }
         val manifestInfoOutputFile = project.layout.buildDirectory.file("intermediates/bugsnag/manifestInfoFor${taskNameForOutput(output)}.json")
         return if (BugsnagManifestUuidTaskV2.isApplicable()) {
+            val processedManifestOutput = project.layout.buildDirectory.file("intermediates/bugsnag/processed${taskNameForOutput(output)}Manifest.xml")
             val manifestUpdater = project.tasks.register(taskName, BugsnagManifestUuidTaskV2::class.java) {
                 it.buildUuid.set(buildUuidProvider)
                 it.manifestInfoProvider.set(manifestInfoOutputFile)
+                it.outputManifest.set(processedManifestOutput)
             }
             val android = project.extensions.getByType(BaseAppModuleExtension::class.java)
             android.onVariants.withName(variant.name) {

@fractalwrench
Copy link
Contributor Author

I can make a PR and target this branch if that works?

That would be great! It's worth noting that inputManifest is also not set on the task so the patch doesn't seem to work as-is.

@ZacSweers
Copy link
Contributor

hmm, inputManifest should be getting set by AGP for you there. What's the full error?

@fractalwrench fractalwrench marked this pull request as ready for review July 28, 2020 13:41
Copy link
Member

@imjoehaines imjoehaines left a comment

Choose a reason for hiding this comment

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

Changes look good, but I wonder if we should keep 4.0 on CI and add new fixtures for 4.1

@fractalwrench
Copy link
Contributor Author

CI comment has now been addressed in #246

Increase test matrix for E2E scenarios
@fractalwrench fractalwrench merged commit 45f636f into v5 Jul 29, 2020
@fractalwrench fractalwrench deleted the v5-fixture-dep-bump branch July 29, 2020 15:43
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.

None yet

3 participants