qmake support is going away

[Dirk Hohndel]

The issue with base64 encoded patches is that they are such a pain to
respond to :-(

On Fri, Apr 17, 2015 at 10:07:04PM +0300, Lubomir I. Ivanov wrote:
> >
> > So with the latest master, what still breaks for you?
> >
> 
> two patches attached:
> 1) this adds some options - NO_TESTS, NO_DOCS etc.
> please let me know if these don't make any sense.
> 
> the PKGCONFIG_* options allow me to use my library path where libgit
> and libdc are installed even if forked from the subsurface git.

Why do you need that? Is it not possible to just install those in
install-root next to the Subsurface sources? That's why Tomaz and I added
that feature, so that regardless how things are setup you simply install
things at a consistent spot and all works out of the box...

> looks like the way to test if an options is ON is via:
> IF(SOME_OPTION)
> ...
> 
> i'm able to build with the following cmd line:
> cmake -G "MinGW Makefiles" -DCMAKE_BUILD_TYPE=Debug
> -DPKGCONFIG_LIBGIT2=1 -DPKGCONFIG_LIBDC=1 -DNO_DOCS=1 -DNO_MARBLE=1
> -DNO_TESTS=1 ..
> 
> 2) whitespace cleanup
> - real tabs
> - indentation

Yeah, I keep thinking that I want to clean up the whitespace disaster and
capitalization disaster that we have. I don't know what Tomaz is smoking
but the file looks hideous...

> the case insensitive format of cmake can be a bit confusing. i see
> different projects use different style, but i suggest that we convert
> all built-in macros to lower case e.g.:
> 
> OPTION() -> option()
> IF() -> if()
> ELSEIF() -> elseif()
> SET() -> set()
> etc...

YES!
Macros, variables and the keyword options in upper case, commands lower
case.

OK, here's my take on your first patch:



> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 93b0bdf..b15b1a5 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -7,7 +7,14 @@ cmake_minimum_required(VERSION 2.8.11)
>  
>  SET(CMAKE_AUTOMOC ON)
>  SET(CMAKE_AUTOUIC ON)
> -OPTION(PREFER_GIT_FROMSOURCE "Turn off if you wanna use system's libgit 0.21.5" ON)
> +
> +#options
> +
> +OPTION(PKGCONFIG_LIBGIT2 "use pkg-config to retrieve libgit2" OFF)
> +OPTION(PKGCONFIG_LIBDC "use pkg-config to retrieve libdivecomputer" OFF)
> +OPTION(NO_MARBLE "disable the marble widget" OFF)
> +OPTION(NO_TESTS "disable the tests" OFF)
> +OPTION(NO_DOCS "disable the docs" OFF)
>  
>  SET(CMAKE_MODULE_PATH ${${PROJECT_NAME}_SOURCE_DIR}/cmake/Modules)
>  INCLUDE_DIRECTORIES( . ${CMAKE_CURRENT_BINARY_DIR} ${CMAKE_BINARY_DIR} qt-ui qt-ui/profile)
> @@ -30,18 +37,31 @@ pkg_config_library(LIBUSB libusb-1.0 QUIET)
>  
>  # more libraries with special handling in case we build them ourselves
>  
> -if(NOT ${PREFER_GIT_FROMSOURCE})
> +IF(PKGCONFIG_LIBGIT2)

So you are inverting "FROMSOURCE" and are turning it into FROMPKGCONFIG.

Why is that better?

>  	pkg_config_library(LIBGIT2 libgit2 REQUIRED)
> +	SET(LIBGIT2_LIBRARIES "")

Hmm - don't you clear out the variable that you just set?

>  ELSE()
>  	FIND_PACKAGE(LIBGIT2 REQUIRED)
>  	INCLUDE_DIRECTORIES(${LIBGIT2_INCLUDE_DIR})
>  ENDIF()
>  
> -FIND_PACKAGE(Libdivecomputer REQUIRED)
> -INCLUDE_DIRECTORIES(${LIBDIVECOMPUTER_INCLUDE_DIR})
> +IF(PKGCONFIG_LIBDC)
> +	pkg_config_library(LIBDC libdivecomputer REQUIRED)
> +	SET(LIBDIVECOMPUTER_LIBRARIES "")

Again clearing out the libs. Weird

> +ELSE()
> +	FIND_PACKAGE(Libdivecomputer REQUIRED)
> +	INCLUDE_DIRECTORIES(${LIBDIVECOMPUTER_INCLUDE_DIR})
> +ENDIF()
> +
> +# optional marble
>  
> -FIND_PACKAGE(Marble REQUIRED)
> -include_directories(${MARBLE_INCLUDE_DIR})
> +IF(NO_MARBLE)
> +	ADD_DEFINITIONS(-DNO_MARBLE)
> +	SET(MARBLE_LIBRARIES "")
> +ELSE()
> +	FIND_PACKAGE(Marble REQUIRED)
> +	include_directories(${MARBLE_INCLUDE_DIR})
> +ENDIF()

Why doesn't this add a check that if Marble wasn't found NO_MARBLE is set?

>  SET(SUBSURFACE_LINK_LIBRARIES   ${SUBSURFACE_LINK_LIBRARIES} ${LIBDIVECOMPUTER_LIBRARIES}  ${LIBGIT2_LIBRARIES} -lusb-1.0)

But those xxx_LIBRARIES variables were set to "" above. So how does this
work?

> @@ -51,7 +71,9 @@ STRING(COMPARE EQUAL "${${PROJECT_NAME}_SOURCE_DIR}" "${${PROJECT_NAME}_BINARY_D
>  GET_FILENAME_COMPONENT(PARENTDIR ${${PROJECT_NAME}_SOURCE_DIR} PATH)
>  STRING(COMPARE EQUAL "${${PROJECT_NAME}_SOURCE_DIR}" "${PARENTDIR}" insourcesubdir)
>  IF(NOT (insource OR insourcedir))
> -	add_custom_target(link_marble_data ALL COMMAND rm -f marbledata && ln -s ${${PROJECT_NAME}_SOURCE_DIR}/marbledata ${${PROJECT_NAME}_BINARY_DIR}/marbledata)
> +	IF(NOT NO_MARBLE)
> +		add_custom_target(link_marble_data ALL COMMAND rm -f marbledata && ln -s ${${PROJECT_NAME}_SOURCE_DIR}/marbledata ${${PROJECT_NAME}_BINARY_DIR}/marbledata)
> +	ENDIF()
>  ENDIF()
>  
>  #configure Qt.

That's a good one

> @@ -339,12 +361,16 @@ ENDMACRO()
>  ENABLE_TESTING()
>  ADD_DEFINITIONS(-DSUBSURFACE_SOURCE="${CMAKE_SOURCE_DIR}")
>  ADD_DEFINITIONS(-g)
> -test(TestUnitConversion testunitconversion.cpp)
> -test(TestProfile testprofile.cpp)
> -test(TestGpsCoords testgpscoords.cpp)
> -test(TestParse testparse.cpp)
> +IF(NOT NO_TESTS)
> +	test(TestUnitConversion testunitconversion.cpp)
> +	test(TestProfile testprofile.cpp)
> +	test(TestGpsCoords testgpscoords.cpp)
> +	test(TestParse testparse.cpp)
> +ENDIF()
>  
> -ADD_CUSTOM_TARGET(documentation ALL mkdir -p ${CMAKE_BINARY_DIR}/Documentation/ \\; make -C ${CMAKE_SOURCE_DIR}/Documentation OUT=${CMAKE_BINARY_DIR}/Documentation/ doc)
> +IF(NOT NO_DOCS)
> +	ADD_CUSTOM_TARGET(documentation ALL mkdir -p ${CMAKE_BINARY_DIR}/Documentation/ \\; make -C ${CMAKE_SOURCE_DIR}/Documentation OUT=${CMAKE_BINARY_DIR}/Documentation/ doc)
> +ENDIF()
>  
>  # install Subsurface
>  # first some variables with files that need installing

So could I get one commit that adds "NO_TESTS" and "NO_DOCS"
One commit that fixes the NO_MARBLE case
And then one commit that makes sense and that deals with the pkgconfig
stuff. I'm OK with the inverted naming logic (just have them be
<libname>_FROM_PKGCONFIG) as long as it's used consistently.

And I really want to know how your patch works if you are clearing out the
variables for the LIBRARIES. I must be missing something there...

/D