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

Add BUILD_UUID to app bundle manifests #153

Merged
merged 12 commits into from
Mar 21, 2019
Merged

Add BUILD_UUID to app bundle manifests #153

merged 12 commits into from
Mar 21, 2019

Conversation

simonbowring
Copy link
Contributor

@simonbowring simonbowring commented Mar 15, 2019

Goal

Add a Build UUID to app bundle manifests.

The Build UUID currently only gets added when generating APKs directly without using app bundles.

Changeset

  • Return a path to the bundle manifest when generating an app bundle rather than the APK manifest
  • Add the Build UUID to the bundle manifest rather than the merged manifest when an app bundle is being generated
  • Use the Gradle task graph to determine whether an assemble task (generate APK) is being run or bundle task (generate app bundle)
  • When manually uploading a mapping file, determine which manifest file to read based on which one has a Build UUID.
    • Incidentally, both a merged manifest and bundle manifest get generated for assemble and bundle tasks, but we only put the build UUID into one of them.

Tests

Made sure that the mazerunner tests pass.

Manual tests:

  • Check that an app built with the bundleDebug Gradle task contains the Build UUID in the manifest in the app bundle
    • Checked with AGP versions:
      • 3.2.1
      • 3.3.0
      • 3.3.2
  • Check that an app built with the assembleDebug Gradle task contains the Build UUID in the manifest in the APK
    • Checked with AGP versions:
      • 3.0.1 (pre-app bundles)
      • 3.2.1
      • 3.3.0
      • 3.3.2
  • Check that manually uploading a mapping file after building a bundle then the build UUID is read from the bundle manifest
  • Check that manually uploading a mapping file after building an APK then the build UUID is read from the merged manifest

@simonbowring simonbowring requested a review from a team March 15, 2019 15:23
Copy link
Contributor

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

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

I left one comment inline, otherwise I believe this should work for the common case. We should verify in an example app running AGP 3.3.0, and one running AGP 3.0.1, which won't have App Bundles. We should also test whether this fix works with DexGuard - I'm not sure that it will as currently implemented.

This approach is not ideal in the long-term as it effectively doubles the amount of IO each task needs to perform, regardless of whether App Bundles are actually being used. If other manifest locations are added to the build folder in future, then this will become even more of a problem. Also I think this approach is likely to make it harder to make this task incremental, which has been scheduled in the past.

If we can verify that the plugin works for the above scenarios then I'm happy to approve as a short-term solution, with the caveat that we should revisit the design at a later date.

@simonbowring
Copy link
Contributor Author

I've made a change to the approach so that either the merged manifest, the bundle manifest or both are written to depending on which tasks are in the Gradle task graph for each variant.

This has also been tested with an app running AGP 3.3.0, and one running AGP 3.0.1

Tasks left to do:

  • Test with DexGuard
  • Get mazerunner tests working

// Get the Bugsnag API key
apiKey = getApiKey(metaDataTags, ns)
if (!apiKey) {
project.logger.warn("Could not find apiKey in '$BugsnagPlugin.API_KEY_TAG' <meta-data> tag in your AndroidManifest.xml or in your gradle config")
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we've recently changed this to throw an exception in #151

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fractalwrench
Copy link
Contributor

fractalwrench commented Mar 19, 2019

I believe the mazerunner tests are failing as the fixture's gradle wrapper has been updated, which seems to alter the order in which requests are sent. Setting it back to the previous version passes the scenarios, and the failing scenarios appear to be in the wrong order when inspecting the maze_output directory. I don't think this is an issue in a real world scenario and is just something that needs fixing in our tests.

@simonbowring
Copy link
Contributor Author

I've reverted the gradle and AGP version changes in the maze fixture.

@fractalwrench
Copy link
Contributor

I'm happy to approve this as a short-term fix with the caveat that this has been tested to work with DexGuard. We should create a ticket to address previous concerns I've raised about this general approach at a later date.

@simonbowring
Copy link
Contributor Author

I've tested this with DexGuard, deobfuscation is working.

@simonbowring simonbowring changed the base branch from master to next March 21, 2019 13:22
@simonbowring simonbowring merged commit e20ec55 into next Mar 21, 2019
@simonbowring simonbowring deleted the app-bundle-uuid branch March 21, 2019 13:28
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.

2 participants