Page MenuHomePhabricator

[benchmark] Change GetGitVersion to only check "dirty" when a tag is found
AbandonedPublic

Authored by andrewng on Jun 21 2019, 10:17 AM.

Details

Summary

The check for "dirty" executes the following command:

git update-index -q --refresh

This can take a considerable amount of time if the index does
need to be refreshed (particularly with the mono repo). This
situation can arise when building shared source on a host in
VMs.

This change removes the explicit "dirty" check and adds
the "--dirty" flag to the "git describe".

Diff Detail

Event Timeline

andrewng created this revision.Jun 21 2019, 10:17 AM
Herald added a project: Restricted Project. · View Herald Transcript
  1. Can you just use something along the lines of git describe --always --dirty to avoid the update-index/diff-index?
  2. Can you please either submit this upstream yourself, or explicitly state that it is okay to steal it from you and upstream (to avoid getting out of sync)

(that being said this entire script is kinda bogus, in this sense of integration into other build, the version tag makes no sense since it will be a llvm-one)

  1. Can you just use something along the lines of git describe --always --dirty to avoid the update-index/diff-index?

Yes, you're right, I can just add '--dirty' to the 'describe' and remove the 'dirty' check altogether. This would be equivalent and simpler.

  1. Can you please either submit this upstream yourself, or explicitly state that it is okay to steal it from you and upstream (to avoid getting out of sync)

I didn't realise that this is kept in sync with upstream. I'm happy for any change to be submitted to the upstream benchmark project.

andrewng updated this revision to Diff 206125.Jun 22 2019, 11:55 AM
andrewng edited the summary of this revision. (Show Details)

Simplified as suggested by reviewer.

(that being said this entire script is kinda bogus, in this sense of integration into other build, the version tag makes no sense since it will be a llvm-one)

Yes, it doesn't really apply in this usage scenario. Perhaps the versioning should be changed or disabled in some way?

Whoops, forgot to click "Submit".

  1. Can you just use something along the lines of git describe --always --dirty to avoid the update-index/diff-index?

Yes, you're right, I can just add '--dirty' to the 'describe' and remove the 'dirty' check altogether. This would be equivalent and simpler.

Oh nice.

  1. Can you please either submit this upstream yourself, or explicitly state that it is okay to steal it from you and upstream (to avoid getting out of sync)

I didn't realise that this is kept in sync with upstream. I'm happy for any change to be submitted to the upstream benchmark project.

(I'm not sure which one that is, either is fine)

(that being said this entire script is kinda bogus, in this sense of integration into other build, the version tag makes no sense since it will be a llvm-one)

Yes, it doesn't really apply in this usage scenario. Perhaps the versioning should be changed or disabled in some way?

Yeah, i've thought about it, and i think it will be best to just hardcode v0.0.0 there, avoids the need to submit anything upstream, too

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 9db1361..464cfe2 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -76,8 +76,11 @@ list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
 
 
 # Read the git tags to determine the project version
-include(GetGitVersion)
-get_git_version(GIT_VERSION)
+# wARNING: this is meaningless for when the benchmark is being built in-tree,
+# so we disable it, and hardcode a null version.
+# include(GetGitVersion)
+# get_git_version(GIT_VERSION)
+set(GIT_VERSION "v0.0.0")
 
 # Tell the user what versions we are using
 string(REGEX MATCH "[0-9]+\\.[0-9]+\\.[0-9]+" VERSION ${GIT_VERSION})

Please document that in llvm/utils/benchmark/README.LLVM

andrewng abandoned this revision.Jun 28 2019, 4:27 AM

Abandoning in favour of D63925.