Differential D3646

Ignore host libs/includes/cmakeconfig files in Android toolchain
ClosedPublic
Actions

Authored by kossebau on Dec 11 2016, 9:52 PM.

Details

Reviewers
nienhueser
apol
mutlaqja
cordlandwehr
sandsmark
Group Reviewers
GCompris
Frameworks
Minuet
Commits
R240:5f17bf2c0c65: Ignore host libs/includes/cmakeconfig files in Android toolchain
Summary

Currently (since 123d0d14017a25fb387efd8fe3c2c1323f9c3815) any
find_library() and find_path() calls look both at the host and the toolchain
paths, which often results in includes and/or libraries wrongly being picked
up from the host system, which then results in a failed build.
CMake config files have always been also looked for on the host, which
most often also is not wanted and resulting in a failed build.

This patch fixes that by changing the mode for finding libraries,
includes & packages to ONLY (again), as also recommended in the
cmake-toolchains documentation.

While before CMAKE_PREFIX_PATH was recommended to let cmake e.g.
discover the Qt5 for Android libs, this patch wants a custom
variable ECM_ADDITIONAL_FIND_ROOT_PATH to be used instead.
Reason is that CMAKE_PREFIX_PATH would be subject to the root
handling, while here instead the root paths themselves are
wanted.
This patch does not add backward compatibility for still passing
the Qt5 install prefix via CMAKE_PREFIX_PATH, as there are not
that many users known yet and the old code did not work for many
anyway, so the extra code hassle is not worth it. Instead the few
build instructions would need to be updated (and should ask to use
latest ECM in any case).

Test Plan

Building Marble now works without trying to use stuff from the
host system.

Diff Detail

Repository
R240 Extra CMake Modules
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau updated this revision to Diff 8932.Dec 11 2016, 9:52 PM
kossebau retitled this revision from to Ignore host libs/includes/cmakeconfig files in Android toolchain.
kossebau updated this object.
kossebau edited the test plan for this revision. (Show Details)
kossebau added reviewers: Frameworks, GCompris, Minuet, apol, cordlandwehr, mutlaqja, sandsmark, nienhueser.
nienhueser accepted this revision.Dec 12 2016, 5:12 AM
nienhueser edited edge metadata.

Thanks for fixing. I was stuck with an ancient ecm without it.

This revision is now accepted and ready to land.Dec 12 2016, 5:12 AM
kossebau added a comment.Dec 18 2016, 1:27 PM

Ping?`@apol, @cordlandwehr, any comment on this?
And does this improve or at least not regress with how GCompris & KStars Light developers build for Android?

I would like to avoid having to fork the toolchain file for Marble :/

cordlandwehr accepted this revision.Dec 18 2016, 3:01 PM
cordlandwehr edited edge metadata.

The patch absolutely goes into the right direction. All changes look fine to me. However, I did not test the patch yet, since I am currently in the process of updating my cross-compilation setup.
IMO it is also fine to not provide complete backward compatibility, since adapting is easy and a backward compatibility workaround would loose the sysroot separation that we just introduce with this patch.

apol added inline comments.Dec 18 2016, 6:59 PM
toolchain/Android.cmake
163

Maybe call it ECM_ALTERNATIVE_ROOT_PATH, in case we ever need to use it on other platforms as well?

188

This is unrelated. Also maybe while at it you can document it right away.
I just added it so that we can detect problems at config time rather than while linking.

214

That's not requried, as the NDK already passes the __ANDROID__.

kossebau added inline comments.Dec 19 2016, 12:37 AM
toolchain/Android.cmake
163

Hm, indeed a point in that ECM_ANDROID_ROOT_PATH sounds like this is the only one and might replace the root set by the ndk. So would agree a better term should be found.

ALTERNATIVE sounds a little bit strange to me initially, also not part of any existing terms in cmake/ecm vars for what I found. But given it is used in the description of CMAKE_FIND_ROOT_PATH (see https://cmake.org/cmake/help/v3.1/variable/CMAKE_FIND_ROOT_PATH.html?highlight=alternative) for the values passed here, and in the end is pretty self explaining.
Or something like ECM_ADDITIONAL_FIND_ROOT_PATH to pick up the cmake-naming and still point out this are just additional ones? Perhaps with a better term than ADDITIONAL?

188

So gnustl_shared is required by Qt for Android? Any link for backing this up which I could use to please curious people? I could not find anything when I quickly searched, so this here seemed just some magic checking :)

214

This was just moved around (see l.167 before), as part of grouping things together instead of interleaved setup (as also mentioed at end of summary text). Am to be blamed for trying too many things in one review, even if related, will see to keep things as simple as possible in the future.

This was added in faedc8d01697ab1d573d5740f24e7279f4dba14f.
So it was now found this would not be needed? Which Qt versions would __ANDROID__ work with? @cordlandwehr , your commit to be defended here, please tell :)

apol added inline comments.Dec 19 2016, 12:00 PM
toolchain/Android.cmake
188

It's where the STL lives: https://developer.android.com/ndk/guides/cpp-support.html

214

I was introduced in Qt in 499f9b2abfabacd254e1b23566343ac20322eb48 back in September 2015. If you didn't add it, then just leave it as is...

kossebau updated this revision to Diff 9166.Dec 19 2016, 2:27 PM
kossebau edited edge metadata.
  • Rename ECM_ANDROID_ROOT_PATH -> ECM_ADDITIONAL_FIND_ROOT_PATH
  • drop gnustl_shared documenting todo for a separate review
kossebau added inline comments.Dec 19 2016, 2:31 PM
toolchain/Android.cmake
188

Moved to separate https://phabricator.kde.org/D3732

apol accepted this revision.Dec 19 2016, 2:50 PM
apol edited edge metadata.
kossebau updated this object.Dec 19 2016, 7:00 PM
kossebau edited edge metadata.
Closed by commit R240:5f17bf2c0c65: Ignore host libs/includes/cmakeconfig files in Android toolchain (authored by kossebau). · Explain WhyDec 19 2016, 7:01 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mtoolchain/Android.cmake (50 lines)

Diff 9180

View Options

toolchain/Android.cmake

Loading...