Manually calculate location of objdump in gradle plugin #136
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Updates the NDK fixture project to use bugsnag-android-ndk v4+, and to build all the default supported abis, rather than just armeabi-v7a and x86. The scenarios have also been updated to check that the correct architecture is set.
Verifies that the calculated location of objdump follows a given pattern when the ABI, os name, and NDK directory differ.
Calculates the location of objdump manually. The location of the file depends on 3 factors: the ABI of the shared object file being processed, the operating system, and the installation location of the NDK bundle.
If the objdump location changes in future versions of the AGP, it would be helpful to override the objdump location on a per-ABI basis - which this property provides. A mazerunner scenario has been added to verify the behaviour.
bengourley
reviewed
Dec 10, 2018
| return "windows-x86_64" | ||
| } | ||
| } else { | ||
| return "unknown" |
There was a problem hiding this comment.
is there something better we can do here? such as raising an exception or logging a warning? Tell people to use the objdumpPaths option? I think once you reach this branch and it builds a string that we know won't work, the error message will be confusing or missing enough detail to work out what went wrong
There was a problem hiding this comment.
This is a good suggestion - I've updated the function to return null if the osName can't be calculated. I've then added a null check for ABI + osName, which throws an exception that will ultimately be caught and logged to warn that objdump couldn't be found.
bengourley
approved these changes
Dec 10, 2018
Pezzah
approved these changes
Dec 11, 2018
…ather than creating a non-exist
bengourley
approved these changes
Dec 11, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Goal
The bugsnag gradle plugin relies on objdump to upload shared object mapping files, which deobfuscate stacktraces in NDK projects. The plugin relies on the
NdkHandlerandToolchainclasses to calculate the location of objdump on disk. These classes are internal APIs within the Android Gradle Plugin (AGP), and have been removed in v3.4.0-alpha.The bugsnag gradle plugin will throw an Error when attempting to load these classes, which will break the build for any NDK project depending on our plugin and AGP 3.4.0. This changeset alters the plugin to perform the calculation manually, and removes the dependency on these classes.
N.B. a design doc is available which covers the goal in more detail
Changeset
Toolchain,NdkHandler, andAbiclasses which were internal to the AGP.Abiclasses which contains pre-calculated information on the names of ABIs, and the expected location of objdump.objdumpPathsproperty to the bugsnag plugin extension which allows users to override the calculation location on a per-ABI basisTests
objdumpPathsproperty overrides the calculated objdump location.support.
Discussion
Linked issues
Fixes #134