Skip to content

Commit

Permalink
[build] CMake: Introduce DownloadProject module (#851)
Browse files Browse the repository at this point in the history
This module is like FetchContent in CMake 3.11, but for the moment it may be more inviting to hackers not to require that yet.

The principle is explained here: https://crascit.com/2015/07/25/cmake-gtest/

It allows us to work with CMake in a more natural environment for CMake-submodules, which are otherwise nigh-uncontrollable in between escaping variables in CMake syntax and CMake being extremely bad at taking command-line arguments. (A fact I utterly despise as a regular user just downloading a project to compile & try.)

Upping the minimum CMake version over 3.3 should also allow us to get rid of a few braindead Android-related workarounds for sdcv, where we had to completely force-feed the `.a` suffix.
  • Loading branch information
Frenzie committed Mar 13, 2019
1 parent 8c94566 commit 2540b18
Show file tree
Hide file tree
Showing 4 changed files with 214 additions and 38 deletions.
2 changes: 1 addition & 1 deletion Makefile.defs
Expand Up @@ -605,7 +605,7 @@ LIBFFI_BUILD_DIR=$(THIRDPARTY_DIR)/libffi/build/$(MACHINE)
LIBFFI_DIR=$(CURDIR)/$(LIBFFI_BUILD_DIR)/libffi-prefix/src/libffi-build

SDCV_BUILD_DIR=$(THIRDPARTY_DIR)/sdcv/build/$(MACHINE)
SDCV_DIR=$(CURDIR)/$(SDCV_BUILD_DIR)/sdcv-prefix/src/sdcv
SDCV_DIR=$(CURDIR)/$(SDCV_BUILD_DIR)/build/$(MACHINE)/sdcv-prefix/src/sdcv
GLIB_BUILD_DIR=$(THIRDPARTY_DIR)/glib/build/$(MACHINE)
GLIB_DIR=$(CURDIR)/$(GLIB_BUILD_DIR)/glib-prefix/src/glib-build
LIBICONV_BUILD_DIR=$(THIRDPARTY_DIR)/libiconv/build/$(MACHINE)
Expand Down
17 changes: 17 additions & 0 deletions thirdparty/cmake_modules/DownloadProject.CMakeLists.cmake.in
@@ -0,0 +1,17 @@
# Distributed under the OSI-approved MIT License. See accompanying
# file LICENSE or https://github.com/Crascit/DownloadProject for details.

cmake_minimum_required(VERSION 3.5.1)

project(${DL_ARGS_PROJ}-download NONE)

include(ExternalProject)
ExternalProject_Add(${DL_ARGS_PROJ}-download
${DL_ARGS_UNPARSED_ARGUMENTS}
SOURCE_DIR "${DL_ARGS_SOURCE_DIR}"
BINARY_DIR "${DL_ARGS_BINARY_DIR}"
CONFIGURE_COMMAND ""
BUILD_COMMAND ""
INSTALL_COMMAND ""
TEST_COMMAND ""
)
182 changes: 182 additions & 0 deletions thirdparty/cmake_modules/DownloadProject.cmake
@@ -0,0 +1,182 @@
# Distributed under the OSI-approved MIT License. See accompanying
# file LICENSE or https://github.com/Crascit/DownloadProject for details.
#
# MODULE: DownloadProject
#
# PROVIDES:
# download_project( PROJ projectName
# [PREFIX prefixDir]
# [DOWNLOAD_DIR downloadDir]
# [SOURCE_DIR srcDir]
# [BINARY_DIR binDir]
# [QUIET]
# ...
# )
#
# Provides the ability to download and unpack a tarball, zip file, git repository,
# etc. at configure time (i.e. when the cmake command is run). How the downloaded
# and unpacked contents are used is up to the caller, but the motivating case is
# to download source code which can then be included directly in the build with
# add_subdirectory() after the call to download_project(). Source and build
# directories are set up with this in mind.
#
# The PROJ argument is required. The projectName value will be used to construct
# the following variables upon exit (obviously replace projectName with its actual
# value):
#
# projectName_SOURCE_DIR
# projectName_BINARY_DIR
#
# The SOURCE_DIR and BINARY_DIR arguments are optional and would not typically
# need to be provided. They can be specified if you want the downloaded source
# and build directories to be located in a specific place. The contents of
# projectName_SOURCE_DIR and projectName_BINARY_DIR will be populated with the
# locations used whether you provide SOURCE_DIR/BINARY_DIR or not.
#
# The DOWNLOAD_DIR argument does not normally need to be set. It controls the
# location of the temporary CMake build used to perform the download.
#
# The PREFIX argument can be provided to change the base location of the default
# values of DOWNLOAD_DIR, SOURCE_DIR and BINARY_DIR. If all of those three arguments
# are provided, then PREFIX will have no effect. The default value for PREFIX is
# CMAKE_BINARY_DIR.
#
# The QUIET option can be given if you do not want to show the output associated
# with downloading the specified project.
#
# In addition to the above, any other options are passed through unmodified to
# ExternalProject_Add() to perform the actual download, patch and update steps.
# The following ExternalProject_Add() options are explicitly prohibited (they
# are reserved for use by the download_project() command):
#
# CONFIGURE_COMMAND
# BUILD_COMMAND
# INSTALL_COMMAND
# TEST_COMMAND
#
# Only those ExternalProject_Add() arguments which relate to downloading, patching
# and updating of the project sources are intended to be used. Also note that at
# least one set of download-related arguments are required.
#
# If using CMake 3.2 or later, the UPDATE_DISCONNECTED option can be used to
# prevent a check at the remote end for changes every time CMake is run
# after the first successful download. See the documentation of the ExternalProject
# module for more information. It is likely you will want to use this option if it
# is available to you. Note, however, that the ExternalProject implementation contains
# bugs which result in incorrect handling of the UPDATE_DISCONNECTED option when
# using the URL download method or when specifying a SOURCE_DIR with no download
# method. Fixes for these have been created, the last of which is scheduled for
# inclusion in CMake 3.8.0. Details can be found here:
#
# https://gitlab.kitware.com/cmake/cmake/commit/bdca68388bd57f8302d3c1d83d691034b7ffa70c
# https://gitlab.kitware.com/cmake/cmake/issues/16428
#
# If you experience build errors related to the update step, consider avoiding
# the use of UPDATE_DISCONNECTED.
#
# EXAMPLE USAGE:
#
# include(DownloadProject)
# download_project(PROJ googletest
# GIT_REPOSITORY https://github.com/google/googletest.git
# GIT_TAG master
# UPDATE_DISCONNECTED 1
# QUIET
# )
#
# add_subdirectory(${googletest_SOURCE_DIR} ${googletest_BINARY_DIR})
#
#========================================================================================


set(_DownloadProjectDir "${CMAKE_CURRENT_LIST_DIR}")

include(CMakeParseArguments)

function(download_project)

set(options QUIET)
set(oneValueArgs
PROJ
PREFIX
DOWNLOAD_DIR
SOURCE_DIR
BINARY_DIR
# Prevent the following from being passed through
CONFIGURE_COMMAND
BUILD_COMMAND
INSTALL_COMMAND
TEST_COMMAND
)
set(multiValueArgs "")

cmake_parse_arguments(DL_ARGS "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})

# Hide output if requested
if (DL_ARGS_QUIET)
set(OUTPUT_QUIET "OUTPUT_QUIET")
else()
unset(OUTPUT_QUIET)
message(STATUS "Downloading/updating ${DL_ARGS_PROJ}")
endif()

# Set up where we will put our temporary CMakeLists.txt file and also
# the base point below which the default source and binary dirs will be.
# The prefix must always be an absolute path.
if (NOT DL_ARGS_PREFIX)
set(DL_ARGS_PREFIX "${CMAKE_BINARY_DIR}")
else()
get_filename_component(DL_ARGS_PREFIX "${DL_ARGS_PREFIX}" ABSOLUTE
BASE_DIR "${CMAKE_CURRENT_BINARY_DIR}")
endif()
if (NOT DL_ARGS_DOWNLOAD_DIR)
set(DL_ARGS_DOWNLOAD_DIR "${DL_ARGS_PREFIX}/${DL_ARGS_PROJ}-download")
endif()

# Ensure the caller can know where to find the source and build directories
if (NOT DL_ARGS_SOURCE_DIR)
set(DL_ARGS_SOURCE_DIR "${DL_ARGS_PREFIX}/${DL_ARGS_PROJ}-src")
endif()
if (NOT DL_ARGS_BINARY_DIR)
set(DL_ARGS_BINARY_DIR "${DL_ARGS_PREFIX}/${DL_ARGS_PROJ}-build")
endif()
set(${DL_ARGS_PROJ}_SOURCE_DIR "${DL_ARGS_SOURCE_DIR}" PARENT_SCOPE)
set(${DL_ARGS_PROJ}_BINARY_DIR "${DL_ARGS_BINARY_DIR}" PARENT_SCOPE)

# The way that CLion manages multiple configurations, it causes a copy of
# the CMakeCache.txt to be copied across due to it not expecting there to
# be a project within a project. This causes the hard-coded paths in the
# cache to be copied and builds to fail. To mitigate this, we simply
# remove the cache if it exists before we configure the new project. It
# is safe to do so because it will be re-generated. Since this is only
# executed at the configure step, it should not cause additional builds or
# downloads.
file(REMOVE "${DL_ARGS_DOWNLOAD_DIR}/CMakeCache.txt")

# Create and build a separate CMake project to carry out the download.
# If we've already previously done these steps, they will not cause
# anything to be updated, so extra rebuilds of the project won't occur.
# Make sure to pass through CMAKE_MAKE_PROGRAM in case the main project
# has this set to something not findable on the PATH.
configure_file("${_DownloadProjectDir}/DownloadProject.CMakeLists.cmake.in"
"${DL_ARGS_DOWNLOAD_DIR}/CMakeLists.txt")
execute_process(COMMAND ${CMAKE_COMMAND} -G "${CMAKE_GENERATOR}"
-D "CMAKE_MAKE_PROGRAM:FILE=${CMAKE_MAKE_PROGRAM}"
.
RESULT_VARIABLE result
${OUTPUT_QUIET}
WORKING_DIRECTORY "${DL_ARGS_DOWNLOAD_DIR}"
)
if(result)
message(FATAL_ERROR "CMake step for ${DL_ARGS_PROJ} failed: ${result}")
endif()
execute_process(COMMAND ${CMAKE_COMMAND} --build .
RESULT_VARIABLE result
${OUTPUT_QUIET}
WORKING_DIRECTORY "${DL_ARGS_DOWNLOAD_DIR}"
)
if(result)
message(FATAL_ERROR "Build step for ${DL_ARGS_PROJ} failed: ${result}")
endif()

endfunction()
51 changes: 14 additions & 37 deletions thirdparty/sdcv/CMakeLists.txt
Expand Up @@ -4,6 +4,7 @@ cmake_minimum_required(VERSION 3.5.1)
SET(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_CURRENT_LIST_DIR}/../cmake_modules")
include("koreader_thirdparty_common")
include("koreader_thirdparty_git")
include("DownloadProject")

enable_language(C CXX)

Expand All @@ -27,20 +28,14 @@ assert_var_defined(ZLIB_DIR)

ep_get_source_dir(SOURCE_DIR)

if(DEFINED ENV{ANDROID})
set(CRIPPLED_BY_ANDROID_FILES "${SOURCE_DIR}/src/libwrapper.cpp ${SOURCE_DIR}/src/sdcv.cpp ${SOURCE_DIR}/src/utils.cpp")
set(PATCH_CMD "${ISED} 's|_(|(|' ${CRIPPLED_BY_ANDROID_FILES}")
set(PATCH_CMD "${PATCH_CMD} && ${ISED} 's|#include <glib/gi18n.h>||' ${CRIPPLED_BY_ANDROID_FILES}")
set(PATCH_CMD sh -c "${PATCH_CMD}")
endif()

# because cmake needs all kinds of annoying special cmake variables
set(CMAKE_EXE_LINKER_FLAGS "${LDFLAGS} -static-libstdc++")
if(DEFINED ENV{DARWIN})
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -framework CoreFoundation -framework Security")
endif()

if(DEFINED ENV{ANDROID})
set(CMAKE_SYSTEM_NAME Android)
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -static-libgcc -static")
endif()

Expand All @@ -59,8 +54,6 @@ set(CMAKE_PREFIX_PATH "${GLIB2_INCLUDE_DIRS}$<SEMICOLON>${ZLIB_DIR}$<SEMICOLON>$
# Which is funny, because the .a is in the *same* directory. Just saying.
# Instead we add semi-hardcoded references to the right libraries in GLIB2_LIBRARIES
if(DEFINED ENV{ANDROID} OR DEFINED ENV{DARWIN})
set(CMAKE_FIND_LIBRARY_SUFFIXES ".a")

# glib2 also needs to link with libiconv and gettext
# this is a fairly clean hack
# CMAKE_CXX_FLAGS with -I and -L doesn't seem to have much of an effect
Expand All @@ -72,26 +65,13 @@ set(ZLIB_LIBRARIES "${ZLIB}")
# I just want to be able to -I and -L and have things work. CMake, CMake...
set(ZLIB_LIBRARY_RELEASE "${ZLIB}")

### Includes and libraries
# By overspecifying the heck out of everything we hope to force CMake into the
# equivalent of a couple of simple -I and -L flags because the proper method
# with CMAKE_PREFIX_PATH and CMAKE_FIND_LIBRARY_SUFFIXES does. not. work
set(CFG_OPTS "-DGLIB2_INCLUDE_DIRS='${GLIB2_INCLUDE_DIRS}' -DGLIB2_LIBRARIES='${GLIB2_LIBRARIES}' -DZLIB_INCLUDE_DIR='${ZLIB_INCLUDE_DIR}' -DZLIB_LIBRARIES='${ZLIB_LIBRARIES}' -DZLIB_LIBRARY_RELEASE='${ZLIB_LIBRARY_RELEASE}'")
# These are the directories where we tell CMake to search for libs and includes
set(CFG_OPTS "${CFG_OPTS} -DCMAKE_PREFIX_PATH='${CMAKE_PREFIX_PATH}'")
if($ENV{ANDROID})
# the default `;` causes escape issues
# we could escape it but on Android we only want .a and otherwise the default
set(CFG_OPTS "${CFG_OPTS} -DCMAKE_FIND_LIBRARY_SUFFIXES='${CMAKE_FIND_LIBRARY_SUFFIXES}'")
set(CFG_OPTS "${CFG_OPTS} -DCMAKE_SYSTEM_NAME=Android -DCMAKE_SYSTEM_VERSION=1")
endif()
### Compiler and linker flags
set(CFG_OPTS "${CFG_OPTS} -DCMAKE_C_COMPILER='${CMAKE_C_COMPILER}' -DCMAKE_C_COMPILER_LAUNCHER='${CMAKE_C_COMPILER_LAUNCHER}' -DCMAKE_C_COMPILER_ARG1='${CMAKE_C_COMPILER_ARG1}' -DCMAKE_CXX_COMPILER='${CMAKE_CXX_COMPILER}' -DCMAKE_CXX_COMPILER_LAUNCHER='${CMAKE_CXX_COMPILER_LAUNCHER}' -DCMAKE_CXX_COMPILER_ARG1='${CMAKE_CXX_COMPILER_ARG1}' -DCMAKE_CXX_FLAGS='${CXXFLAGS}' -DCMAKE_EXE_LINKER_FLAGS='${CMAKE_EXE_LINKER_FLAGS}'")
### Disable some sdcv stuff we don't need
set(CFG_OPTS "${CFG_OPTS} -DENABLE_NLS:BOOL=False -DWITH_READLINE:BOOL=False")
### Disable the silly build tree RPATH
set(CFG_OPTS "${CFG_OPTS} -DCMAKE_SKIP_BUILD_RPATH:BOOL=True")
set(CFG_CMD sh -c "${CMAKE_COMMAND} ${CFG_OPTS}")
set(CMAKE_SKIP_BUILD_RPATH True)

# These need to be option(), not set(), for deep & profound CMake reasons
option(ENABLE_NLS False)
option(WITH_READLINE False)

# Force utf8 command line parsing, and accept not-found -u dictnames
set(PATCH_CMD2 sh -c "patch -N -p1 < ${CMAKE_CURRENT_SOURCE_DIR}/sdcv.patch || true")

Expand All @@ -102,14 +82,11 @@ ko_write_gitclone_script(
${SOURCE_DIR}
)

include(ExternalProject)
ExternalProject_Add(
${PROJECT_NAME}
include(DownloadProject)
download_project(
PROJ ${PROJECT_NAME}
DOWNLOAD_COMMAND ${CMAKE_COMMAND} -P ${GIT_CLONE_SCRIPT_FILENAME}
BUILD_IN_SOURCE 1
PATCH_COMMAND COMMAND ${PATCH_CMD} COMMAND ${PATCH_CMD2}
CONFIGURE_COMMAND ${CFG_CMD}
BUILD_COMMAND $(MAKE) -j${PARALLEL_JOBS}
# skip install
INSTALL_COMMAND ""
PATCH_COMMAND COMMAND ${PATH_CMD2}

This comment has been minimized.

Copy link
@Frenzie

Frenzie Mar 14, 2019

Author Member

@poire-z In what scenario did this Andoid patch make a difference @poire-z ? I ask because I accidentally typod but I didn't notice any issues. :-)

This comment has been minimized.

Copy link
@poire-z

poire-z Mar 14, 2019

Contributor

I don't really remember :|
Was from #557. Whole story in koreader/koreader#3378 I think (haven't re-read it).

And possibly for some previous version: #502 koreader/koreader#3144

This comment has been minimized.

Copy link
@Frenzie

Frenzie Mar 14, 2019

Author Member

Alright, so if a word like académie works, the patch isn't necessary anymore and if it doesn't it is.

This comment has been minimized.

Copy link
@poire-z

poire-z Mar 14, 2019

Contributor

And if you have the Académie Française dict: find a word that have a result in it and in some other dicts - then disable that Académie Française one, and do a lookup for that same word: you should no more see that result from Académie Française but you should see the results from other dicts.

This comment has been minimized.

Copy link
@poire-z

poire-z Mar 14, 2019

Contributor

koreader/koreader#3378 (comment)
(Glad i was verbose in my comments about my investigations :)

This comment has been minimized.

Copy link
@poire-z

This comment has been minimized.

Copy link
@Frenzie

Frenzie Mar 15, 2019

Author Member

@poire-z That all seems to work for me.

This comment has been minimized.

Copy link
@poire-z

poire-z Mar 15, 2019

Contributor

No more for me with today's nightly on my android 6 phone:

  • have my 4 dictionaries checked: results from all 4
  • just uncheck one (whether Académie or Robert without accent) & keep the 3 others checked: zero result

This comment has been minimized.

Copy link
@Frenzie

Frenzie Mar 15, 2019

Author Member

Could it be about the filename too? The one I've got is called acadfran.etc. (The one I added to our own repo here.)

Anyway, I'll fix the typo and that'll apply the patch properly. I'm just confused about what I'm doing wrong, also on Android 6.

I took two dictionaries, gcide & académie. Then I looked up some words with and without accents. I disabled académie, repeated. Only results from gcide. Re-enabled, repeated. Results from both. No issues, all as expected.

This comment has been minimized.

Copy link
@poire-z

poire-z Mar 15, 2019

Contributor

Yes, it's about the dictionary name, specified on the sdcv command line, something like ./sdcv -u "toto" -u "Académie" and the encoding of the args. So, if you end up with -u "acadfran.etc", you may not notice it.
edit: although even if you renamed the dict directory to acadfran.etc, koreader should still use the dict name from the .ifo in that command line.

This comment has been minimized.

Copy link
@poire-z

poire-z Mar 15, 2019

Contributor

Also, you should disable gcide for testing, so then only Académie will be specified with -u:
https://github.com/koreader/koreader/blob/645d41eda91154715c5fbaca8644c93321e2779e/frontend/apps/reader/modules/readerdictionary.lua#L155-L159

This comment has been minimized.

Copy link
@Frenzie

Frenzie Mar 15, 2019

Author Member

Ah, I understood the instructions backward. I can indeed reproduce it I disable gcide.

I didn't rename anything though. That's just what they're named. :-)

)

add_subdirectory(${SOURCE_DIR})

0 comments on commit 2540b18

Please sign in to comment.