[PATCH] D57063: [CMake] Unify scripts for generating VCS headers

[Michał Górny via Phabricator via llvm-commits]

mgorny added inline comments.


================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:1
+# CMake project that writes version control information to a header.
+#
----------------
A 'project'? Unless I'm misunderstanding something, it's rather a 'script'.


================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:3
+#
+# Input variables:
+#   SRC               - Source directory
----------------
This list seems outdated.


================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:22
+function(append_info name path)
+  #add_version_info_from_vcs(REVISION ${path})
+  get_source_info("${path}" revision repository)
----------------
Why is this commented out?


================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:28
+    "#define ${name}_REVISION \"${revision}\"\n")
+  file(APPEND "${HEADER_FILE}.txt"
+    "#define ${name}_REPOSITORY \"${repository}\"\n")
----------------
Hmm, can't you pass both strings to a single `file(APPEND ...`?


================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:33
+if (DEFINED SOURCE_DIRS AND DEFINED NAMES)
+  list(LENGTH SOURCE_DIRS source_dirs_length)
+  list(LENGTH NAMES names_length)
----------------
To be honest, seeing all this extra logic to handle two lists, I kinda feel like a simpler solution would be to pass colon-separated sublist, e.g. `LLVM:directory;CLANG:directory`. But that's just a loose idea.


================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:53
+execute_process(COMMAND ${CMAKE_COMMAND} -E copy_if_different
+  "${HEADER_FILE}.txt" "${HEADER_FILE}")
+file(REMOVE "${HEADER_FILE}.txt")
----------------
Any reason not to name it `.tmp`? `.txt` looks like a weird addition here.


================
Comment at: llvm/cmake/modules/VersionFromVCS.cmake:13
+  # FindSubversion does not work with symlinks. See PR 8437
+  if (NOT IS_SYMLINK "${path}")
+    find_package(Subversion)
----------------
Wouldn't it be possible to use `get_filename_component(... REALPATH)` to workaround the problem? Or maybe even a cheaper trick like `${path}/.`? (I know it was there before, just mentioning since I've noticed it)


================
Comment at: llvm/cmake/modules/VersionFromVCS.cmake:58
+      if (EXISTS "${git_dir}/svn/refs")
+        execute_process(COMMAND ${GIT_EXECUTABLE} svn info
+          WORKING_DIRECTORY ${path}
----------------
Can't you reuse `get_source_info_git_svn` here?


================
Comment at: llvm/cmake/modules/VersionFromVCS.cmake:72
+      else()
+        execute_process(COMMAND ${GIT_EXECUTABLE} log -1 --pretty=format:%H
+          WORKING_DIRECTORY ${path}
----------------
Any reason not to use `rev-parse HEAD`?


================
Comment at: llvm/cmake/modules/VersionFromVCS.cmake:74
+          WORKING_DIRECTORY ${path}
           TIMEOUT 5
           RESULT_VARIABLE git_result
----------------
I don't think TIMEOUT is a good idea here. Given that git works fully locally (and shouldn't really hang), you might terminate it unnecessarily when the system is under load.


================
Comment at: llvm/cmake/modules/VersionFromVCS.cmake:80
+        endif()
+        execute_process(COMMAND ${GIT_EXECUTABLE} remote -v
+          WORKING_DIRECTORY ${path}
----------------
I don't think that's really doing what you expect it to do. Unless I'm mistaken, it's going to get random (first? last?) URL, not the one relevant to current branch. Also, if you ever need to regex-match output of git, you're doing it wrong — git has a lot of nice script-oriented commands.

What you should do is figure out the upstream of the current branch (if any). https://stackoverflow.com/a/9753364/165333 might be of help. Alternatively, I suppose hardcoding `origin` may be good enough.

Then, `git remote get-url ${UPSTREAM}` is what you need.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57063/new/

https://reviews.llvm.org/D57063