Re: [PATCH v4 1/8] Introduce CMake support for configuring Git

["Øystein Walle" <oystwa@gmail.com>]

Hi,

I haven't been able to pay much attention lately. I see this is v4 of this
patch series and thought I took a quick look and didn't find anything, it's
possible some of this has been addressed already. If so I apologize.

> So let's start building CMake support for Git.

Yay!

> +
> +Instructions to run CMake:
> +
> +cmake `relative-path-to-CMakeLists.txt` -DCMAKE_BUILD_TYPE=Release
> +Eg.
> +From the root of git source tree
> +	`cmake contrib/buildsystems/ `
> +This will build the git binaries at the root
> +
> +For out of source builds, say build in 'git/git-build/'
> +	`mkdir git-build;cd git-build; cmake ../contrib/buildsystems/`
> +This will build the git binaries in git-build directory
> +

Since the mininum required version is sufficiently high I suggest you
recommend the following as well:

    cmake -B build-dir -S contrib/buildsystems

This might be easier for scripted tasks (packaging and whatnot) since cd
and mkdir aren't necessary.

> +#set the source directory to root of git
> +set(CMAKE_SOURCE_DIR ${CMAKE_CURRENT_LIST_DIR}/../..)

See if you can avoid this. This this will break if another project includes Git
as part of itself with add_subdirectory() or the ExternalProject module. If all
you use it for is paths to other files then you might as well do this:

    set(repo_root "${CMAKE_CURRENT_LIST_DIR}/../..")

and use ${repo_root} the places you use ${CMAKE_SOURCE_DIR} now.

AFAIK the places CMake accepts relative paths it's usually relative to
CMAKE_CURRENT_SOURCE_DIR and not CMAKE_SOURCE_DIR, anyway. I don't think it's
automagically updated when CMAKE_SOURCE_DIR is changed.

> +include_directories(SYSTEM ${ZLIB_INCLUDE_DIRS})
> +if(CURL_FOUND)
> +	include_directories(SYSTEM ${CURL_INCLUDE_DIRS})
> +endif()

This is better handled like this these days:

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 8367b73e94..ca1f90e58c 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -121,10 +121,6 @@ if(NOT Intl_FOUND)
 	endif()
 endif()
 
-include_directories(SYSTEM ${ZLIB_INCLUDE_DIRS})
-if(CURL_FOUND)
-	include_directories(SYSTEM ${CURL_INCLUDE_DIRS})
-endif()
 if(EXPAT_FOUND)
 	include_directories(SYSTEM ${EXPAT_INCLUDE_DIRS})
 endif()
@@ -606,7 +602,7 @@ endif()
 #link all required libraries to common-main
 add_library(common-main OBJECT ${CMAKE_SOURCE_DIR}/common-main.c)
 
-target_link_libraries(common-main libgit xdiff ${ZLIB_LIBRARIES})
+target_link_libraries(common-main libgit xdiff ZLIB::ZLIB)
 if(Intl_FOUND)
 	target_link_libraries(common-main ${Intl_LIBRARIES})
 endif()
@@ -659,17 +655,17 @@ if(CURL_FOUND)
 	add_library(http_obj OBJECT ${CMAKE_SOURCE_DIR}/http.c)
 
 	add_executable(git-imap-send ${CMAKE_SOURCE_DIR}/imap-send.c)
-	target_link_libraries(git-imap-send http_obj common-main ${CURL_LIBRARIES})
+	target_link_libraries(git-imap-send http_obj common-main CURL::libcurl)
 
 	add_executable(git-http-fetch ${CMAKE_SOURCE_DIR}/http-walker.c ${CMAKE_SOURCE_DIR}/http-fetch.c)
-	target_link_libraries(git-http-fetch http_obj common-main ${CURL_LIBRARIES})
+	target_link_libraries(git-http-fetch http_obj common-main CURL::libcurl)
 
 	add_executable(git-remote-http ${CMAKE_SOURCE_DIR}/http-walker.c ${CMAKE_SOURCE_DIR}/remote-curl.c)
-	target_link_libraries(git-remote-http http_obj common-main ${CURL_LIBRARIES} )
+	target_link_libraries(git-remote-http http_obj common-main CURL::libcurl)
 
 	if(EXPAT_FOUND)
 		add_executable(git-http-push ${CMAKE_SOURCE_DIR}/http-push.c)
-		target_link_libraries(git-http-push http_obj common-main ${CURL_LIBRARIES} ${EXPAT_LIBRARIES})
+		target_link_libraries(git-http-push http_obj common-main CURL::libcurl ${EXPAT_LIBRARIES})
 	endif()
 endif()
 
With this change we're feeding proper CMake targets to target_link_libraries()
instead of just a bunch of strings. CMake can know a lot about a target, such
as it's dependencies, required macros and so on[1]. This means some boilerplate
code can be removed: Notice that the manual handling of Zlib's include path is
gone. The same can perhaps be done for the other libraries as well but I
haven't checked that.

> +include_directories(${CMAKE_SOURCE_DIR})
> +(...)
> +include_directories(${CMAKE_BINARY_DIR})

See if CMAKE_INCLUDE_CURRENT_DIR[2] makes this unneeded. It might not since you
overwrite CMAKE_SOURCE_DIR manually.

Øsse

[1]: https://cmake.org/cmake/help/latest/manual/cmake-properties.7.html#properties-on-targets
[2]: https://cmake.org/cmake/help/latest/variable/CMAKE_INCLUDE_CURRENT_DIR.html