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

[Sibi Siddharthan <sibisiddharthan.github@gmail.com>]

On Mon, Jun 15, 2020 at 7:31 PM Øystein Walle <oystwa@gmail.com> wrote:
>
> 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.
>

We could do that.

> > +#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.
>

Since we are using a single list file, it is fine.
I understand your concern that git is included as part of another
project things might break, but that is really rare.
I will try to fix these issues after adding support for BSD and APPLE systems.
The main priority for this patch series is to help git-for-windows
developers for now.

> > +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.
>

It depends on the implementation of the CMake modules. We can do as
you suggested for Zlib and curl.
Iconv added exported targets recently.
Intl still does not have this syntax yet.
Using ${*_LIBRARIES} works all across the board.

Thank You
Sibi Siddharthan

> > +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