Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use only Git commands in git_info #179

Merged
merged 4 commits into from
Nov 18, 2016
Merged

Use only Git commands in git_info #179

merged 4 commits into from
Nov 18, 2016

Conversation

robertodr
Copy link
Contributor

@robertodr robertodr commented Nov 16, 2016

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.

endif()

configure_file(
${PROJECT_SOURCE_DIR}/cmake/downloaded/git_info.h.in
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bast This path is hardcoded. I was not able to find a permanent fix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @robertodr ,

is there some requirements as to the git version ? Change is based on git flags, would be good to know what is the lowest git version...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miroi The oldest version of Git I have access to is 1.7.1 and these commands work fine.
Might be that for Git <= 1.6 git log should be used instead of git show:
For the commit hash:

git --no-pager log -1 --pretty=format:"%h" -n 1

For the author name:

git --no-pager log -1 --pretty=format:"%aN" -n 1

For the commit date:

git --no-pager log -1 --pretty=format:"%ad" -n 1

@bast
Copy link
Member

bast commented Nov 16, 2016

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
Copy link
Contributor Author

@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
Copy link
Member

bast commented Nov 17, 2016

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

@bast
Copy link
Member

bast commented Nov 17, 2016

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
Copy link
Contributor Author

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

@bast
Copy link
Member

bast commented Nov 18, 2016

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

@bast bast merged commit caf8d63 into dev-cafe:master Nov 18, 2016
@bast
Copy link
Member

bast commented Nov 18, 2016

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

@robertodr
Copy link
Contributor Author

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

@bast
Copy link
Member

bast commented Nov 21, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants