This is an archive of the discontinued LLVM Phabricator instance.

Support: Add a VCSRevision.h header file.
ClosedPublic

Authored by pcc on Apr 12 2017, 11:11 AM.
Tokens
"Cup of Joe" token, awarded by qneill2014.

Details

Summary

This is a magic header file supported by the build system that provides a
single definition, LLVM_REVISION, containing an LLVM revision identifier,
if available. This functionality previously lived in the LTO library, but
I am moving it out to lib/Support because I want to also start using it in
lib/Object to create the IR symbol table.

This change also fixes a bug where LLVM_REVISION was never actually being
used in lib/LTO because the macro HAS_LLVM_REVISION was never defined (it
was misspelled as HAVE_SVN_VERSION_INC in lib/LTO/CMakeLists.txt, and was
only being defined in a non-existent file Version.cpp).

I also changed the code to use "git rev-parse --git-dir" to locate the .git
directory, instead of looking for it in the LLVM source root directory,
which makes this compatible with monorepos as well as git worktrees.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Apr 12 2017, 11:11 AM
pcc updated this revision to Diff 95003.Apr 12 2017, 11:15 AM

Remove some dead code

beanz accepted this revision.Apr 12 2017, 1:43 PM

LGTM.

This revision is now accepted and ready to land.Apr 12 2017, 1:43 PM
This revision was automatically updated to reflect the committed changes.

FYI this breaks downstream builders that rely on the "repo" utility, since the file ".git/logs/HEAD" may not exist.

pcc added a comment.Apr 20 2017, 1:22 PM

FYI this breaks downstream builders that rely on the "repo" utility, since the file ".git/logs/HEAD" may not exist.

Hi Quentin, please let me know whether this patch fixes the problem in your scenario.

diff --git a/llvm/include/llvm/Support/CMakeLists.txt b/llvm/include/llvm/Support/CMakeLists.txt
index b4b99370574..f1901e55004 100644
--- a/llvm/include/llvm/Support/CMakeLists.txt
+++ b/llvm/include/llvm/Support/CMakeLists.txt
@@ -17,7 +17,9 @@ macro(find_first_existing_vc_file out_var path)
     OUTPUT_VARIABLE git_dir)
   if(git_result EQUAL 0)
     string(STRIP "${git_dir}" git_dir)
-    set(${out_var} "${git_dir}/logs/HEAD")
+    if(EXISTS "${git_dir}/logs/HEAD")
+      set(${out_var} "${git_dir}/logs/HEAD")
+    endif()
   else()
     find_first_existing_file(${out_var}
       "${path}/.svn/wc.db"   # SVN 1.7
qneill2014 added a comment.EditedApr 21 2017, 6:17 AM
In D31985#732557, @pcc wrote:

FYI this breaks downstream builders that rely on the "repo" utility, since the file ".git/logs/HEAD" may not exist.

Hi Quentin, please let me know whether this patch fixes the problem in your scenario.

diff --git a/llvm/include/llvm/Support/CMakeLists.txt b/llvm/include/llvm/Support/CMakeLists.txt
index b4b99370574..f1901e55004 100644
--- a/llvm/include/llvm/Support/CMakeLists.txt
+++ b/llvm/include/llvm/Support/CMakeLists.txt
@@ -17,7 +17,9 @@ macro(find_first_existing_vc_file out_var path)
     OUTPUT_VARIABLE git_dir)
   if(git_result EQUAL 0)
     string(STRIP "${git_dir}" git_dir)
-    set(${out_var} "${git_dir}/logs/HEAD")
+    if(EXISTS "${git_dir}/logs/HEAD")
+      set(${out_var} "${git_dir}/logs/HEAD")
+    endif()
   else()
     find_first_existing_file(${out_var}
       "${path}/.svn/wc.db"   # SVN 1.7

Thanks. That fixes the build breakage, but it still won't work with "repo" managed repositories. The "repo" utility manages .git/logs directories itself and doesn't seem to change ".git/logs/HEAD" at all when using manifests that track remote branches (no local branches are used).

I think you could just use ".git/HEAD" itself. If this feature is merely tracking a dependency to detect when to regenerate the header, that should work perfectly.

diff --git a/include/llvm/Support/CMakeLists.txt b/include/llvm/Support/CMakeLists.txt
index b4b9937..377462a 100644
--- a/include/llvm/Support/CMakeLists.txt
+++ b/include/llvm/Support/CMakeLists.txt
@@ -17,7 +17,7 @@ macro(find_first_existing_vc_file out_var path)
     OUTPUT_VARIABLE git_dir)
   if(git_result EQUAL 0)
     string(STRIP "${git_dir}" git_dir)
-    set(${out_var} "${git_dir}/logs/HEAD")
+    set(${out_var} "${git_dir}/HEAD")
   else()
     find_first_existing_file(${out_var}
       "${path}/.svn/wc.db"   # SVN 1.7

Thanks. That fixes the build breakage, but it still won't work with "repo" managed repositories....

(Just in case my comment edits above didn't get noticed - reposting the last part with my proposed diff again.)

I think you could just use ".git/HEAD" itself. If this feature is merely tracking a dependency to detect when to regenerate the header, that should work perfectly.

This works for the "repo" case. Let me know @pcc if that works for your tests, and if you will post the fix or if you would like me to do it.

diff --git a/include/llvm/Support/CMakeLists.txt b/include/llvm/Support/CMakeLists.txt
index b4b9937..377462a 100644
--- a/include/llvm/Support/CMakeLists.txt
+++ b/include/llvm/Support/CMakeLists.txt
@@ -17,7 +17,7 @@ macro(find_first_existing_vc_file out_var path)
     OUTPUT_VARIABLE git_dir)
   if(git_result EQUAL 0)
     string(STRIP "${git_dir}" git_dir)
-    set(${out_var} "${git_dir}/logs/HEAD")
+    set(${out_var} "${git_dir}/HEAD")
   else()
     find_first_existing_file(${out_var}
       "${path}/.svn/wc.db"   # SVN 1.7
pcc added a comment.Apr 21 2017, 12:44 PM

Thanks for looking into this, I agree that we should be using .git/HEAD and your fix LGTM.

Do you have commit access?

pcc added a comment.Apr 21 2017, 12:48 PM

Actually, I think that won't work because it looks like .git/HEAD does not get updated if a commit is made on a branch.

An alternative solution would be to touch .git/logs/HEAD if it does not exist.

qneill2014 added a comment.EditedApr 25 2017, 8:26 PM
In D31985#734032, @pcc wrote:

Actually, I think that won't work because it looks like .git/HEAD does not get updated if a commit is made on a branch.

Hmm, you are right, because .git/HEAD holds a ref

An alternative solution would be to touch .git/logs/HEAD if it does not exist.

I checked and this seems to work, it doesn't seem to affect our typical usage ('repo init' ... 'repo sync').

To answer your other question, I haven't committed anything for a couple of years, so practically I don't have commit access; please submit a patch

FWIW I am testing this locally to see if anything else pops up:

diff --git a/include/llvm/Support/CMakeLists.txt b/include/llvm/Support/CMakeLists.txt
index b4b9937..41e2668 100644
--- a/include/llvm/Support/CMakeLists.txt
+++ b/include/llvm/Support/CMakeLists.txt
@@ -18,6 +18,10 @@ macro(find_first_existing_vc_file out_var path)
   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
pcc added a comment.Apr 27 2017, 10:17 AM
In D31985#734032, @pcc wrote:

Actually, I think that won't work because it looks like .git/HEAD does not get updated if a commit is made on a branch.

Hmm, you are right, because .git/HEAD holds a ref

An alternative solution would be to touch .git/logs/HEAD if it does not exist.

I checked and this seems to work, it doesn't seem to affect our typical usage ('repo init' ... 'repo sync').

To answer your other question, I haven't committed anything for a couple of years, so practically I don't have commit access; please submit a patch

FWIW I am testing this locally to see if anything else pops up:

diff --git a/include/llvm/Support/CMakeLists.txt b/include/llvm/Support/CMakeLists.txt
index b4b9937..41e2668 100644
--- a/include/llvm/Support/CMakeLists.txt
+++ b/include/llvm/Support/CMakeLists.txt
@@ -18,6 +18,10 @@ macro(find_first_existing_vc_file out_var path)
   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

Thanks, I have committed your patch as r301565.

In D31985#739662, @pcc wrote:
In D31985#734032, @pcc wrote:

Actually, I think that won't work because it looks like .git/HEAD does not get updated if a commit is made on a branch.

Hmm, you are right, because .git/HEAD holds a ref

An alternative solution would be to touch .git/logs/HEAD if it does not exist.

I checked and this seems to work, it doesn't seem to affect our typical usage ('repo init' ... 'repo sync').

To answer your other question, I haven't committed anything for a couple of years, so practically I don't have commit access; please submit a patch

FWIW I am testing this locally to see if anything else pops up:

diff --git a/include/llvm/Support/CMakeLists.txt b/include/llvm/Support/CMakeLists.txt
index b4b9937..41e2668 100644
--- a/include/llvm/Support/CMakeLists.txt
+++ b/include/llvm/Support/CMakeLists.txt
@@ -18,6 +18,10 @@ macro(find_first_existing_vc_file out_var path)
   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

Thanks, I have committed your patch as r301565.

Thanks! Cheers.