<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">Cool, thanks. One comment still:</div><div class=""><br class=""></div><div class=""><div class="">+ if (Project_WC_REVISION)</div><div class="">+ set(revision ${Project_WC_REVISION} PARENT_SCOPE)</div><div class="">+ endif()</div><div class="">+ if (Project_WC_URL)</div><div class="">+ set(repository ${Project_WC_URL} PARENT_SCOPE)</div><div class="">+ endif()</div></div><div class=""><br class=""></div><div class="">These should be using "${revision}" and "${repository}" instead of "revision" and "repository". It happens to work in this case because the variable names you picked are the same as the variables in the caller, but we should get it right anyway! (The macro arguments are really "revision_var" and "repository_var" or something.)</div><div class=""><br class=""></div><div class="">After fixing that, please feel free to commit!</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><div class="WordSection1" style="page: WordSection1;"><div class="" style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span class="" style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">I am more than happy to let you deal with the non-svn-git-branch-inside-git-svn-clone problem! But it's probably easy to handle by having get_source_info fall back to the git path if git-svn produces no result.<o:p class=""></o:p></span></div></div></blockquote></div><div class=""><div class="WordSection1" style="page: WordSection1;"><div class="" style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span class="" style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);"><br class=""></span></div></div></div><div class="">Something like this. There are cases where git-svn takes a very long time to produce no result, though. We'll see...!</div><div class=""><br class=""></div><div class="">Jordan</div><div class=""><br class=""></div><div class=""><br class=""></div><div><blockquote type="cite" class=""><div class="">On Dec 4, 2014, at 16:15, Robinson, Paul <<a href="mailto:Paul_Robinson@playstation.sony.com" class="">Paul_Robinson@playstation.sony.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="WordSection1" style="page: WordSection1; font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">Hi Jordan,<o:p class=""></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">Sorry for the delay, here's a revised patch.<o:p class=""></o:p></span></div><div style="margin: 0in 0in 0.0001pt 0.5in; font-size: 12pt; font-family: 'Times New Roman', serif; text-indent: -0.25in;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""><span class="">-<span style="font-style: normal; font-variant: normal; font-weight: normal; font-size: 7pt; line-height: normal; font-family: 'Times New Roman';" class=""> <span class="Apple-converted-space"> </span></span></span></span><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">git-svn path uses a single 'git svn info' to extract info; this also lets me steal regexes from FindSubversion.cmake so it really should work exactly like the SVN case.<o:p class=""></o:p></span></div><div style="margin: 0in 0in 0.0001pt 0.5in; font-size: 12pt; font-family: 'Times New Roman', serif; text-indent: -0.25in;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""><span class="">-<span style="font-style: normal; font-variant: normal; font-weight: normal; font-size: 7pt; line-height: normal; font-family: 'Times New Roman';" class=""> <span class="Apple-converted-space"> </span></span></span></span><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">git path doesn't assume the name of the remote. That regex might be more paranoid than it needs to be, but it works.<o:p class=""></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><a name="_MailEndCompose" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">I've now tried this with git, git-svn, and svn.<o:p class=""></o:p></span></a></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""> </span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">I am more than happy to let you deal with the non-svn-git-branch-inside-git-svn-clone problem! But it's probably easy to handle by having get_source_info fall back to the git path if git-svn produces no result.<o:p class=""></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">Thanks,<o:p class=""></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">--paulr<o:p class=""></o:p></span></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""> </span></div><div style="border-style: none none none solid; border-left-color: blue; border-left-width: 1.5pt; padding: 0in 0in 0in 4pt;" class=""><div class=""><div style="border-style: solid none none; border-top-color: rgb(181, 196, 223); border-top-width: 1pt; padding: 3pt 0in 0in;" class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><b class=""><span style="font-size: 10pt; font-family: Tahoma, sans-serif;" class="">From:</span></b><span style="font-size: 10pt; font-family: Tahoma, sans-serif;" class=""><span class="Apple-converted-space"> </span>Jordan Rose [<a href="mailto:jordan_rose@apple.com" class="">mailto:jordan_rose@apple.com</a>]<span class="Apple-converted-space"> </span><br class=""><b class="">Sent:</b><span class="Apple-converted-space"> </span>Monday, December 01, 2014 7:21 PM<br class=""><b class="">To:</b><span class="Apple-converted-space"> </span>Robinson, Paul<br class=""><b class="">Cc:</b><span class="Apple-converted-space"> </span>Sean Silva; <a href="mailto:llvm-commits@cs.uiuc.edu" class="">llvm-commits@cs.uiuc.edu</a>; Mark Lacey<br class=""><b class="">Subject:</b><span class="Apple-converted-space"> </span>Re: [PATCH] Fix Windows build<o:p class=""></o:p></span></div></div></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><o:p class=""> </o:p></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class="">Oh right, the Makefile build still exists. :-)<o:p class=""></o:p></div></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><o:p class=""> </o:p></div><div class=""><blockquote style="margin-top: 5pt; margin-bottom: 5pt;" class=""><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class="">On Dec 1, 2014, at 14:12 , Robinson, Paul <<a href="mailto:Paul_Robinson@playstation.sony.com" style="color: purple; text-decoration: underline;" class="">Paul_Robinson@playstation.sony.com</a>> wrote:<o:p class=""></o:p></div></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><o:p class=""> </o:p></div><div class=""><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">Hi Jordan,</span><o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">GetSourceVersion and GetRepositoryPath are still used by clang/lib/Basic/Makefile, so we should not delete them. Your original thought--have make and CMake use the same scripts—was laudable, but we'd have to do something like rewrite this stuff in Python to make it both multi-platform and multi-build-tools compatible. I don't have the Python chops for that, but if somebody else wants to do it, I'm happy to throw away my stuff.</span><o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""> </span><o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">So, I used separate git-svn commands because I was first adapting code from VersionFromVCS.cmake (which uses git svn log) and then translating GetRepositoryPath into CMake (which uses git svn info) and finally filling in the holes regarding getting both bits of info for the 3 source-control options.</span><o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">But of course 'git svn info' has both the URL and the revision, so rejiggering the regex stuff to extract it all from one git command would be preferable.</span><o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class=""> </span><o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);" class="">How does the pure git case "regress" from what GetRepositoryPath does? GetRepositoryPath looks for the "git remote –v" line with "fetch" in it, then prints the URL from that line. I thought my CMake code would do the same. Or should I not be looking for the text "origin"? I was seeing that in my git repo, but I'm not exactly a sophisticated git user. Maybe I should have that match any non-whitespace string?</span><o:p class=""></o:p></div></div></div></blockquote><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class="">I just saw this:<o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class="">+ # FIXME: Extract URL from 'git remote -v'<o:p class=""></o:p></div></div><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><br class="">But maybe I was looking at the wrong version of the patch. Regardless, I would do the same thing GetRepositoryPath does, which doesn't assume the name of the remote.<o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class="">On a related note, Mark mentioned an issue using the git-svn code path when the current git branch<span class="Apple-converted-space"> </span><i class="">isn't</i> based on SVN, but I'll look into that later—no need to block this patch on it.<o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><o:p class=""> </o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class="">Thanks, Paul,<o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class="">Jordan<o:p class=""></o:p></div></div><div class=""><div style="margin: 0in 0in 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;" class=""><o:p class=""> </o:p></div></div></div></div></div><span id="cid:220566B2-F2CD-49BB-9CE2-5DD9C7C0585B@apple.com"><GetSVN3.diff></span></div></blockquote></div><br class=""></body></html>