Re: [PATCH v4 1/8] Introduce CMake support for configuring Git
- From
Øystein Walle <oystwa@gmail.com>
- Date
- Jun 15, 2020, 14:00 UTC
- Message-ID
- <20200615140049.1647-1-oystwa@gmail.com>
- In-Reply-To
- <c4e1ba74464cfae882cde646d3c79778244b8f5e.1591986566.git.gitgitgadget@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!
Show 13 quoted lines
> + > +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