[PATCH] Fix kirigami build

[Anton Lundin]

On 17 August, 2016 - Dirk Hohndel wrote:

> On Wed, Aug 17, 2016 at 06:50:35PM +0200, Anton Lundin wrote:
> > This fixes the subsurface-mobile build on my Linux desktop and on
> > android.
> > 
> > To get it to run on Linux desktop, set QT_QUICK_CONTROLS_STYLE to
> > something that kirigami doesn't know about, like kakor. Otherwise it
> > will detect Desktop and die on:
> > qrc:/styles/Desktop/Icon.qml:23 module "org.kde.kquickcontrolsaddons" is not installed
> > 
> > This works with kirigami 32c980c46, when 47fb7821 is reverted.
> > 
> > Signed-off-by: Anton Lundin <glance at acc.umu.se>
> > ---
> > The cmake parts are sketchy at best, and should be fixed in a better way.
> > 
> > It would also be nice if kirigami static build did have a install target
> > that created a dir contaning the needed parts.
> > 
> >  CMakeLists.txt              |  3 ++-
> >  packaging/android/build.sh  | 13 +++++++++----
> >  scripts/build.sh            | 10 +++++-----
> >  scripts/mobilecomponents.sh |  7 ++++---
> >  4 files changed, 20 insertions(+), 13 deletions(-)
> > 
> > diff --git a/CMakeLists.txt b/CMakeLists.txt
> > index a824d03..6328dc0 100644
> > --- a/CMakeLists.txt
> > +++ b/CMakeLists.txt
> > @@ -240,7 +240,8 @@ if(${SUBSURFACE_TARGET_EXECUTABLE} MATCHES "MobileExecutable")
> >  	endif()
> >  	ADD_LIBRARY(kirigami_static_library STATIC IMPORTED)
> >  	SET_TARGET_PROPERTIES(kirigami_static_library PROPERTIES
> > -		IMPORTED_LOCATION ${CMAKE_SOURCE_DIR}/../kirigami-build/src/libkirigamiplugin.a)
> > +		IMPORTED_LOCATION ${KIRIGAMI_LIBRARY})
> 
> No, that breaks the build as KIRIGAMI LIBRARY is not defined in the
> CMakeLists.txt file. It's fine to have this work if the value is provided
> buy a build script, but there must be a default value so existing builds
> don't break with syntax error.
> 

This cmake part can be done better, as said above. I don't know how to
create a mandatory parameter which is conditional on it being the
MobileExecutable thats being built, so thats why i did it this way.

I agree that its bad to break peoples build dirs, but how is building a
mobile version by just rebuilding subsurface at the moment?
When built via the scripts, both scripts/build.sh and
packaging/android/build.sh, they will add the relevant parameter, so
that why i really don't see this as a big blocker.

The ios build script doesn't build kirigami at all as far as it looks to me.

Anyhow, this will _never_ work with the android builds (and probably ios
to) due to that kirigami library not being next to the subsurface dir.

Even worse, if one would do as i do and build multiple different arches
at on the same machine, the library might exist but being built for the
wrong arch.

So, cmake'ers: how do one create a conditional mandatory parameter?

> > diff --git a/packaging/android/build.sh b/packaging/android/build.sh
> > index d844e8d..4b353e3 100644
> > --- a/packaging/android/build.sh
> > +++ b/packaging/android/build.sh
> > @@ -316,11 +316,15 @@ if [ "$SUBSURFACE_MOBILE" = "ON" ] ; then
> >  	pushd $SUBSURFACE_SOURCE
> >  	bash ./scripts/mobilecomponents.sh
> >  	popd
> > -	rm -rf kirigami-build
> > -	mkdir -p kirigami-build
> > -	pushd kirigami-build
> > -	cmake $SUBSURFACE_SOURCE/mobile-widgets/qml/kirigami/ -DSTATIC_LIBRARY=ON -DCMAKE_PREFIX_PATH:UNINITIALIZED=${QT5_ANDROID}/android_${QT_ARCH}/lib/cmake
> > +	rm -rf kirigami-build-$ARCH
> > +	mkdir -p kirigami-build-$ARCH
> > +	pushd kirigami-build-$ARCH
> > +	cmake \
> > +		$SUBSURFACE_SOURCE/../kirigami/ \
> > +		-DSTATIC_LIBRARY=ON \
> > +		-DCMAKE_PREFIX_PATH:UNINITIALIZED=${QT5_ANDROID}/android_${QT_ARCH}/lib/cmake
> >  	make -j4
> > +	KIRIGAMI_LIBRARY="-DKIRIGAMI_LIBRARY=../kirigami-build-$ARCH/src/libkirigamiplugin.a"
> 
> I don't like this way of using a variable...
> 
> > @@ -361,6 +365,7 @@ cmake $MOBILE_CMAKE \
> >  	-DCMAKE_BUILD_TYPE=${BUILD_TYPE} \
> >  	-DMAKE_TESTS=OFF \
> >  	-DFTDISUPPORT=${FTDI} \
> > +	$KIRIGAMI_LIBRARY \
> 
> I'd rather have
> 	-DKIRIGAMI_LIBRARY=$KIRIGAMI_LIBRARY
> 

You can't do it that way, because then we would provide a path to
something not built for the desktop version. Thats why its populated via
a variable, thats only filled in when building the mobile version.

> > diff --git a/scripts/build.sh b/scripts/build.sh
> > index 02f1095..113ab6f 100755
> > --- a/scripts/build.sh
> > +++ b/scripts/build.sh
> >  		make -j4
> > +		KIRIGAMI_LIBRARY="-DKIRIGAMI_LIBRARY=$SRC/kirigami-build/src/libkirigamiplugin.a"
> >  	fi
> >  	cmake -DCMAKE_BUILD_TYPE=Debug .. \
> > +		$KIRIGAMI_LIBRARY \
> >  		-DSUBSURFACE_TARGET_EXECUTABLE=$SUBSURFACE_EXECUTABLE \
> 
> Ditto.
> 

Same answer as above.

So, id say this ain't perfect but its better than what we got in master,
which doesn't build at all.

//Anton

-- 
Anton Lundin	+46702-161604