Page MenuHomePhabricator

[Basic] Update CMakeLists.txt to handle Repo with git
ClosedPublic

Authored by minseong.kim on Jul 18 2017, 12:53 AM.

Details

Summary

The find_first_existing_file and find_first_existing_vc_file macros in lib/Basic/CMakeLists.txt are removed. The macros are also defined in {LLVM}/cmake/modules/AddLLVM.cmake for the same purpose. This change serves the following 2 objectives:

  1. To remove the redundant code in clang to use the same macros in llvm,
  2. The macros in AddLLVM.cmake can also handle repo for displaying correct version information.

Diff Detail

Repository
rL LLVM

Event Timeline

minseong.kim created this revision.Jul 18 2017, 12:53 AM
jordan_rose edited edge metadata.Jul 18 2017, 10:26 AM

As commented at D34955#798369, this isn't sufficient because it doesn't force a rebuild when new changes are merged in from upstream. It's not harmful to stick with the old version information, but if you can find something better that would be, um, better.

hintonda added inline comments.
lib/Basic/CMakeLists.txt
17 ↗(On Diff #107026)

LLVM already has a version of find_first_existing_vc_file in llvm/include/llvm/Support/CMakelists.txt.

Would it make sense move it to an llvm module and reuse it in clang?

Thanks for the reviewing this patch. Please correct if I am wrong.

The addition of "${path}/.git/HEAD" at line 37 is used only once when repo is initially synced "for the first time". This is because ${git_path}/logs/HEAD file at line 34 for Git or Git submodule does not exist with initial repo sync and therefore no generation of SVNVersion.inc which is referenced by --version option, resulting null for the revision values of clang and llvm. After the second repo sync, the first file "${git_path}/logs/HEAD" will be created and can be used to generate "SVNVersion.inc". This enables correct version information to be displayed even if new changes are merged.

lib/Basic/CMakeLists.txt
17 ↗(On Diff #107026)

Thanks for the suggestion.
My understanding is that "llvm/include/llvm/Support/CMakeLists.txt" is used to generate VCSRevision.h which is used by llvm-specific modules such as opt, not clang. Furthermore find_first_existing_vc_file function in llvm/include/llvm/Support/CMakeLists.txt does not handle the version info either.

The addition of "${path}/.git/HEAD" at line 37 is used only once when repo is initially synced "for the first time". This is because ${git_path}/logs/HEAD file at line 34 for Git or Git submodule does not exist with initial repo sync[.]

Hm, interesting. Unfortunately, adding more commits won't automatically force a reconfigure. Maybe we should consider adding all of these as dependencies? cc @modocache, who just changed some of this.

Thanks @jordan_rose @modocache @hintonda for your time and efforts.
This patch does regenerate the version control info correctly (SVNVersion.inc) every time I re-make clang. Probably I am missing something here.
Could you please be more specific and share your idea about "adding all of these as dependencies" to force reconfigure revision info whenever new commits added.
I will gladly change this patch accordingly.

I've submitted a patch, https://reviews.llvm.org/D36971, that moves find_first_existing_file and
find_first_existing_vc_file to ADDLLVM so they can be reused here. If that patch is accepted and lands,
you can then remove these versions and check to see if that solves your problem.

Note that you must remove these versions in order for cmake to use the moved ones, which would be hidden
by the new ones, but can still be accessed by adding a '_' prefix.

lib/Basic/CMakeLists.txt
17 ↗(On Diff #107026)

I believe the version of find_first_vc_file in llvm/Support/CMakeLists.txt handles this case correctly. So, instead of fixing/maintaining this version, which is slightly different, perhaps the version in llvm/Support/CMakeList.txt could be moved to ADDLLVM.cmake where it could be used by both.

I will test your patch with repo. Thanks for your time and efforts, @hintonda.

minseong.kim retitled this revision from [Basic] Update CMakeLists.txt to handle Repo to [Basic] Update CMakeLists.txt to handle Repo with git.Sep 6 2017, 2:32 AM
minseong.kim edited the summary of this revision. (Show Details)
minseong.kim added a reviewer: modocache.
minseong.kim removed a subscriber: modocache.

I have updated the description with a hope for it to be more descriptive.

Kindly ping~

hintonda added a comment.EditedSep 6 2017, 7:07 AM

Now that https://reviews.llvm.org/D36971 has been committed, I believe you can just remove the find_first_existing_file and find_first_existing_vc_file macro definitions from this file.

Hi~ @hintonda,

Using using find_first_existing_file in ADDLLVM.cmake solves the cases with repo in conjunction with D35532. However, I am not sure it can handle @modocache's git submodule cases (D34955).

@modocache, could you please check removing find_first_existing_file and find_first_existing_vc_file from lib/Basic/CMakeLists.txt solves git submodule cases ?

If so, I will remove them from clang's cmake to make one unified place for the same functionality.

Hi~ @hintonda,

Using using find_first_existing_file in ADDLLVM.cmake solves the cases with repo in conjunction with D35532. However, I am not sure it can handle @modocache's git submodule cases (D34955).

@modocache, could you please check removing find_first_existing_file and find_first_existing_vc_file from lib/Basic/CMakeLists.txt solves git submodule cases ?

If so, I will remove them from clang's cmake to make one unified place for the same functionality.

Essentially, I think having a single version that works in all cases would be best.
I really haven't looked into it, but would it make sense to incorporate @modocache's module changes into the version in AddLLVM.cmake?

I just looked into both approaches and believe the one taken in AddLLVM.cmake is correct and handles the submodule case correctly. It uses git rev-parse --git-dir to find the location of the actual .git directory.

Please see https://reviews.llvm.org/D31985 for details.

@hintonda, Absolutely. Incorporating @modocache's module changes into the version in AddLLVM.cmake would solve the current version display issue for repo and do not affect the process of other version control systems (e.g. git, git-svn, svn, and git submodule).

@hintonda, Absolutely. Incorporating @modocache's module changes into the version in AddLLVM.cmake would solve the current version display issue for repo and do not affect the process of other version control systems (e.g. git, git-svn, svn, and git submodule).

I think we crossed, but yes, the version in AddLLVM.cmake handles everything correctly. So, I'd recommend removing these versions.

minseong.kim edited the summary of this revision. (Show Details)
minseong.kim added a reviewer: hintonda.
minseong.kim removed a subscriber: hintonda.

I have updated the diff.

minseong.kim marked 3 inline comments as done.

Re-uploading the patch, removing debug messages accidentally included in the patch.

hintonda accepted this revision.Sep 7 2017, 9:09 AM

LGTM.

This revision is now accepted and ready to land.Sep 7 2017, 9:09 AM
This revision was automatically updated to reflect the committed changes.