This is an archive of the discontinued LLVM Phabricator instance.

[cmake] Stop putting the revision info in LLVM_VERSION_STRING
ClosedPublic

Authored by rnk on Aug 29 2017, 1:45 PM.

Details

Summary

This reduces the number of build actions after a no-op commit from
thousands to about six, which should be acceptable. If six actions is
still too many, developers can disable the LLVM_APPEND_VC_REV cmake
option.

llvm-config.h is a widely included header that should rarely change.
Before this patch, it would change after every re-configure. Very few
users of llvm-config.h need to know the precise version, and those that
do can migrate to incorporating LLVM_REVISION as provided by
llvm/Support/VCSRevision.h.

This should bring LLVM back to the behavior that it had before r306858
from June 30 2017. Most LLVM tools will now print a version string like
"6.0.0svn" instead of "6.0.0-git-c40c2a23de4".

Fixes PR34308

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Aug 29 2017, 1:45 PM
hans accepted this revision.Aug 29 2017, 2:04 PM
hans added inline comments.
llvm/CMakeLists.txt
207 ↗(On Diff #113150)

I had to do a double-take to understand how this affects llvm-config.h.. but I see the function here appends to PACKAGE_VERSION which is then used in the header. ok.

llvm/docs/CMake.rst
249 ↗(On Diff #113150)

I suppose it's not actually appending anything anymore, though.

This revision is now accepted and ready to land.Aug 29 2017, 2:04 PM
rnk added inline comments.Aug 29 2017, 2:19 PM
llvm/docs/CMake.rst
249 ↗(On Diff #113150)

Yeah, it's really "embedding" the revision somewhere. Let's do a rename in a follow-up. Using a new option name will ensure everyone gets the default build behavior again.

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jan 29 2019, 8:59 AM

Should we guard clang's SVNVersion.inc and LLD's LLD_REVISION behind this too?

Should we guard clang's SVNVersion.inc and LLD's LLD_REVISION behind this too?

+1, make it so. I'm not sure how popular this option is in practice. I think mainly @espindola used it, but I'm sure there are a small handful of other developers that know about it.