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

[android] Replace libuv with Looper #4280

Merged
merged 3 commits into from
Apr 6, 2016
Merged

[android] Replace libuv with Looper #4280

merged 3 commits into from
Apr 6, 2016

Conversation

tmpsantos
Copy link
Contributor

Looper ships with Android and can provide the same functionality.

@tmpsantos
Copy link
Contributor Author

👀 @jfirebaugh @tobrun @zugaldia @bleege

This is a step towards #2909

@tmpsantos tmpsantos added ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold ✓ ready for review and removed in progress labels Mar 11, 2016
@tmpsantos
Copy link
Contributor Author

Marking as DO NOT MERGE because although it is apparently working just fine, I would feel more comfortable after getting the utests running on Android.

@zugaldia
Copy link
Member

@tmpsantos thank you!

@tobrun let's schedule a new battery of tests on Monday?

@zugaldia
Copy link
Member

Related: #3440

@tobrun
Copy link
Member

tobrun commented Mar 11, 2016

@zugaldia we should look into those performance tests we did . These test should be able to give us some extra information about if it's less/more performant by using Traceview.

@jfirebaugh
Copy link
Contributor

Also noting that we want to hold off merging this until after we branch for the 4.0.0 release.

@tobrun tobrun added this to the android-v4.1.0 milestone Mar 11, 2016
@tmpsantos tmpsantos force-pushed the tmpsantos-looper branch 2 times, most recently from 6ae06dd to 64d9a71 Compare March 22, 2016 03:08
@bleege bleege added the Android Mapbox Maps SDK for Android label Apr 4, 2016
@bleege
Copy link
Contributor

bleege commented Apr 5, 2016

Discussed with @tmpsantos in chat and will pick this up now. I'll build it locally to make sure the TestApp works as expected. From there I'll rebase and make sure CI is happy.

@bleege
Copy link
Contributor

bleege commented Apr 5, 2016

Local tests worked great! I'm going to rebase now and push back to the branch so that we can run the CI on it before merging back to master.

@bleege
Copy link
Contributor

bleege commented Apr 5, 2016

Rebased, but the only way to push to GitHub required a git pull first from the remote branch which created a merge commit. Everything appears to be as expected. Will wait for CI to playout before merging to master.

@bleege
Copy link
Contributor

bleege commented Apr 5, 2016

All CI save the Android build passed. Will explore.

@bleege
Copy link
Contributor

bleege commented Apr 5, 2016

Error turned out to be the following. Will try to resolve locally and push again for CI.

�[1m�[32m* Recreating project...�[0m
gyp: Undefined variable libuv_cflags in gyp/mbgl.gyp
make[1]: Leaving directory `/bitrise/src'
make[1]: *** [Makefile/__project__] Error 1
make: *** [android-lib] Error 2

@bleege
Copy link
Contributor

bleege commented Apr 5, 2016

As best I can tell some other C++ configs shifted between when @tmpsantos first wrote his 3 commits and now. To rule out a possible messed up rebase and merge, I created a new branch based off master today (commit 81261ca) and then tried cherrypicking @tmpsantos's original 3 commits from this PR on to it. I did this in a time sequential manor to mirror @tmpsantos original work and then ran make android after each cherrypick. The first two (Introduce and Attach RunLoop) both cherrypicked cleanly and executed make android just fine. However when the third commit (Replace libuv with Looper) was cherrypicked the make android command ended with the same error as above. Based on what I can see (see attached screenshot) it appears that the build script is still trying to build with libuv as indicated by the loop=uv print out.

  • Introduce RunLoop ( a610d14 )
  • Attach thread on RunLoop ( 9b15e6e )
  • Replace libuv With Looper ( ea699ec )

screen shot 2016-04-05 at 4 25 40 pm
Is loop=uv Still Being Used?

@jfirebaugh
Copy link
Contributor

Give me a second and I'll be available to fix this up.

@bleege
Copy link
Contributor

bleege commented Apr 5, 2016

Forgot to mention that my test branch is called 3440-replace-libuv and is available for 👀 at:

https://github.com/mapbox/mapbox-gl-native/commits/3440-replace-libuv

Also implement a Timer and AsyncTask based on Android's Looper.
Needed because Looper could be used by Java and it would crash
complaining that we didn't detach otherwise.
Build using Looper instead, provided by the NDK.
@tmpsantos tmpsantos removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Apr 6, 2016
@tmpsantos tmpsantos merged commit 4b05705 into master Apr 6, 2016
@tmpsantos tmpsantos deleted the tmpsantos-looper branch April 6, 2016 08:43
@tmpsantos
Copy link
Contributor Author

Got it fixed and merged!

@bleege
Copy link
Contributor

bleege commented Apr 6, 2016

Ah ha! Libuv had to also be turned off in defaults.mk Thanks again for making this happen @tmpsantos!

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.

5 participants