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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

I'd prefer that @mgorny have another look, but I'm happy to stamp this if he hasn't gotten around to it in a day or two.



================
Comment at: llvm/CMakeLists.txt:756
+get_source_info(${CMAKE_CURRENT_SOURCE_DIR} revision repository)
+if(revision MATCHES "[0-9]+")
+  set(LLVM_RPM_SPEC_REVISION "r${revision}")
----------------
The intent here is distinguish between SVN revision numbers and git hashes, right? Isn't it theoretically possible to have a Git hash that's all numbers? A length check might be better, or the regex should at least have `^$` anchors.


================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:9
+# The output header will contain macros <NAME>_REPOSITORY and <NAME>_REVISION,
+# where "<NAME>" and is substituted with the names specified in the input
+# variables, for each of the SOURCE_DIRS given.
----------------
Remove the "and"


================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:10
+# where "<NAME>" and is substituted with the names specified in the input
+# variables, for each of the SOURCE_DIRS given.
+
----------------
SOURCE_DIRS references the older interface.


================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:16
+
+set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${LLVM_DIR}/cmake/modules")
+include(VersionFromVCS)
----------------
`list(APPEND)`?


================
Comment at: llvm/cmake/modules/VersionFromVCS.cmake:6
 
-function(add_version_info_from_vcs VERS)
-  SET(SOURCE_DIR ${ARGV1})
-  if("${SOURCE_DIR}" STREQUAL "")
-      SET(SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
-  endif()
-  string(REPLACE "svn" "" result "${${VERS}}")
-  if( EXISTS "${SOURCE_DIR}/.svn" )
-    set(result "${result}svn")
-    # FindSubversion does not work with symlinks. See PR 8437
-    if( NOT IS_SYMLINK "${SOURCE_DIR}" )
-      find_package(Subversion)
+macro(get_source_info_svn path revision repository)
+  # If svn is a bat file, find_program(Subversion) doesn't find it.
----------------
Was it intentional to make these macros? The variable pollution with macros is awful, and you're already using PARENT_SCOPE to set the variables that should be propagated upward.


================
Comment at: llvm/cmake/modules/VersionFromVCS.cmake:13
+
+  # Subversion module does not work with symlinks. See PR 8437
+  get_filename_component(realpath ${path} REALPATH)
----------------
Can you clarify that this is LLVM PR8437, and not a CMake or Subversion PR?


================
Comment at: llvm/cmake/modules/VersionFromVCS.cmake:29
+  if(GIT_FOUND)
+    # Run from a subdirectory to force git to print an absoute path.
+    execute_process(COMMAND ${GIT_EXECUTABLE} rev-parse --git-dir
----------------
This seems kinda fragile (what's to prevent git from printing a `../` relative path in the future...). Maybe it would be better to use `get_filename_component` with `ABSOLUTE` instead? I'm fine with that being a follow-up.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57063