This is an archive of the discontinued LLVM Phabricator instance.

[cmake] GetSVN.cmake takes a list of arguments
ClosedPublic

Authored by modocache on Jul 7 2017, 11:11 AM.

Details

Summary

GetSVN.cmake currently takes one or two pairs of <source directory path,
name>, then attempts to get the remote repository URL and source control
revision of the repositories at those one or two paths.

It takes two pairs in order for Clang to get the revision of both
itself, and its dependency LLVM.

For projects that rely upon both LLVM and Clang (Apple's Swift is one
example, but there are others), GetSVN.cmake is used to fetch *three*
revisions: Swift, Clang, and LLVM.

To support this use case, change GetSVN.cmake: instead of taking one or
two pairs (specified via FIRST_SOURCE_DIR/FIRST_NAME, have it take a list
of pairs (SOURCE_DIRS/NAMES).

In order to allow Clang to migrate, have GetSVN.cmake support both sets
of arguments for now. The old arguments can be removed once Clang begins
using the new arguments, and Swift can follow when it updates its
copy of LLVM.

Test Plan:

  1. Perform a clean build of Clang, verify that clang --version still prints the correct LLVM and Clang revision information.
  2. Modify Clang's CMake to use the new arguments, then perform another clean build. clang --version should still print the correct revision information.

Diff Detail

Repository
rL LLVM

Event Timeline

modocache created this revision.Jul 7 2017, 11:11 AM

Here's an example of how a repository that requires three sets of source control information would use these new parameters, from Apple's Swift: https://github.com/apple/swift/pull/10824

Friendly ping! I'm pretty excited about this set of changes, which simplify the CMake in projects such as Swift:

  1. https://reviews.llvm.org/D35132: Convert "first", "second" parameters to a parameter that takes a list of items
  2. https://reviews.llvm.org/D35133: Use the new parameters in Clang's CMake
  3. https://reviews.llvm.org/D35138: Delete the old "first", "second" parameters

Does anyone have any feedback on these changes?

From a high level it sounds fine. You'll probably want someone else that's hacked on cmake to give an ack though. :)

jordan_rose edited edge metadata.Jul 18 2017, 2:41 PM

It looks fine to me too. Ideally @beanz would be around to weigh in but it's not such a big change. I think it's okay to land.

This revision was automatically updated to reflect the committed changes.

Awesome, thanks to both of you! I went ahead and committed it, going off of @jordan_rose's comment. :)