Completelly disable git/svn version checking if not needed

[• rafael]

Summary

Working with git on a branch I find it really annoying that committing a change causes ninja to think that stuff needs to be rebuilt.

With this change at least nothing in llvm needs to be rebuild when something is committed.

I am not sure if this is the right variable to use. Should I add a new cmake option?

[pcc]

Using LLVM_APPEND_VC_REV seems fine to me, but we probably want to update the documentation and most likely change the default because right now the VC revision affects ThinLTO cache invalidation, and we shouldn't silently use stale cache entries when clang is upgraded.

@beanz any thoughts?

[• rafael]

Updated the default and the docs.

[pcc]

LGTM

[• rafael]

r306858

[vitalybuka]

CMakeLists.txt

209

New default slowed down many build bots which relied on incremental build.

[rnk]

In D34560#789101, @pcc wrote:

Using LLVM_APPEND_VC_REV seems fine to me, but we probably want to update the documentation and most likely change the default because right now the VC revision affects ThinLTO cache invalidation, and we shouldn't silently use stale cache entries when clang is upgraded.

@beanz any thoughts?

I think we can revert the change to the default value of LLVM_APPEND_VC_REV. The VCS revision doesn't need to be included in LLVM_VERSION_STRING because ThinLTO includes VCSRevision.h to add LLVM_REVISION into the hash.

[pcc]

In D34560#855667, @rnk wrote:

In D34560#789101, @pcc wrote:

Using LLVM_APPEND_VC_REV seems fine to me, but we probably want to update the documentation and most likely change the default because right now the VC revision affects ThinLTO cache invalidation, and we shouldn't silently use stale cache entries when clang is upgraded.

@beanz any thoughts?

I think we can revert the change to the default value of LLVM_APPEND_VC_REV. The VCS revision doesn't need to be included in LLVM_VERSION_STRING because ThinLTO includes VCSRevision.h to add LLVM_REVISION into the hash.

I thought that the purpose of this change was to make thinlto use LLVM_APPEND_VC_REV to control whether to add the VCS revision to the hash. Or do you think there should be a separate CMake variable to control whether thinlto uses the VCS revision?

[rnk]

In D34560#855735, @pcc wrote:

I thought that the purpose of this change was to make thinlto use LLVM_APPEND_VC_REV to control whether to add the VCS revision to the hash. Or do you think there should be a separate CMake variable to control whether thinlto uses the VCS revision?

I think I see what happened now.

LLVM_APPEND_VC_REV was a dormant option that probably nobody was using. The effect of that option was to add the revision to LLVM_VERSION_STRING in llvm-config.h, which causes llvm-config.h to change after every git commit. This is, IMO, completely undesirable.

Because that functionality was disabled by default, you and others (Jordan?) created VCSRevision.h and the things that came before it to compute accurate version information as part of the build. Rafael didn't want actions to run after changing git branches, so he wrote this patch to put the behavior under this flag. Because you requested the default to change, now everyone is getting the unintended updates to llvm-config.h, causing much larger rebuilds.

However, nobody noticed for months, because cmake caches option settings when it builds. In order to observe the new LLVM_APPEND_VC_REV default value, you have to delete CMakeCache.txt.

I think the default behavior should go back to what it was before this change, where LLVM_REVISION is incorporated into the ThinLTO hash, and we don't have a full rebuild after every cmake run. I'll send a patch.

[rnk]

Oh, right, I sent it already, it's D37272.

[pcc]

In D34560#855831, @rnk wrote:

In D34560#855735, @pcc wrote:

I thought that the purpose of this change was to make thinlto use LLVM_APPEND_VC_REV to control whether to add the VCS revision to the hash. Or do you think there should be a separate CMake variable to control whether thinlto uses the VCS revision?

I think I see what happened now.

LLVM_APPEND_VC_REV was a dormant option that probably nobody was using. The effect of that option was to add the revision to LLVM_VERSION_STRING in llvm-config.h, which causes llvm-config.h to change after every git commit. This is, IMO, completely undesirable.

Because that functionality was disabled by default, you and others (Jordan?) created VCSRevision.h and the things that came before it to compute accurate version information as part of the build. Rafael didn't want actions to run after changing git branches, so he wrote this patch to put the behavior under this flag. Because you requested the default to change, now everyone is getting the unintended updates to llvm-config.h, causing much larger rebuilds.

However, nobody noticed for months, because cmake caches option settings when it builds. In order to observe the new LLVM_APPEND_VC_REV default value, you have to delete CMakeCache.txt.

Ouch, sorry about that. Somehow I never noticed this, presumably because I didn't create a new build directory recently.

CMakeLists.txt

 option(LLVM_USE_FOLDERS "Enable solution folders in Visual Studio. Disable for Express versions." ON)
 if ( LLVM_USE_FOLDERS )
   set_property(GLOBAL PROPERTY USE_FOLDERS ON)
 endif()
 
 include(VersionFromVCS)
 
 option(LLVM_APPEND_VC_REV
-  "Append the version control system revision id to LLVM version" OFF)
+  "Embed the version control system revision id in LLVM" ON)
 
 if( LLVM_APPEND_VC_REV )
   add_version_info_from_vcs(PACKAGE_VERSION)
 endif()
 
 set(PACKAGE_NAME LLVM)
 set(PACKAGE_STRING "${PACKAGE_NAME} ${PACKAGE_VERSION}")
 set(PACKAGE_BUGREPORT "http://llvm.org/bugs/")

docs/CMake.rst

**LLVM_BUILD_TESTS**:BOOL
   with the target *UnitTests*.
 
 **LLVM_INCLUDE_TESTS**:BOOL
   Generate build targets for the LLVM unit tests. Defaults to ON. You can use
   this option to disable the generation of build targets for the LLVM unit
   tests.
 
 **LLVM_APPEND_VC_REV**:BOOL
-  Append version control revision info (svn revision number or Git revision id)
-  to LLVM version string (stored in the PACKAGE_VERSION macro). For this to work
-  cmake must be invoked before the build. Defaults to OFF.
+  Embed version control revision info (svn revision number or Git revision id).
+  This is used among other things in the LLVM version string (stored in the
+  PACKAGE_VERSION macro). For this to work cmake must be invoked before the
+  build. Defaults to ON.
 
 **LLVM_ENABLE_THREADS**:BOOL
   Build with threads support, if available. Defaults to ON.
 
 **LLVM_ENABLE_CXX1Y**:BOOL
   Build in C++1y mode, if available. Defaults to OFF.
 
 **LLVM_ENABLE_ASSERTIONS**:BOOL

include/llvm/Support/CMakeLists.txt

 # Figure out if we can track VC revisions.
 function(find_first_existing_file out_var)
   foreach(file ${ARGN})
     if(EXISTS "${file}")
       set(${out_var} "${file}" PARENT_SCOPE)
       return()
     endif()
   endforeach()
 endfunction()
 
 macro(find_first_existing_vc_file out_var path)
+  if ( LLVM_APPEND_VC_REV )
     find_program(git_executable NAMES git git.exe git.cmd)
     # Run from a subdirectory to force git to print an absolute path.
     execute_process(COMMAND ${git_executable} rev-parse --git-dir
       WORKING_DIRECTORY ${path}/cmake
       RESULT_VARIABLE git_result
       OUTPUT_VARIABLE git_dir
       ERROR_QUIET)
     if(git_result EQUAL 0)
       string(STRIP "${git_dir}" git_dir)
       set(${out_var} "${git_dir}/logs/HEAD")
       # some branchless cases (e.g. 'repo') may not yet have .git/logs/HEAD
       if (NOT EXISTS "${git_dir}/logs/HEAD")
         file(WRITE "${git_dir}/logs/HEAD" "")
       endif()
     else()
       find_first_existing_file(${out_var}
         "${path}/.svn/wc.db"   # SVN 1.7
         "${path}/.svn/entries" # SVN 1.6
       )
     endif()
+  endif()
 endmacro()
 
 find_first_existing_vc_file(llvm_vc "${LLVM_MAIN_SRC_DIR}")
 
 # The VC revision include that we want to generate.
 set(version_inc "${CMAKE_CURRENT_BINARY_DIR}/VCSRevision.h")
 
 set(get_svn_script "${LLVM_CMAKE_PATH}/GenerateVersionFromCVS.cmake")