This is an archive of the discontinued LLVM Phabricator instance.

[CMAKE] Speedup developer builds when passing LLVM_APPEND_VC_REV = OFF
ClosedPublic

Authored by hintonda on Jul 13 2017, 12:40 PM.

Details

Summary

Make sure multiple targets don't get rebuilt unnecessarily when LLVM_APPEND_VC_REV = OFF.

In https://reviews.llvm.org/D34560, the default value for LLVM_APPEND_VC_REV was changed from OFF to ON, and subsequently used to control how VCSRevision.h was created.

While this is a good idea in general, it exposed an existing problem where VCRevision.h was always rewritten if the version control file is not found.

Currently, if LLVM_APPEND_VC_REV = OFF, an empty VCSRevision.h will always get rewritten every time cmake is (re)run,
which causes multiple dependencies to get rebuilt unnecessarily. Please see https://bugs.llvm.org/show_bug.cgi?id=33717 for more info.

This change unifies how VCRevision.h is maintained, and only creates an empty VCRevision.h once if LLVM_APPEND_VC_REV = OFF, minimizing the need to rebuild dependencies.

Diff Detail

Repository
rL LLVM

Event Timeline

hintonda created this revision.Jul 13 2017, 12:40 PM
hintonda updated this revision to Diff 106495.Jul 13 2017, 12:45 PM
  • Fix typo -- replace tab with spaces.
hintonda updated this revision to Diff 106580.Jul 13 2017, 7:21 PM
  • Handle VCSRevision.h correctly when toggling LLVM_APPEND_VC_REV.
dblaikie edited edge metadata.

I don't really have enough cmake-fu to understand what's going on here. (CC'd chandlerc since I think he still technically owns the cmake build... )

Could we also make some bot to check that "cmake/ninja" has nothing to do on incremental build without sync?

Could we also make some bot to check that "cmake/ninja" has nothing to do on incremental build without sync?

If you create a new bot, you'll get the new default value and get the pre r306858.

This change does 2 things:

  • obviate unnecessary work when rerunning cmake if LLVM_APPEND_VC_REV=OFF (which was the previous default and probably still in your CMakeCache if your build directory was setup prior to r306858)
  • allows users to toggle LLVM_APPEND_VC_REV and get correct behavior without needing to run git pull, etc...

Please see https://reviews.llvm.org/D34560 which changed the default value for LLVM_APPEND_VC_REV.

hintonda updated this revision to Diff 106784.Jul 15 2017, 1:20 PM
  • Simplify VCRevision.h creation.
hintonda retitled this revision from [CMAKE] PR33717 When LLVM_APPEND_VC_REV = OFF, only create VCSRevision.h if it doesn't already exist. to [CMAKE] Speedup developer builds when passing LLVM_APPEND_VC_REV = OFF.Jul 17 2017, 2:09 PM
hintonda edited the summary of this revision. (Show Details)
hintonda updated this revision to Diff 106961.Jul 17 2017, 2:54 PM
hintonda edited the summary of this revision. (Show Details)

Simplify VCRevision.h creation when LLVM_APPEN_VC_REV = OFF.

hintonda updated this revision to Diff 106963.Jul 17 2017, 3:06 PM

Fix comments -- NFC.

hintonda updated this revision to Diff 107195.Jul 18 2017, 3:51 PM

Only add custom commands if LLVM_APPEND_VC_REV = ON.

hintonda updated this revision to Diff 107196.Jul 18 2017, 3:55 PM

Only add custom commands if LLVM_APPEND_VC_REV = ON -- minimize diff.

hintonda updated this revision to Diff 107212.Jul 18 2017, 5:20 PM
  • Remove phony target so rebuilds with nothing to do report 'no work to do.'
This revision was automatically updated to reflect the committed changes.