Remove mips and armeabi as supported ABIs #11458

tobrun · 2018-03-15T12:15:04Z

Capturing from ndk-guides that:

armeabi and mips were deprecated in r16 and will be removed in r17

This PR adds the required changes to build with NDK 17.

cc @mapbox/maps-android

tobrun · 2018-03-15T12:27:12Z

PR'ed too soon, also hitting the following when compiling locally against NDK 17:

This error points to src/mbgl/renderer/buckets/fill_extrusion_bucket.cpp:105

Any ideas how to correctly resolve this issue?

tobrun · 2018-03-15T12:30:10Z

Also hitting:

for bitmap.cpp:

kkaefer · 2018-03-15T12:35:03Z

Yeah, we should never throw in destructors; it's undefined behavior.

jfirebaugh · 2018-03-15T16:52:56Z

The -Wtautological-constant-compare warning is a good catch: dist is int16_t, so this comparison is always false:

mapbox-gl-native/src/mbgl/renderer/buckets/fill_extrusion_bucket.cpp

Line 105 in 11d506f

if (dist > std::numeric_limits<int16_t>::max()) {

@lbud do you recall what the function of this comparison is intended to be?

tobrun · 2018-03-22T22:19:01Z

I'm putting this PR on hold til official release of NDK17 + CI image update mapbox/mbgl-ci-images#16.

tobrun · 2018-05-11T09:40:25Z

Working back on this after updating the mbgl-ci-image, addressed previous comments but now hitting:

../../../../../../../mason_packages/headers/boost/1.65.1/include/boost/iostreams/seek.hpp:85:20: error: comparison 'boost::iostreams::stream_offset' (aka 'long') < -9223372036854775808 is always false [-Werror,-Wtautological-constant-compare]
( off < integer_traitsstd::streamoff::const_min ||
~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../../../../mason_packages/headers/boost/1.65.1/include/boost/iostreams/seek.hpp:86:20: error: comparison 'boost::iostreams::stream_offset' (aka 'long') > 9223372036854775807 is always false [-Werror,-Wtautological-constant-compare]
off > integer_traitsstd::streamoff::const_max ) )
~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../../../../mason_packages/headers/boost/1.65.1/include/boost/iostreams/seek.hpp:112:20: error: comparison 'boost::iostreams::stream_offset' (aka 'long') < -9223372036854775808 is always false [-Werror,-Wtautological-constant-compare]
( off < integer_traitsstd::streamoff::const_min ||
~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../../../../mason_packages/headers/boost/1.65.1/include/boost/iostreams/seek.hpp:113:20: error: comparison 'boost::iostreams::stream_offset' (aka 'long') > 9223372036854775807 is always false [-Werror,-Wtautological-constant-compare]
off > integer_traitsstd::streamoff::const_max ) )
~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4 errors generated.

These errors are similar to #11458 (comment) but are found in boost that we fetch from Mason. Any guidance how I can fix this?

mb12 · 2018-05-11T20:18:42Z

@tobrun To find out if there are any other issues with the new NDK, you can remove -Werror from CMAKE_CXX_FLAGS in CMakeLists.txt. This may unblock you from making progress. Some mapbox projects like vtzero allow pasisng -DWERROR=OFF for precisely these kind of experiments. Eventually we should add something similar that we can use in MapboxGLAndroidSDK/build.gradle.

kkaefer · 2018-05-14T10:06:24Z

@tobrun we're using this pattern to silence the warnings from external libraries:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wno-tautological-constant-compare"
#include <boost/...>
#pragma GCC diagnostic pop

Technically, we should include them with -isystem instead of -I, but that is blocked by CMake's Xcode generator not supporting them yet.

kkaefer · 2018-05-14T10:07:22Z

platform/android/src/bitmap.cpp

-        if (result != ANDROID_BITMAP_RESULT_SUCCESS) {
-            throw std::runtime_error("bitmap decoding: could not unlock pixels");
-        }
+        AndroidBitmap_unlockPixels(&env, jni::Unwrap(*bitmap));


Can we replace this with a warning?

kkaefer · 2018-05-14T10:07:49Z

platform/android/src/run_loop.cpp

-    if (close(fds[PIPE_IN]) || close(fds[PIPE_OUT])) {
-        throw std::runtime_error("Failed to close file descriptor.");
-    }
-


Why are these calls no longer necessary?

they still are, replaced the throw std::runtime_error(); with logging an error.

kkaefer · 2018-05-14T10:07:55Z

src/mbgl/renderer/buckets/fill_extrusion_bucket.cpp

@@ -102,9 +102,6 @@ void FillExtrusionBucket::addFeature(const GeometryTileFeature& feature,

                    const Point<double> perp = util::unit(util::perp(d1 - d2));
                    const auto dist = util::dist<int16_t>(d1, d2);
-                    if (dist > std::numeric_limits<int16_t>::max()) {
-                        edgeDistance = 0;
-                    }


the compiler warned that this check was always false, didn't see an issue removing it.

Hmm yeah, but I think this means that we should change the underlying code that leads to dist being a uint16_t, since the overflow will be hidden there.

Restored the check and added silenced the warning for now to make it compile:
56ff8b1#diff-f07899604a0493652b354b7d538040b1R107

How would you like to proceed? ticketing this in a follow up ticket or fixing it in this PR?

followup ticket 👍

tobrun · 2018-05-15T07:30:34Z

@kkaefer this PR is ready for another round of 👀, follow up ticket in #11909.
@LukasPaczos would you be able to 👀 the android changes?

LukasPaczos · 2018-05-15T08:40:34Z

platform/android/gradle/dependencies.gradle

-            compileSdkVersion: 25,
-            buildToolsVersion: '26.0.3'
+            targetSdkVersion : 26,
+            compileSdkVersion: 26,


Can we bump compileSdkVersion to the 27 while we are at it?

LukasPaczos

Good on my end 👍

[android] - bump CI image to NDK 17 compatible [core] - remove setting edgeDistance to 0, comparison 'const short' > 32767 is always false [android] - remove throwing in desructor, undefined behaviour [android] - bump dependency versions of project

Jose4gg · 2018-05-22T14:58:33Z

Is there a way that I don't have to wait for this PR and still use NDK 17.x?

tobrun · 2018-05-22T15:07:46Z

@Jose4gg not afaik, I hit the same issue after making this PR and was only able to compile the project again by downgrading my ndk. @kkaefer can I get a second pair of 👀on this?

Jose4gg · 2018-05-22T15:26:03Z

Is there an ETA for this PR?

tobrun added the Android Mapbox Maps SDK for Android label Mar 15, 2018

tobrun added this to the android-v7.0.0 milestone Mar 15, 2018

tobrun self-assigned this Mar 15, 2018

tobrun requested a review from kkaefer March 15, 2018 12:15

tobrun force-pushed the tvn-ndk-17 branch from ac9650e to 85f26a2 Compare March 15, 2018 12:19

tobrun added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Mar 15, 2018

tobrun mentioned this pull request Mar 25, 2018

Camera issues on armeabi only binaries #11517

Closed

jfirebaugh mentioned this pull request Apr 3, 2018

-Wtautological-constant-compare warning with clang++ 6 #11586

Closed

tobrun force-pushed the tvn-ndk-17 branch from 85f26a2 to 11bbde4 Compare May 11, 2018 09:40

kkaefer reviewed May 14, 2018

View reviewed changes

tobrun force-pushed the tvn-ndk-17 branch 5 times, most recently from 3c525e4 to 78517b9 Compare May 14, 2018 12:47

tobrun mentioned this pull request May 15, 2018

autological-constant-compare in fill_extrusion_bucket.cpp #11909

Closed

tobrun requested a review from LukasPaczos May 15, 2018 07:30

tobrun added ✓ ready for review and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels May 15, 2018

LukasPaczos reviewed May 15, 2018

View reviewed changes

tobrun force-pushed the tvn-ndk-17 branch from 78517b9 to d55d1ef Compare May 16, 2018 08:11

LukasPaczos approved these changes May 16, 2018

View reviewed changes

tobrun force-pushed the tvn-ndk-17 branch from d55d1ef to 536a49d Compare May 17, 2018 16:33

tobrun merged commit 60505b0 into master May 22, 2018

tobrun deleted the tvn-ndk-17 branch May 22, 2018 15:53

LukasPaczos mentioned this pull request Nov 27, 2018

Android SDK 6.7.0 won't generate libmapbox-gl.so under armeabi ? #13448

Closed

zestyping mentioned this pull request May 25, 2019

Reduce size of APK getodk/collect#3097

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove mips and armeabi as supported ABIs #11458

Remove mips and armeabi as supported ABIs #11458

tobrun commented Mar 15, 2018

tobrun commented Mar 15, 2018

tobrun commented Mar 15, 2018

kkaefer commented Mar 15, 2018

jfirebaugh commented Mar 15, 2018

tobrun commented Mar 22, 2018

tobrun commented May 11, 2018

mb12 commented May 11, 2018

kkaefer commented May 14, 2018

kkaefer May 14, 2018

kkaefer May 14, 2018

tobrun May 14, 2018

kkaefer May 14, 2018

tobrun May 14, 2018

kkaefer May 14, 2018

tobrun May 14, 2018

kkaefer May 14, 2018

tobrun May 15, 2018

tobrun commented May 15, 2018

LukasPaczos May 15, 2018

LukasPaczos left a comment

Jose4gg commented May 22, 2018

tobrun commented May 22, 2018 •

edited

Loading

Jose4gg commented May 22, 2018

Remove mips and armeabi as supported ABIs #11458

Remove mips and armeabi as supported ABIs #11458

Conversation

tobrun commented Mar 15, 2018

tobrun commented Mar 15, 2018

tobrun commented Mar 15, 2018

kkaefer commented Mar 15, 2018

jfirebaugh commented Mar 15, 2018

tobrun commented Mar 22, 2018

tobrun commented May 11, 2018

mb12 commented May 11, 2018

kkaefer commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobrun commented May 15, 2018

Choose a reason for hiding this comment

LukasPaczos left a comment

Choose a reason for hiding this comment

Jose4gg commented May 22, 2018

tobrun commented May 22, 2018 • edited Loading

Jose4gg commented May 22, 2018

tobrun commented May 22, 2018 •

edited

Loading