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
