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

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
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.
----------------
smeenai wrote:
> 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.
That's just a leftover from the original version, I replaced them with `functions`.


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


================
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
----------------
smeenai wrote:
> 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.
That's a great point, done.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57063