Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Only perform thread checking in debug builds #14293

Merged
merged 1 commit into from
Apr 12, 2019
Merged

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Apr 2, 2019

Closes #14287, possible solution alternatively the static lazy initialization can be replaced if we can ask users to provide their BuildConfig.DEBUG value through Mapbox.getInstance.

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Apr 2, 2019
@tobrun tobrun added this to the release-m milestone Apr 2, 2019
@tobrun tobrun self-assigned this Apr 2, 2019
@tobrun tobrun requested a review from LukasPaczos April 2, 2019 10:27
@LukasPaczos
Copy link
Member

Another alternative would be generating the ThreadUtils class during build time (annotation processing step or something similar), depending on config - returning for release and crashing for debug. This would allow us to skip build type checks introduced here. Maybe a library like AutoService could be useful here. What do you think?

@tobrun
Copy link
Member Author

tobrun commented Apr 2, 2019

I'm not sure that would work as our users library are always consuming the release binary. For them thread checking will always be disabled. What is proposed in this PR is to check against the debug/release state of the consuming app.

@LukasPaczos
Copy link
Member

True, unless we ship a processor in a separate artifact that replaces the default (slow) implementation, and possibly unlocks other code generation features. Interested users could include that artifact in their build scripts. Or does that sound like overkill?

@tobrun
Copy link
Member Author

tobrun commented Apr 8, 2019

We will be looking into #14293 (comment) as alternative approach to fixing this issue. Closing this PR for now.

@tobrun tobrun closed this Apr 8, 2019
@maxtracking
Copy link

Hi @tobrun,

Any idea of ETA for the alternative solution ?

Alex

@tobrun
Copy link
Member Author

tobrun commented Apr 10, 2019

No concrete ETA but we are trying to pick this up for upcoming release aimed at end of the month. If we don't hit that, it will be included in the release of next month.

@LukasPaczos
Copy link
Member

ship a processor in a separate artifact that replaces the default (slow) implementation

I've done my due research on the topic:

  • We are unable to write into or replace a class file during the annotation processing step in a stable manner. There might be some ways of achieving the desired effect, but those are dependent on too many assumptions regarding the local build environment, which we don't want to do.
  • There's a way of replacing a class file from the Gradle's perspective. It requires us to bypass the typical maven setup by excluding the aar, downloading the artifact on the side, unpacking the classes jar, replacing the class file and re-including that artifact in the build.

Both of the above sound like a hell to maintain.

The only realistic way of doing a completely clean build-type optimization, without any additional checks, would be not including ThreadUtils class in the Maps SDK's aar at all while simultaneously shipping an annotation processor (or a Gradle plugin) that does the code generation, and forcing end users to depend on it.

I don't think that offered features would benefit users enough to force that dependency now, but it's something we can revisit in the future.

In the meantime, I'm reopening this PR as a way to tackle the original issue.

@LukasPaczos LukasPaczos reopened this Apr 10, 2019
Copy link
Member

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

🚢

@tobrun tobrun force-pushed the tvn-thread-utils branch 4 times, most recently from e115b08 to b5b58a5 Compare April 12, 2019 11:31
…rating app. Use constants for source when thread checking
@tobrun tobrun merged commit b4d5890 into master Apr 12, 2019
@tobrun tobrun deleted the tvn-thread-utils branch April 12, 2019 14:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to switch off thread checks in release
3 participants