[PATCH] D148262: [clang][cmake] Add options to pass in vcs repo and revision info

[Daniel Rodríguez Troitiño via Phabricator via cfe-commits]

drodriguez requested changes to this revision.
drodriguez added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/lib/Basic/CMakeLists.txt:16
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
 endif()
 if(clang_vc AND LLVM_APPEND_VC_REV)
----------------
I imagine you do not need it, but maybe LLVM should have the same changes? There's a `for` loop in `GenerateVersionFromVCS.cmake` that is already going to try to look into `LLVM_VC_REPOSITORY/REVISION`.


================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:21
 
-function(append_info name path)
-  if(path)
-    get_source_info("${path}" revision repository)
+function(append_info name path repository revision)
+  if (NOT repository AND NOT revision)
----------------
While I agree that `repository` and then `revision` makes more sense, the existing `get_source_info` receives them in the opposite order.


================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:23-25
+    if(path)
+      get_source_info("${path}" revision repository)
+    endif()
----------------
Nit: Looks like tabs were added?


================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:47-49
+    if(NOT DEFINED ${name}_SOURCE_DIR)
+      message(FATAL_ERROR "${name}_SOURCE_DIR is not defined")
+    endif()
----------------
Tabs?


================
Comment at: llvm/cmake/modules/GenerateVersionFromVCS.cmake:50
+    endif()
+    append_info(${name} "${${name}_SOURCE_DIR}" "" "")
   endif()
----------------
I would recommend changing the flow a little bit:

- Remove `path` argument from `append_info`. Leave `name`, `revision`, and `repository`.
- Remove the `if(path) get_source_info() endif()` part.
- Change this `foreach` to the following:

```
foreach(name IN LISTS NAMES)
  if(${name}_VC_REPOSITORY AND ${name}_VC_REVISION)
   set(revision ${${name}_VC_REVISION})
   set(repository ${${name}_VC_REPOSITORY})
  elseif(DEFINED ${name}_SOURCE_DIR)
    get_source_info(${${name}_SOURCE_DIR} revision repository)
  else()
    message(FATAL_ERROR "${name}_SOURCE_DIR is not defined")
  endif()
  append_info(${name} ${revision} ${repository})
endforeach()
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148262/new/

https://reviews.llvm.org/D148262