This revision was automatically updated to reflect the committed changes.
Closed by commit rL312865: [Basic] Update CMakeLists.txt to handle repo
(authored by MinSeongKIM).
Changed prior to commit:
https://reviews.llvm.org/D35533?vs=114160=114488#toc
Repository:
rL LLVM
hintonda accepted this revision.
hintonda added a comment.
This revision is now accepted and ready to land.
LGTM.
https://reviews.llvm.org/D35533
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
minseong.kim updated this revision to Diff 114160.
minseong.kim marked 3 inline comments as done.
minseong.kim added a comment.
Re-uploading the patch, removing debug messages accidentally included in the
patch.
https://reviews.llvm.org/D35533
Files:
lib/Basic/CMakeLists.txt
Index:
minseong.kim updated this revision to Diff 114115.
minseong.kim edited the summary of this revision.
minseong.kim added a reviewer: hintonda.
minseong.kim removed a subscriber: hintonda.
minseong.kim added a comment.
I have updated the diff.
https://reviews.llvm.org/D35533
Files:
hintonda added a comment.
In https://reviews.llvm.org/D35533#862852, @minseong.kim wrote:
> @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
minseong.kim added a comment.
@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 added a comment.
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
hintonda added a comment.
In https://reviews.llvm.org/D35533#862798, @minseong.kim wrote:
> Hi~ @hintonda,
>
> Using using find_first_existing_file in ADDLLVM.cmake solves the cases with
> repo in conjunction with https://reviews.llvm.org/D35532. However, I am not
> sure it can handle
minseong.kim added a comment.
Hi~ @hintonda,
Using using find_first_existing_file in ADDLLVM.cmake solves the cases with
repo in conjunction with https://reviews.llvm.org/D35532. However, I am not
sure it can handle @modocache's git submodule cases
(https://reviews.llvm.org/D34955).
hintonda added a comment.
Now that 36971 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.
https://reviews.llvm.org/D35533
___
cfe-commits mailing
minseong.kim added a comment.
I have updated the description with a hope for it to be more descriptive.
Kindly ping~
https://reviews.llvm.org/D35533
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
minseong.kim added a comment.
I will test your patch with repo. Thanks for your time and efforts, @hintonda.
https://reviews.llvm.org/D35533
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
hintonda added a comment.
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
minseong.kim added a comment.
kindly ping~
https://reviews.llvm.org/D35533
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
minseong.kim added a comment.
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
jordan_rose added a subscriber: modocache.
jordan_rose added a comment.
> 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
minseong.kim added a comment.
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
hintonda added inline comments.
Comment at: lib/Basic/CMakeLists.txt:17
macro(find_first_existing_vc_file out_var path)
set(git_path "${path}/.git")
LLVM already has a version of find_first_existing_vc_file in
llvm/include/llvm/Support/CMakelists.txt.
jordan_rose added a comment.
As commented at https://reviews.llvm.org/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
minseong.kim created this revision.
Herald added a subscriber: mgorny.
When repo is used, '--version' option does not display correct version
information (i.e. Git hashes). This change makes the parsing of the
version info correctly recognise svn, git, git-svn and repo. This
in turn enables the
20 matches
Mail list logo
