dev-cafe / autocmake

CMake plugin composer.
http://autocmake.org
BSD 3-Clause "New" or "Revised" License
41 stars 18 forks source link

Use only Git commands in `git_info` #179

Closed robertodr closed 7 years ago

robertodr commented 7 years ago

This PR removes the need for CMake regexes to find the commit author name. Moreover, the header can be generated in a custom directory and with a custom name.

@bast @miroi I have fixed the test and fine-tuned the implementation:

  1. I have substituted the add_custom_command in git_info.cmake with a CMake function generate_git_info_header (the name can be changed) to fine-tune how the header is generated. The user can now choose location and name. If no arguments are provided to the function, this falls back to the previous implementation, i.e. generate a git_info.h header in ${PROJECT_BINARY_DIR}
  2. I have remove the git_info_sub.cmake file, since it is no longer necessary.
bast commented 7 years ago

Thanks! Can you summarize in one or two sentences what has changed? A bit tricky to see from the diff. Why is the test failing? You write that a path is hardcoded - how would you ideally like the code to be if it was not hardcoded? In other words what did you have in mind?

robertodr commented 7 years ago

@bast Before my changes that line read ${CMAKE_CURRENT_LIST_DIR}/git_info.h.in Doing that within a function returns the location from where it was called, which can now be anywhere in the CMake code. Since the CMake files need not necessarily be under ${PROJECT_SOURCE_DIR}/cmake I think that line could be problematic.

bast commented 7 years ago

Do you have an idea why the OS X tests failed?

bast commented 7 years ago

The hardcoding can be solved like this:

diff --git a/modules/git_info/git_info.cmake b/modules/git_info/git_info.cmake
index f499ddc..26f00f2 100644
--- a/modules/git_info/git_info.cmake
+++ b/modules/git_info/git_info.cmake
@@ -11,6 +11,9 @@
 #   fetch:
 #     - "%(url_root)modules/git_info/git_info.h.in"

+
+set(_current_dir ${CMAKE_CURRENT_LIST_DIR})
+
 function(generate_git_info_header)
   # _header_location: where the Git info header file should be generated
   # _header_name: the Git info header name, complete with extension (.h, .hpp, .hxx or whatever)
@@ -61,7 +64,7 @@ function(generate_git_info_header)
   endif()

   configure_file(
-    ${PROJECT_SOURCE_DIR}/cmake/downloaded/git_info.h.in
+    ${_current_dir}/git_info.h.in
     ${_header_location}/${_header_name}
     @ONLY
     )

Also I wonder whether we should not drop the default parameters but always require explicit parameters from the outside to simplify code and make things more explicit.

robertodr commented 7 years ago

@bast I have integrated your modifications. The failure on Mac seems unrelated to this PR, maybe a failure in running update.py?

bast commented 7 years ago

Thanks! Looks good now. I will test a bit more on my side and integrate very soon.

bast commented 7 years ago

Merged - however I will remove the defaults and the function needs to be called explicitly always. I think this is better.

robertodr commented 7 years ago

I left the default arguments not to hurt existing code using this module. I fully agree with you, explicit is better than implicit.

bast commented 7 years ago

I have changed it to explicit. The change anyway requires codes to add at least one line.