[CMake] Unify scripts for generating VCS headers

[phosek]

Summary

Previously, there were two different scripts for generating VCS headers:
one used by LLVM and one used by Clang. They were both similar, but
different. They were both broken in their own ways, for example the one
used by Clang didn't properly handle monorepo resulting in an incorrect
version information reported by Clang, while the one used by LLVM didn't
support more than one repository which is needed in the case that Clang
is checked out separately.

This change unifies two the scripts by introducing a new script that's
used from both LLVM and Clang, ensures that the new script supports both
monorepo and standalone SVN and Git setups, and removes the old scripts.

[mgorny]

llvm/cmake/modules/GenerateVersionFromVCS.cmake

1 ↗

(On Diff #182957)

A 'project'? Unless I'm misunderstanding something, it's rather a 'script'.

3 ↗

(On Diff #182957)

This list seems outdated.

22 ↗

(On Diff #182957)

Why is this commented out?

28 ↗

(On Diff #182957)

Hmm, can't you pass both strings to a single file(APPEND ...?

33 ↗

(On Diff #182957)

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.

53 ↗

(On Diff #182957)

Any reason not to name it .tmp? .txt looks like a weird addition here.

llvm/cmake/modules/VersionFromVCS.cmake

13 ↗

(On Diff #182957)

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)

58 ↗

(On Diff #182957)

Can't you reuse get_source_info_git_svn here?

72 ↗

(On Diff #182957)

Any reason not to use rev-parse HEAD?

74 ↗

(On Diff #182957)

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.

80 ↗

(On Diff #182957)

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.

[smeenai]

llvm/CMakeLists.txt

756 ↗

(On Diff #182957)

Super nit: I think it's more common in LLVM's CMake to *not* have the space between the if and the opening parenthesis, i.e. you'd have if(...) instead of if (...). (I only noticed cos you were reformatting in a bunch of other places.)

llvm/cmake/modules/GenerateVersionFromVCS.cmake

33 ↗

(On Diff #182957)

You'd need to verify the colon separation in that case, so I think it'd end up being a wash...

Another possibility would be to still have NAMES for specifying all projects, but then have <NAME>_SOURCE_DIR variables to specify the source directories and have the script ensure all those variables are defined. In other words, if you passed -DNAMES=LLVM;CLANG, you'd also pass -DLLVM_SOURCE_DIR=... and -DCLANG_SOURCE_DIR=....

[phosek]

llvm/CMakeLists.txt

756 ↗

(On Diff #182957)

I saw both styles in LLVM but never figured out which one is the official. Doing a quick grep, I found 975 for if(...) and 525 for if (...) so it seems like the version without space is indeed more common.

llvm/cmake/modules/GenerateVersionFromVCS.cmake

22 ↗

(On Diff #182957)

Leftover, this version was replaced by get_source_info.

llvm/cmake/modules/VersionFromVCS.cmake

58 ↗

(On Diff #182957)

I considered it but that macro does some duplicate work like finding Git and finding the working directory which seems unnecessary.

[mgorny]

llvm/cmake/modules/VersionFromVCS.cmake

58 ↗

(On Diff #182957)

Isn't the result cached? And if it isn't, can you cache it? Avoiding duplicate code makes future maintenance easier, and I think this is exactly the purpose of this diff.

[phosek]

llvm/cmake/modules/VersionFromVCS.cmake

58 ↗

(On Diff #182957)

I'm not sure what you mean by duplicate code? I have removed get_source_info_git_svn (the one in the first patch was just a leftover) and instead merged its logic into this macro to avoid duplication. I can split this code into a macro again if you prefer?

[phosek]

Does anyone have any other opinions on this? I'd like to land as soon as possible this since the current implementation is broken with monorepo.

[smeenai]

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.

llvm/CMakeLists.txt

756 ↗

(On Diff #183040)

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.

llvm/cmake/modules/GenerateVersionFromVCS.cmake

9 ↗

(On Diff #183040)

Remove the "and"

10 ↗

(On Diff #183040)

SOURCE_DIRS references the older interface.

16 ↗

(On Diff #183040)

list(APPEND)?

llvm/cmake/modules/VersionFromVCS.cmake

6 ↗

(On Diff #183040)

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.

13 ↗

(On Diff #183040)

Can you clarify that this is LLVM PR8437, and not a CMake or Subversion PR?

29 ↗

(On Diff #183040)

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.

[phosek]

llvm/cmake/modules/VersionFromVCS.cmake

6 ↗

(On Diff #183040)

That's just a leftover from the original version, I replaced them with functions.

13 ↗

(On Diff #183040)

Confirmed, see https://bugs.llvm.org/show_bug.cgi?id=8437

29 ↗

(On Diff #183040)

That's a great point, done.

[smeenai]

LGTM with the comment addressed. Like I said, I'd prefer to wait for a few days to give @mgorny another change to take a look, but you're better placed to judge when this should be landed :)

llvm/cmake/modules/VersionFromVCS.cmake

36 ↗

(On Diff #183682)

This needs BASE_DIR "${path}", right?

[mgorny]

clang/lib/Basic/CMakeLists.txt

15 ↗

(On Diff #183682)

Please disregard if you think it's not worth the effort right now but… I'm wondering if you could also move this part of the logic to GenerateVersionfromVCS. I mean, the contents resemble those that you are generating inside that script, and you're basically repeating this whole conditional part in both call sites. I suppose you could alos kill the .undef file, and just call GenerateVersionfromVCS unconditionally, presuming it'll either generate revision data or undefs.

llvm/cmake/modules/VersionFromVCS.cmake

43 ↗

(On Diff #183682)

Hmm, I'm wondering if it's possible to make subversion_wc_info to work on top of git-svn.

58 ↗

(On Diff #182957)

Sorry, I haven't looked. It's fine now, yes.

[phosek]

I had to do some more refactoring, but I managed to factor out the .undef handling altogether as suggested by @mgorny. Let me know if this is still OK to land.

[phosek]

llvm/cmake/modules/VersionFromVCS.cmake

36 ↗

(On Diff #183682)

I don't know if it's really necessary (it worked fine without in my testing) but I'm fine including it.

43 ↗

(On Diff #183682)

I tried it briefly but couldn't make it work because git-svn metadata isn't a Subversion checkout.

[phosek]

Ping, is it okay to land this change?

[smeenai]

Still LGTM.

[mgorny]

Thanks a lot. That's an amazing piece of work, and I can't find anything else to nitpick ;-).

[phosek]

This broke Sanitizer Windows bots (http://lab.llvm.org:8011/builders/sanitizer-windows/builds/41295) so I had to revert the change and need to find out why it's failing on Windows.

[phosek]

It was an escaping issues when passing a list to the GenerateVersionFromVCS.cmake, this should now be addressed.

[mgorny]

@phosek, I'm sorry to say but this is apparently breaking stand-alone clang builds:

FAILED: lib/Basic/VCSVersion.inc
cd /var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999-abi_x86_64.amd64/lib/Basic && /usr/bin/cmake -DNAMES="LLVM;CLANG" -DLLVM_SOURCE_DIR= -DCLANG_SOURCE_DIR=/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999 -DHEADER_FILE=/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999-abi_x86_64.amd64/lib/Basic/VCSVersion.inc -P /usr/lib64/llvm/9/lib64/cmake/llvm/GenerateVersionFromVCS.cmake
CMake Error at /usr/lib64/llvm/9/lib64/cmake/llvm/GenerateVersionFromVCS.cmake:18 (include):
  include could not find load file:

    VersionFromVCS

[mgorny]

(apparently because the CMAKE_MODULE_PATH logic relies on going backwards to find cmake/modules and does not account that the modules will actually be in the same directory)

[mgorny]

Submitted a fix as D57996.

lib/Basic/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Core
   MC
   Support
   )
 
-find_first_existing_vc_file(llvm_vc "${LLVM_MAIN_SRC_DIR}")
-find_first_existing_vc_file(clang_vc "${CLANG_SOURCE_DIR}")
+find_first_existing_vc_file("${LLVM_MAIN_SRC_DIR}" llvm_vc)
+find_first_existing_vc_file("${CLANG_SOURCE_DIR}" clang_vc)
 
 # The VC revision include that we want to generate.
-set(version_inc "${CMAKE_CURRENT_BINARY_DIR}/SVNVersion.inc")
+set(version_inc "${CMAKE_CURRENT_BINARY_DIR}/VCSVersion.inc")
 
-set(get_svn_script "${LLVM_CMAKE_PATH}/GetSVN.cmake")
+set(generate_vcs_version_script "${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
+
+if(llvm_vc)
+  set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
+endif()
+if(clang_vc)
+  set(clang_source_dir ${CLANG_SOURCE_DIR})
+endif()
 
-if(DEFINED llvm_vc AND DEFINED clang_vc)
-  # Create custom target to generate the VC revision include.
-  add_custom_command(OUTPUT "${version_inc}"
-    DEPENDS "${llvm_vc}" "${clang_vc}" "${get_svn_script}"
-    COMMAND
-    ${CMAKE_COMMAND} "-DFIRST_SOURCE_DIR=${LLVM_MAIN_SRC_DIR}"
-                     "-DFIRST_NAME=LLVM"
-                     "-DSECOND_SOURCE_DIR=${CLANG_SOURCE_DIR}"
-                     "-DSECOND_NAME=SVN"
+  DEPENDS "${llvm_vc}" "${clang_vc}" "${generate_vcs_version_script}"
+  COMMAND ${CMAKE_COMMAND} "-DNAMES=\"LLVM;CLANG\""
+                           "-DLLVM_SOURCE_DIR=${llvm_source_dir}"
+                           "-DCLANG_SOURCE_DIR=${clang_source_dir}"
                            "-DHEADER_FILE=${version_inc}"
-                     -P "${get_svn_script}")
+                           -P "${generate_vcs_version_script}")
 
-  # Mark the generated header as being generated.
-  set_source_files_properties("${version_inc}"
-    PROPERTIES GENERATED TRUE
-               HEADER_FILE_ONLY TRUE)
 
-  # Tell Version.cpp that it needs to build with -DHAVE_SVN_VERSION_INC.
-  set_source_files_properties(Version.cpp
-    PROPERTIES COMPILE_DEFINITIONS "HAVE_SVN_VERSION_INC")
-else()
-  # Not producing a VC revision include.
-  set(version_inc)
-
-  # Being able to force-set the SVN revision in cases where it isn't available
-  # is useful for performance tracking, and matches compatibility from autoconf.
-  if(SVN_REVISION)
-    set_source_files_properties(Version.cpp
-      PROPERTIES COMPILE_DEFINITIONS "SVN_REVISION=\"${SVN_REVISION}\"")
-  endif()
-endif()
+set_property(SOURCE Version.cpp APPEND PROPERTY
+             COMPILE_DEFINITIONS "HAVE_VCS_VERSION_INC")
 
 add_clang_library(clangBasic
   Attributes.cpp
   Builtins.cpp
   CharInfo.cpp
   CodeGenOptions.cpp
   Cuda.cpp
   Diagnostic.cpp

lib/Basic/Version.cpp

 
 #include "clang/Basic/Version.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Config/config.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstdlib>
 #include <cstring>
 
-#ifdef HAVE_SVN_VERSION_INC
-#  include "SVNVersion.inc"
+#ifdef HAVE_VCS_VERSION_INC
+#include "VCSVersion.inc"
 #endif
 
 namespace clang {
 
 std::string getClangRepositoryPath() {
 #if defined(CLANG_REPOSITORY_STRING)
   return CLANG_REPOSITORY_STRING;
 #else
-#ifdef SVN_REPOSITORY
-  StringRef URL(SVN_REPOSITORY);
+#ifdef CLANG_REPOSITORY
+  StringRef URL(CLANG_REPOSITORY);
 #else
   StringRef URL("");
 #endif
 
-  // If the SVN_REPOSITORY is empty, try to use the SVN keyword. This helps us
+  // If the CLANG_REPOSITORY is empty, try to use the SVN keyword. This helps us
   // pick up a tag in an SVN export, for example.
   StringRef SVNRepository("$URL$");
   if (URL.empty()) {
     URL = SVNRepository.slice(SVNRepository.find(':'),
                               SVNRepository.find("/lib/Basic"));
   }
 
   // Strip off version from a build from an integration branch.
#endif
   size_t Start = URL.find("llvm/");
   if (Start != StringRef::npos)
     URL = URL.substr(Start);
 
   return URL;
 }
 
 std::string getClangRevision() {
-#ifdef SVN_REVISION
-  return SVN_REVISION;
+#ifdef CLANG_REVISION
+  return CLANG_REVISION;
 #else
   return "";
 #endif
 }
 
 std::string getLLVMRevision() {
 #ifdef LLVM_REVISION
   return LLVM_REVISION;