subject:"\[PATCH\] D79400\: \[CMAKE\] Fix build failure when source directory is read only"

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-06-30 Thread Pushpinder Singh via Phabricator via cfe-commits

pdhaliwal added a comment.

In D79400#2121685 , @vsapsai wrote:

> Seems like this change causes `ninja clean` to fail with the error
>
> > ninja: error: remove(include/llvm/Support): Directory not empty
>
> Full repro steps are
>
>   
>   ninja install
>   ninja clean
>
>
> Simpler steps are
>
>   
>   ninja include/llvm/Support/all
>   ninja clean
>

Thanks @vsapsai for reporting this.

I found that when `${fake_version_inc}` is not defined, cmake is still 
considering its value even if it is empty. I think this is the reason why clean 
fails. When I remove `${fake_version_inc}` from `add_custom_command`, clean 
starts working without any issues. I have submitted the patch for fix here -> 
https://reviews.llvm.org/D82847. Please let me know if it fixes your issue.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-06-29 Thread Volodymyr Sapsai via Phabricator via cfe-commits

vsapsai added a comment.

Seems like this change causes `ninja clean` to fail with the error

> ninja: error: remove(include/llvm/Support): Directory not empty

Full repro steps are

  
  ninja install
  ninja clean

Simpler steps are

  
  ninja include/llvm/Support/all
  ninja clean


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-29 Thread Sameer Sahasrabuddhe via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG16fef6d0b46f: Fix build failure when source is read only 
(authored by pdhaliwal, committed by sameerds).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400

Files:
  llvm/cmake/modules/AddLLVM.cmake
  llvm/include/llvm/Support/CMakeLists.txt


Index: llvm/include/llvm/Support/CMakeLists.txt
===
--- llvm/include/llvm/Support/CMakeLists.txt
+++ llvm/include/llvm/Support/CMakeLists.txt
@@ -5,12 +5,19 @@
 
 set(generate_vcs_version_script 
"${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(llvm_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
+
+  # A fake version file and is not expected to exist. It is being used to
+  # force regeneration of VCSRevision.h for source directory with no write
+  # permission available.
+  if (NOT llvm_vc)
+set(fake_version_inc "${CMAKE_CURRENT_BINARY_DIR}/__FakeVCSRevision.h")
+  endif()
 endif()
 
 # Create custom target to generate the VC revision include.
-add_custom_command(OUTPUT "${version_inc}"
+add_custom_command(OUTPUT "${version_inc}" "${fake_version_inc}"
   DEPENDS "${llvm_vc}" "${generate_vcs_version_script}"
   COMMAND ${CMAKE_COMMAND} "-DNAMES=LLVM"
"-DLLVM_SOURCE_DIR=${llvm_source_dir}"
@@ -22,5 +29,5 @@
   PROPERTIES GENERATED TRUE
  HEADER_FILE_ONLY TRUE)
 
-add_custom_target(llvm_vcsrevision_h DEPENDS "${version_inc}")
+add_custom_target(llvm_vcsrevision_h ALL DEPENDS "${version_inc}" 
"${fake_version_inc}")
 set_target_properties(llvm_vcsrevision_h PROPERTIES FOLDER "Misc")
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -2118,7 +2118,13 @@
 get_filename_component(git_dir ${git_output} ABSOLUTE BASE_DIR ${path})
 # Some branchless cases (e.g. 'repo') may not yet have .git/logs/HEAD
 if (NOT EXISTS "${git_dir}/logs/HEAD")
-  file(WRITE "${git_dir}/logs/HEAD" "")
+  execute_process(COMMAND ${CMAKE_COMMAND} -E touch HEAD
+WORKING_DIRECTORY "${git_dir}/logs"
+RESULT_VARIABLE touch_head_result
+ERROR_QUIET)
+  if (NOT touch_head_result EQUAL 0)
+return()
+  endif()
 endif()
 set(${out_var} "${git_dir}/logs/HEAD" PARENT_SCOPE)
   endif()


Index: llvm/include/llvm/Support/CMakeLists.txt
===
--- llvm/include/llvm/Support/CMakeLists.txt
+++ llvm/include/llvm/Support/CMakeLists.txt
@@ -5,12 +5,19 @@
 
 set(generate_vcs_version_script "${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(llvm_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
+
+  # A fake version file and is not expected to exist. It is being used to
+  # force regeneration of VCSRevision.h for source directory with no write
+  # permission available.
+  if (NOT llvm_vc)
+set(fake_version_inc "${CMAKE_CURRENT_BINARY_DIR}/__FakeVCSRevision.h")
+  endif()
 endif()
 
 # Create custom target to generate the VC revision include.
-add_custom_command(OUTPUT "${version_inc}"
+add_custom_command(OUTPUT "${version_inc}" "${fake_version_inc}"
   DEPENDS "${llvm_vc}" "${generate_vcs_version_script}"
   COMMAND ${CMAKE_COMMAND} "-DNAMES=LLVM"
"-DLLVM_SOURCE_DIR=${llvm_source_dir}"
@@ -22,5 +29,5 @@
   PROPERTIES GENERATED TRUE
  HEADER_FILE_ONLY TRUE)
 
-add_custom_target(llvm_vcsrevision_h DEPENDS "${version_inc}")
+add_custom_target(llvm_vcsrevision_h ALL DEPENDS "${version_inc}" "${fake_version_inc}")
 set_target_properties(llvm_vcsrevision_h PROPERTIES FOLDER "Misc")
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -2118,7 +2118,13 @@
 get_filename_component(git_dir ${git_output} ABSOLUTE BASE_DIR ${path})
 # Some branchless cases (e.g. 'repo') may not yet have .git/logs/HEAD
 if (NOT EXISTS "${git_dir}/logs/HEAD")
-  file(WRITE "${git_dir}/logs/HEAD" "")
+  execute_process(COMMAND ${CMAKE_COMMAND} -E touch HEAD
+WORKING_DIRECTORY "${git_dir}/logs"
+RESULT_VARIABLE touch_head_result
+ERROR_QUIET)
+  if (NOT touch_head_result EQUAL 0)
+return()
+  endif()
 endif()
 set(${out_var} "${git_dir}/logs/HEAD" PARENT_SCOPE)
   endif()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-29 Thread Pushpinder Singh via Phabricator via cfe-commits

pdhaliwal updated this revision to Diff 267153.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400

Files:
  llvm/cmake/modules/AddLLVM.cmake
  llvm/include/llvm/Support/CMakeLists.txt


Index: llvm/include/llvm/Support/CMakeLists.txt
===
--- llvm/include/llvm/Support/CMakeLists.txt
+++ llvm/include/llvm/Support/CMakeLists.txt
@@ -5,12 +5,19 @@
 
 set(generate_vcs_version_script 
"${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(llvm_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
+
+  # A fake version file and is not expected to exist. It is being used to
+  # force regeneration of VCSRevision.h for source directory with no write
+  # permission available.
+  if (NOT llvm_vc)
+set(fake_version_inc "${CMAKE_CURRENT_BINARY_DIR}/__FakeVCSRevision.h")
+  endif()
 endif()
 
 # Create custom target to generate the VC revision include.
-add_custom_command(OUTPUT "${version_inc}"
+add_custom_command(OUTPUT "${version_inc}" "${fake_version_inc}"
   DEPENDS "${llvm_vc}" "${generate_vcs_version_script}"
   COMMAND ${CMAKE_COMMAND} "-DNAMES=LLVM"
"-DLLVM_SOURCE_DIR=${llvm_source_dir}"
@@ -22,5 +29,5 @@
   PROPERTIES GENERATED TRUE
  HEADER_FILE_ONLY TRUE)
 
-add_custom_target(llvm_vcsrevision_h DEPENDS "${version_inc}")
+add_custom_target(llvm_vcsrevision_h ALL DEPENDS "${version_inc}" 
"${fake_version_inc}")
 set_target_properties(llvm_vcsrevision_h PROPERTIES FOLDER "Misc")
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -2118,7 +2118,13 @@
 get_filename_component(git_dir ${git_output} ABSOLUTE BASE_DIR ${path})
 # Some branchless cases (e.g. 'repo') may not yet have .git/logs/HEAD
 if (NOT EXISTS "${git_dir}/logs/HEAD")
-  file(WRITE "${git_dir}/logs/HEAD" "")
+  execute_process(COMMAND ${CMAKE_COMMAND} -E touch HEAD
+WORKING_DIRECTORY "${git_dir}/logs"
+RESULT_VARIABLE touch_head_result
+ERROR_QUIET)
+  if (NOT touch_head_result EQUAL 0)
+return()
+  endif()
 endif()
 set(${out_var} "${git_dir}/logs/HEAD" PARENT_SCOPE)
   endif()


Index: llvm/include/llvm/Support/CMakeLists.txt
===
--- llvm/include/llvm/Support/CMakeLists.txt
+++ llvm/include/llvm/Support/CMakeLists.txt
@@ -5,12 +5,19 @@
 
 set(generate_vcs_version_script "${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(llvm_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
+
+  # A fake version file and is not expected to exist. It is being used to
+  # force regeneration of VCSRevision.h for source directory with no write
+  # permission available.
+  if (NOT llvm_vc)
+set(fake_version_inc "${CMAKE_CURRENT_BINARY_DIR}/__FakeVCSRevision.h")
+  endif()
 endif()
 
 # Create custom target to generate the VC revision include.
-add_custom_command(OUTPUT "${version_inc}"
+add_custom_command(OUTPUT "${version_inc}" "${fake_version_inc}"
   DEPENDS "${llvm_vc}" "${generate_vcs_version_script}"
   COMMAND ${CMAKE_COMMAND} "-DNAMES=LLVM"
"-DLLVM_SOURCE_DIR=${llvm_source_dir}"
@@ -22,5 +29,5 @@
   PROPERTIES GENERATED TRUE
  HEADER_FILE_ONLY TRUE)
 
-add_custom_target(llvm_vcsrevision_h DEPENDS "${version_inc}")
+add_custom_target(llvm_vcsrevision_h ALL DEPENDS "${version_inc}" "${fake_version_inc}")
 set_target_properties(llvm_vcsrevision_h PROPERTIES FOLDER "Misc")
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -2118,7 +2118,13 @@
 get_filename_component(git_dir ${git_output} ABSOLUTE BASE_DIR ${path})
 # Some branchless cases (e.g. 'repo') may not yet have .git/logs/HEAD
 if (NOT EXISTS "${git_dir}/logs/HEAD")
-  file(WRITE "${git_dir}/logs/HEAD" "")
+  execute_process(COMMAND ${CMAKE_COMMAND} -E touch HEAD
+WORKING_DIRECTORY "${git_dir}/logs"
+RESULT_VARIABLE touch_head_result
+ERROR_QUIET)
+  if (NOT touch_head_result EQUAL 0)
+return()
+  endif()
 endif()
 set(${out_var} "${git_dir}/logs/HEAD" PARENT_SCOPE)
   endif()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-29 Thread Pushpinder Singh via Phabricator via cfe-commits

pdhaliwal updated this revision to Diff 267123.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400

Files:
  llvm/cmake/modules/AddLLVM.cmake
  llvm/include/llvm/Support/CMakeLists.txt


Index: llvm/include/llvm/Support/CMakeLists.txt
===
--- llvm/include/llvm/Support/CMakeLists.txt
+++ llvm/include/llvm/Support/CMakeLists.txt
@@ -5,12 +5,19 @@
 
 set(generate_vcs_version_script 
"${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(llvm_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
+
+  # A fake version file and is not expected to exist. It is being used to
+  # force regeneration of VCSRevision.h for source directory with no write
+  # permission available.
+  if (NOT llvm_vc)
+set(fake_version_inc "${CMAKE_CURRENT_BINARY_DIR}/__FakeVCSRevision.h")
+  endif()
 endif()
 
 # Create custom target to generate the VC revision include.
-add_custom_command(OUTPUT "${version_inc}"
+add_custom_command(OUTPUT "${version_inc}" "${fake_version_inc}"
   DEPENDS "${llvm_vc}" "${generate_vcs_version_script}"
   COMMAND ${CMAKE_COMMAND} "-DNAMES=LLVM"
"-DLLVM_SOURCE_DIR=${llvm_source_dir}"
@@ -22,5 +29,5 @@
   PROPERTIES GENERATED TRUE
  HEADER_FILE_ONLY TRUE)
 
-add_custom_target(llvm_vcsrevision_h DEPENDS "${version_inc}")
+add_custom_target(llvm_vcsrevision_h ALL DEPENDS "${version_inc}" 
"${fake_version_inc}")
 set_target_properties(llvm_vcsrevision_h PROPERTIES FOLDER "Misc")
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -2118,7 +2118,13 @@
 get_filename_component(git_dir ${git_output} ABSOLUTE BASE_DIR ${path})
 # Some branchless cases (e.g. 'repo') may not yet have .git/logs/HEAD
 if (NOT EXISTS "${git_dir}/logs/HEAD")
-  file(WRITE "${git_dir}/logs/HEAD" "")
+  execute_process(COMMAND ${CMAKE_COMMAND} -E touch HEAD
+WORKING_DIRECTORY "${git_dir}/logs"
+RESULT_VARIABLE touch_head_result
+ERROR_QUIET)
+  if (NOT touch_head_result EQUAL 0)
+return()
+  endif()
 endif()
 set(${out_var} "${git_dir}/logs/HEAD" PARENT_SCOPE)
   endif()


Index: llvm/include/llvm/Support/CMakeLists.txt
===
--- llvm/include/llvm/Support/CMakeLists.txt
+++ llvm/include/llvm/Support/CMakeLists.txt
@@ -5,12 +5,19 @@
 
 set(generate_vcs_version_script "${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(llvm_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
+
+  # A fake version file and is not expected to exist. It is being used to
+  # force regeneration of VCSRevision.h for source directory with no write
+  # permission available.
+  if (NOT llvm_vc)
+set(fake_version_inc "${CMAKE_CURRENT_BINARY_DIR}/__FakeVCSRevision.h")
+  endif()
 endif()
 
 # Create custom target to generate the VC revision include.
-add_custom_command(OUTPUT "${version_inc}"
+add_custom_command(OUTPUT "${version_inc}" "${fake_version_inc}"
   DEPENDS "${llvm_vc}" "${generate_vcs_version_script}"
   COMMAND ${CMAKE_COMMAND} "-DNAMES=LLVM"
"-DLLVM_SOURCE_DIR=${llvm_source_dir}"
@@ -22,5 +29,5 @@
   PROPERTIES GENERATED TRUE
  HEADER_FILE_ONLY TRUE)
 
-add_custom_target(llvm_vcsrevision_h DEPENDS "${version_inc}")
+add_custom_target(llvm_vcsrevision_h ALL DEPENDS "${version_inc}" "${fake_version_inc}")
 set_target_properties(llvm_vcsrevision_h PROPERTIES FOLDER "Misc")
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -2118,7 +2118,13 @@
 get_filename_component(git_dir ${git_output} ABSOLUTE BASE_DIR ${path})
 # Some branchless cases (e.g. 'repo') may not yet have .git/logs/HEAD
 if (NOT EXISTS "${git_dir}/logs/HEAD")
-  file(WRITE "${git_dir}/logs/HEAD" "")
+  execute_process(COMMAND ${CMAKE_COMMAND} -E touch HEAD
+WORKING_DIRECTORY "${git_dir}/logs"
+RESULT_VARIABLE touch_head_result
+ERROR_QUIET)
+  if (NOT touch_head_result EQUAL 0)
+return()
+  endif()
 endif()
 set(${out_var} "${git_dir}/logs/HEAD" PARENT_SCOPE)
   endif()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-28 Thread Scott Linder via Phabricator via cfe-commits

scott.linder accepted this revision.
scott.linder added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!




Comment at: llvm/cmake/modules/AddLLVM.cmake:1916
   endif()
-  if(EXISTS "${path}/.svn")
-set(svn_files

Nit: can this be in a separate commit (no need for another review), or at least 
be explicitly called out in the commit message of this patch?



Comment at: llvm/include/llvm/Support/CMakeLists.txt:12
+  if (NOT llvm_vc)
+set(fake_version_inc "${CMAKE_CURRENT_BINARY_DIR}/__FakeVCSRevision.h")
+  endif()

Nit: can you add a comment indicating this is never expected to exist, and is 
just used to force regeneration for the read-only case? Otherwise it may not be 
obvious to someone reading what the trick here is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-28 Thread Pushpinder Singh via Phabricator via cfe-commits

pdhaliwal added inline comments.



Comment at: llvm/cmake/modules/AddLLVM.cmake:1945
+  if (NOT touch_head_result EQUAL 0)
+return()
+  endif()

scott.linder wrote:
> This seems to implement the behavior that when the log is not present and is 
> not writable the VCS header target is never re-build. Would it instead make 
> more sense to conservatively assume it should //always// be rebuilt? The 
> latter case is what is implemented by the stackoverflow snippet you mentioned 
> previously, right?
> 
> If the assumption is that the failure to write the logs file implies this 
> source is read-only, then it may make sense to assume it never needs to be 
> rebuilt. However if the sources are changed in another context (e.g. `sudo -u 
> user-with-write-privs git checkout rev`) this won't trigger a rebuild until 
> the user re-runs the CMake config step manually.
> 
> I think I would lean towards the conservative approach of always rebuilding, 
> but this patch as it stands is still an improvement over just falling over in 
> this case.
To retrigger the build when `.git/logs/HEAD` is not writable, I have added 
solution from first answer of the linked stackoverflow. Given my limited 
knowledge of cmake, please let me know if that is correct.

Right now, the old behaviour is preserved as it is for read-write context.

Also, ninja output on nth build for same branch/tag/commit for read-only source 
with missing `.git/logs/HEAD`,
```
root@8c167cb7d5b9:/src/build# ninja -j8
[1/93] Generating VCSRevision.h, __FakeVCSRevision.h
-- Found Git: /usr/bin/git (found version "2.17.1") 
root@8c167cb7d5b9:/src/build#
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-28 Thread Pushpinder Singh via Phabricator via cfe-commits

pdhaliwal updated this revision to Diff 266794.
pdhaliwal marked an inline comment as done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400

Files:
  llvm/cmake/modules/AddLLVM.cmake
  llvm/include/llvm/Support/CMakeLists.txt


Index: llvm/include/llvm/Support/CMakeLists.txt
===
--- llvm/include/llvm/Support/CMakeLists.txt
+++ llvm/include/llvm/Support/CMakeLists.txt
@@ -5,12 +5,16 @@
 
 set(generate_vcs_version_script 
"${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(llvm_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
+
+  if (NOT llvm_vc)
+set(fake_version_inc "${CMAKE_CURRENT_BINARY_DIR}/__FakeVCSRevision.h")
+  endif()
 endif()
 
 # Create custom target to generate the VC revision include.
-add_custom_command(OUTPUT "${version_inc}"
+add_custom_command(OUTPUT "${version_inc}" "${fake_version_inc}"
   DEPENDS "${llvm_vc}" "${generate_vcs_version_script}"
   COMMAND ${CMAKE_COMMAND} "-DNAMES=LLVM"
"-DLLVM_SOURCE_DIR=${llvm_source_dir}"
@@ -22,5 +26,5 @@
   PROPERTIES GENERATED TRUE
  HEADER_FILE_ONLY TRUE)
 
-add_custom_target(llvm_vcsrevision_h DEPENDS "${version_inc}")
+add_custom_target(llvm_vcsrevision_h ALL DEPENDS "${version_inc}" 
"${fake_version_inc}")
 set_target_properties(llvm_vcsrevision_h PROPERTIES FOLDER "Misc")
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -1913,34 +1913,27 @@
   if(NOT EXISTS "${path}")
 return()
   endif()
-  if(EXISTS "${path}/.svn")
-set(svn_files
-  "${path}/.svn/wc.db"   # SVN 1.7
-  "${path}/.svn/entries" # SVN 1.6
-)
-foreach(file IN LISTS svn_files)
-  if(EXISTS "${file}")
-set(${out_var} "${file}" PARENT_SCOPE)
-return()
-  endif()
-endforeach()
-  else()
-find_package(Git)
-if(GIT_FOUND)
-  execute_process(COMMAND ${GIT_EXECUTABLE} rev-parse --git-dir
-WORKING_DIRECTORY ${path}
-RESULT_VARIABLE git_result
-OUTPUT_VARIABLE git_output
-ERROR_QUIET)
-  if(git_result EQUAL 0)
-string(STRIP "${git_output}" git_output)
-get_filename_component(git_dir ${git_output} ABSOLUTE BASE_DIR ${path})
-# Some branchless cases (e.g. 'repo') may not yet have .git/logs/HEAD
-if (NOT EXISTS "${git_dir}/logs/HEAD")
-  file(WRITE "${git_dir}/logs/HEAD" "")
+  find_package(Git)
+  if(GIT_FOUND)
+execute_process(COMMAND ${GIT_EXECUTABLE} rev-parse --git-dir
+  WORKING_DIRECTORY ${path}
+  RESULT_VARIABLE git_result
+  OUTPUT_VARIABLE git_output
+  ERROR_QUIET)
+if(git_result EQUAL 0)
+  string(STRIP "${git_output}" git_output)
+  get_filename_component(git_dir ${git_output} ABSOLUTE BASE_DIR ${path})
+  # Some branchless cases (e.g. 'repo') may not yet have .git/logs/HEAD
+  if (NOT EXISTS "${git_dir}/logs/HEAD")
+execute_process(COMMAND ${CMAKE_COMMAND} -E touch HEAD
+  WORKING_DIRECTORY "${git_dir}/logs"
+  RESULT_VARIABLE touch_head_result
+  ERROR_QUIET)
+if (NOT touch_head_result EQUAL 0)
+  return()
 endif()
-set(${out_var} "${git_dir}/logs/HEAD" PARENT_SCOPE)
   endif()
+  set(${out_var} "${git_dir}/logs/HEAD" PARENT_SCOPE)
 endif()
   endif()
 endfunction()


Index: llvm/include/llvm/Support/CMakeLists.txt
===
--- llvm/include/llvm/Support/CMakeLists.txt
+++ llvm/include/llvm/Support/CMakeLists.txt
@@ -5,12 +5,16 @@
 
 set(generate_vcs_version_script "${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(llvm_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
+
+  if (NOT llvm_vc)
+set(fake_version_inc "${CMAKE_CURRENT_BINARY_DIR}/__FakeVCSRevision.h")
+  endif()
 endif()
 
 # Create custom target to generate the VC revision include.
-add_custom_command(OUTPUT "${version_inc}"
+add_custom_command(OUTPUT "${version_inc}" "${fake_version_inc}"
   DEPENDS "${llvm_vc}" "${generate_vcs_version_script}"
   COMMAND ${CMAKE_COMMAND} "-DNAMES=LLVM"
"-DLLVM_SOURCE_DIR=${llvm_source_dir}"
@@ -22,5 +26,5 @@
   PROPERTIES GENERATED TRUE
  HEADER_FILE_ONLY TRUE)
 
-add_custom_target(llvm_vcsrevision_h DEPENDS "${version_inc}")
+add_custom_target(llvm_vcsrevision_h ALL DEPENDS "${version_inc}" "${fake_version_inc}")
 set_target_properties(llvm_vcsrevision_h PROPERTIES FOLDER "Misc")
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -1913,34 +1913,27 @@
   if(NOT EXISTS

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-27 Thread Scott Linder via Phabricator via cfe-commits

scott.linder added inline comments.



Comment at: llvm/cmake/modules/AddLLVM.cmake:1945
+  if (NOT touch_head_result EQUAL 0)
+return()
+  endif()

This seems to implement the behavior that when the log is not present and is 
not writable the VCS header target is never re-build. Would it instead make 
more sense to conservatively assume it should //always// be rebuilt? The latter 
case is what is implemented by the stackoverflow snippet you mentioned 
previously, right?

If the assumption is that the failure to write the logs file implies this 
source is read-only, then it may make sense to assume it never needs to be 
rebuilt. However if the sources are changed in another context (e.g. `sudo -u 
user-with-write-privs git checkout rev`) this won't trigger a rebuild until the 
user re-runs the CMake config step manually.

I think I would lean towards the conservative approach of always rebuilding, 
but this patch as it stands is still an improvement over just falling over in 
this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-26 Thread Pushpinder Singh via Phabricator via cfe-commits

pdhaliwal updated this revision to Diff 266172.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400

Files:
  llvm/cmake/modules/AddLLVM.cmake


Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -1937,7 +1937,13 @@
 get_filename_component(git_dir ${git_output} ABSOLUTE BASE_DIR ${path})
 # Some branchless cases (e.g. 'repo') may not yet have .git/logs/HEAD
 if (NOT EXISTS "${git_dir}/logs/HEAD")
-  file(WRITE "${git_dir}/logs/HEAD" "")
+  execute_process(COMMAND ${CMAKE_COMMAND} -E touch HEAD
+WORKING_DIRECTORY "${git_dir}/logs"
+RESULT_VARIABLE touch_head_result
+ERROR_QUIET)
+  if (NOT touch_head_result EQUAL 0)
+return()
+  endif()
 endif()
 set(${out_var} "${git_dir}/logs/HEAD" PARENT_SCOPE)
   endif()


Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -1937,7 +1937,13 @@
 get_filename_component(git_dir ${git_output} ABSOLUTE BASE_DIR ${path})
 # Some branchless cases (e.g. 'repo') may not yet have .git/logs/HEAD
 if (NOT EXISTS "${git_dir}/logs/HEAD")
-  file(WRITE "${git_dir}/logs/HEAD" "")
+  execute_process(COMMAND ${CMAKE_COMMAND} -E touch HEAD
+WORKING_DIRECTORY "${git_dir}/logs"
+RESULT_VARIABLE touch_head_result
+ERROR_QUIET)
+  if (NOT touch_head_result EQUAL 0)
+return()
+  endif()
 endif()
 set(${out_var} "${git_dir}/logs/HEAD" PARENT_SCOPE)
   endif()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-26 Thread Pushpinder Singh via Phabricator via cfe-commits

pdhaliwal added a comment.

> 3. Another way is to gracefully handle the file write error, for which I 
> don't think there is a portable way. (which @scott.linder also suggested)

I have found the portable way to do this. So cmake provides a command-line tool 
`touch` (doc 
) 
which can be used with `execute_process` 
. 
`execute_process` provides a way to suppress the failure using `ERROR_QUIET` 
option. Coming back to `touch`, it behaves similar to `file(WRITE ..)` command  
however, additionally the exit code can be used to check if file was created 
successfully. This retains the old behaviour while still solving the original 
problem. I will update the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-26 Thread Pushpinder Singh via Phabricator via cfe-commits

pdhaliwal added a comment.

Hi,

Sorry for late reply.

> If that is not feasible for some reason, I would lean towards your option 
> (2), but I think more is needed in this patch to ensure the generation script 
> is always run, right?

I think you are right. As of now, just removing the dependency on 
`.git/logs/HEAD` is not triggering the cmake script for even when branch is 
changed. One way I tried to solve this was to retain the dependency on 
`.git/logs/HEAD` if it is present and fallback to `.git/HEAD` if it is missing. 
Basically, `.git/HEAD` will ensure the retrigger on every branch-to-branch 
change or headless checkouts but will not retrigger in case of changes on same 
branch. Not sure if it is a good idea.

Another solution that I found was to use the logic mentioned here: 
https://stackoverflow.com/questions/13920072/how-to-always-run-command-when-building-regardless-of-any-dependency.
 This requires a bit more code, but ensures all the required cases are handled. 
I think this should be used only in case of branchless checkouts and keep the 
old behaviour if `.git/logs/HEAD` is present. I will test this and update here 
if it is able to solve the problem.

> We also don't need to check Clang or LLD source tree separately from LLVM now 
> that everything is in one repo, but the logic for determining Git revision 
> should be still correct.

I will submit this as a separate patch once this gets resolved.

Let me know if your thoughts.

Thanks,
Dhaliwal


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-15 Thread Scott Linder via Phabricator via cfe-commits

scott.linder added a comment.

I'd be interested in the answer concerning why we need to avoid `git rev-parse 
HEAD`; it seems like the cleanest solution is to just always check if `git 
rev-parse HEAD` changes to determine whether to regenerate the header.

If that is not feasible for some reason, I would lean towards your option (2), 
but I think more is needed in this patch to ensure the generation script is 
always run, right? How does //removing// a dependency cause the target to be 
executed each build? Don't we need to detect the case where `.git/logs/HEAD` is 
missing and make the target depend on `ALL` or whatever the CMake notion of 
"always out of date" is? We will also need to be OK with the regression in what 
happens when you do a `repo` checkout in a read-write context, as that will 
always run the script whereas before it would have the same behavior as a 
normal checkout. I don't know what the implication of all of these changes are, 
though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-15 Thread Pushpinder Singh via Phabricator via cfe-commits

pdhaliwal added a comment.

I understand that `.git/logs/HEAD` acts as a dependency for `vcs_revision_h` 
target. However, problem here is that cmake fails when it tries to create 
`.git/logs/HEAD` in read-only filesystem.

I had following ideas for solving above issue,

1. Skip creating `.git/logs/HEAD` whenever a cmake variable (say, 
LLVM_SKIP_VC_HEAD_CHECK) is turned `On`.
2. Remove the creation of `.git/logs/HEAD`.
  - So, in cases of normal checkout, `.git/logs/HEAD` will added as dependency 
to generation script and hence latter will not be triggered for every build.
  - In case of `repo` based checkout, the file will not be created and hence 
will not be a dependency to generation script. In this scenario, script will be 
triggered for every build, but will not generate header if HEAD does not change 
as GenerateVersionFromVCS.cmake#L49 

 will take care.
3. Another way is to gracefully handle the `file` write error, for which I 
don't think there is a portable way. (which @scott.linder also suggested)
4. Last way will be to check if we have write permissions, which again is also 
not portable.

Please suggest if there might exist some other solution. This patch simply 
implements solution number 2.

Also, could you elaborate more on "avoid running `git rev-parse HEAD` for every 
build"? I think it should not harm the build time in such a bad way or maybe I 
am missing something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-15 Thread Petr Hosek via Phabricator via cfe-commits

phosek added a comment.
Herald added a reviewer: MaskRay.

@scott.linder is actually correct, the reason we write the file is precisely as 
he described in https://reviews.llvm.org/D79400#2021255. When you use tools 
like `repo` which branchless checkout, `.git/logs/HEAD` won't exist on initial 
checkout, but we want to detect changes to it later. It's not true that 
`.git/logs/HEAD` isn't used, see for example in clang/lib/Basic/CMakeLists.txt 
on line 24, while the header is being generated by the 
`GenerateVersionFromVCS.cmake` script, whether that script is executed is 
controlled by a dependency on `.git/logs/HEAD`, if `.git/logs/HEAD` doesn't 
change we won't rerun that script. The reason we do this rather than calling 
`.git/logs/HEAD` directly is to avoid running `git rev-parse HEAD` on every 
build invocation. The reason we depend on ``.git/logs/HEAD` and not `.git/HEAD` 
is because the latter may only contain symbolic information and so won't change 
e.g. on rebase whereas the former will, in which case we want to regenerate the 
header. I agree that the Subversion part of `find_first_existing_vc_file` could 
be removed now. We also don't need to check Clang or LLD source tree separately 
from LLVM now that everything is in one repo, but the logic for determining Git 
revision should be still correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-07 Thread Pushpinder Singh via Phabricator via cfe-commits

pdhaliwal added a comment.

Removing `find_first_existing_vc_file` makes a lot of sense, as since llvm has 
moved from svn to git, there is no need to have logic for svn dependency. Even 
the generation script is dependent on git executable only. I will keep my patch 
ready just in case other reviewers also feel the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-07 Thread Scott Linder via Phabricator via cfe-commits

scott.linder added a comment.

Thank you for the explanation; I'm also still lost as to why `.git/logs/HEAD` 
is referenced at all then. I would propose removing 
`find_first_existing_vc_file` entirely, as it seems to be completely redundant 
if there is another mechanism for ensuring the VCS header is up-to-date without 
triggering gratuitous rebuilds, but I will defer to any of the other reviewers 
on the `blame` for the scripts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-06 Thread Pushpinder Singh via Phabricator via cfe-commits

pdhaliwal updated this revision to Diff 262299.
pdhaliwal added a comment.

Added more context to diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400

Files:
  clang/lib/Basic/CMakeLists.txt
  lld/Common/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/include/llvm/Support/CMakeLists.txt


Index: llvm/include/llvm/Support/CMakeLists.txt
===
--- llvm/include/llvm/Support/CMakeLists.txt
+++ llvm/include/llvm/Support/CMakeLists.txt
@@ -5,7 +5,7 @@
 
 set(generate_vcs_version_script 
"${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(llvm_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
 endif()
 
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -2110,7 +2110,7 @@
 get_filename_component(git_dir ${git_output} ABSOLUTE BASE_DIR ${path})
 # Some branchless cases (e.g. 'repo') may not yet have .git/logs/HEAD
 if (NOT EXISTS "${git_dir}/logs/HEAD")
-  file(WRITE "${git_dir}/logs/HEAD" "")
+  return()
 endif()
 set(${out_var} "${git_dir}/logs/HEAD" PARENT_SCOPE)
   endif()
Index: lld/Common/CMakeLists.txt
===
--- lld/Common/CMakeLists.txt
+++ lld/Common/CMakeLists.txt
@@ -2,13 +2,12 @@
   set(tablegen_deps intrinsics_gen)
 endif()
 
-find_first_existing_vc_file("${LLVM_MAIN_SRC_DIR}" llvm_vc)
 find_first_existing_vc_file("${LLD_SOURCE_DIR}" lld_vc)
 
 set(version_inc "${CMAKE_CURRENT_BINARY_DIR}/VCSVersion.inc")
 set(generate_vcs_version_script 
"${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(lld_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(lld_source_dir ${LLD_SOURCE_DIR})
 endif()
 
Index: clang/lib/Basic/CMakeLists.txt
===
--- clang/lib/Basic/CMakeLists.txt
+++ clang/lib/Basic/CMakeLists.txt
@@ -12,10 +12,10 @@
 
 set(generate_vcs_version_script 
"${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(llvm_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
 endif()
-if(clang_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(clang_source_dir ${CLANG_SOURCE_DIR})
 endif()
 


Index: llvm/include/llvm/Support/CMakeLists.txt
===
--- llvm/include/llvm/Support/CMakeLists.txt
+++ llvm/include/llvm/Support/CMakeLists.txt
@@ -5,7 +5,7 @@
 
 set(generate_vcs_version_script "${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(llvm_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
 endif()
 
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -2110,7 +2110,7 @@
 get_filename_component(git_dir ${git_output} ABSOLUTE BASE_DIR ${path})
 # Some branchless cases (e.g. 'repo') may not yet have .git/logs/HEAD
 if (NOT EXISTS "${git_dir}/logs/HEAD")
-  file(WRITE "${git_dir}/logs/HEAD" "")
+  return()
 endif()
 set(${out_var} "${git_dir}/logs/HEAD" PARENT_SCOPE)
   endif()
Index: lld/Common/CMakeLists.txt
===
--- lld/Common/CMakeLists.txt
+++ lld/Common/CMakeLists.txt
@@ -2,13 +2,12 @@
   set(tablegen_deps intrinsics_gen)
 endif()
 
-find_first_existing_vc_file("${LLVM_MAIN_SRC_DIR}" llvm_vc)
 find_first_existing_vc_file("${LLD_SOURCE_DIR}" lld_vc)
 
 set(version_inc "${CMAKE_CURRENT_BINARY_DIR}/VCSVersion.inc")
 set(generate_vcs_version_script "${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(lld_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(lld_source_dir ${LLD_SOURCE_DIR})
 endif()
 
Index: clang/lib/Basic/CMakeLists.txt
===
--- clang/lib/Basic/CMakeLists.txt
+++ clang/lib/Basic/CMakeLists.txt
@@ -12,10 +12,10 @@
 
 set(generate_vcs_version_script "${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(llvm_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
 endif()
-if(clang_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(clang_source_dir ${CLANG_SOURCE_DIR})
 endif()
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-06 Thread Pushpinder Singh via Phabricator via cfe-commits

pdhaliwal added a comment.

In D79400#2021255 , @scott.linder 
wrote:

> Can you provide more context in the diff? Looking at the current `master` 
> locally it seems like this patch changes the behavior of the 
> `llvm_vcsrevision_h` target when `logs/HEAD` isn't found, such that it only 
> depends on the generation script. I.e. if you configure CMake without the 
> `HEAD` file present, and then you do something which changes the working 
> directory and creates `HEAD` the target will not be out of date and won't be 
> rebuilt. Before this patch it would have noticed the change to `HEAD` and 
> forced regenerating the version header.

If you look at the generation script 
https://github.com/llvm/llvm-project/blob/master/llvm/cmake/modules/GenerateVersionFromVCS.cmake
 and 
https://github.com/llvm/llvm-project/blob/master/llvm/cmake/modules/VersionFromVCS.cmake,
 these are not directly dependent on `.git/logs/HEAD` instead are dependent on 
`.git/HEAD`. The LLVM_REVISION is generated using following command

  # 
https://github.com/llvm/llvm-project/blob/master/llvm/cmake/modules/VersionFromVCS.cmake#L17
  git rev-parse HEAD

which does not seem to use `.git/logs/HEAD` (instead uses `.git/HEAD` as 
revealed by strace)

> I don't know how much this matters? In the case where the filesystem is 
> read-only from CMake's perspective would it make more sense to always 
> regenerate the header instead, or is the assumption that nobody else is 
> modifying it either?

If `HEAD` changes, only then `VCSRevision.h` will be regenerated. This 
behaviour was present before the patch and is retained after the patch. This is 
because of the fact that there are checks already in place in generator script. 
The way GenerateVersionFromVCS handles this is by first creating a temporary 
file (e.g. VCSRevision.h.tmp), then comparing this with older generated 
VCSRevision.h. If they are different, then copy succeeds otherwise is ignored.

Here is the snippet in GenerateVersionFromVCS,

  # Copy the file only if it has changed.
  execute_process(COMMAND ${CMAKE_COMMAND} -E copy_if_different
"${HEADER_FILE}.tmp" "${HEADER_FILE}")

> Also for the case where the filesystem is not read-only do we want to retain 
> the old behavior? I.e. could we try to write the file and if it fails just 
> proceed rather than `FATAL_ERROR`? I don't know if this is possible with 
> CMake's `file()`?

Even with the patch, there is no change in original behaviour. This patch only 
fixes the build error for read only filesystem. Though going through the 
cmake's documentation 
(https://cmake.org/cmake/help/v3.15/command/file.html#writing), I could not 
find a platform independent way for having a check over `file()` error. Though, 
I think, the check on `.git/logs/HEAD` can be removed entirely.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-05 Thread Scott Linder via Phabricator via cfe-commits

scott.linder added a comment.

Can you provide more context in the diff? Looking at the current `master` 
locally it seems like this patch changes the behavior of the 
`llvm_vcsrevision_h` target when `logs/HEAD` isn't found, such that it only 
depends on the generation script. I.e. if you configure CMake without the 
`HEAD` file present, and then you do something which changes the working 
directory and creates `HEAD` the target will not be out of date and won't be 
rebuilt. Before this patch it would have noticed the change to `HEAD` and 
forced regenerating the version header.

I don't know how much this matters? In the case where the filesystem is 
read-only from CMake's perspective would it make more sense to always 
regenerate the header instead, or is the assumption that nobody else is 
modifying it either?

Also for the case where the filesystem is not read-only do we want to retain 
the old behavior? I.e. could we try to write the file and if it fails just 
proceed rather than `FATAL_ERROR`? I don't know if this is possible with 
CMake's `file()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

2020-05-05 Thread Pushpinder Singh via Phabricator via cfe-commits

pdhaliwal created this revision.
pdhaliwal added reviewers: chandlerc, beanz, pcc.
Herald added subscribers: llvm-commits, cfe-commits, mgorny.
Herald added projects: clang, LLVM.

The AddLLVM.cmake file exposes a function find_first_existing_vc_file which in 
case of git repository looks for .git/logs/HEAD. This file can be missing when 
repository is checked out using repo tool. So, the function tries to create an 
empty file in the same directory. This can fail if the source directory is read 
only. E.g. if the llvm source directory is mounted inside docker as ro 
(read-only).

E.g. error reported,
CMake Error at /src/external/llvm-project/llvm/cmake/modules/AddLLVM.cmake:1940 
(file):

  file failed to open for writing (Read-only file system):

/src/external/llvm-project/.git/logs/HEAD
Call Stack (most recent call first):

  /src/external/llvm-project/clang/lib/Basic/CMakeLists.txt:7 
(find_first_existing_vc_file)

I have made the changes to not create .git/logs/HEAD.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79400

Files:
  clang/lib/Basic/CMakeLists.txt
  lld/Common/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/include/llvm/Support/CMakeLists.txt


Index: llvm/include/llvm/Support/CMakeLists.txt
===
--- llvm/include/llvm/Support/CMakeLists.txt
+++ llvm/include/llvm/Support/CMakeLists.txt
@@ -5,7 +5,7 @@
 
 set(generate_vcs_version_script 
"${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(llvm_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
 endif()
 
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -2110,7 +2110,7 @@
 get_filename_component(git_dir ${git_output} ABSOLUTE BASE_DIR ${path})
 # Some branchless cases (e.g. 'repo') may not yet have .git/logs/HEAD
 if (NOT EXISTS "${git_dir}/logs/HEAD")
-  file(WRITE "${git_dir}/logs/HEAD" "")
+  return()
 endif()
 set(${out_var} "${git_dir}/logs/HEAD" PARENT_SCOPE)
   endif()
Index: lld/Common/CMakeLists.txt
===
--- lld/Common/CMakeLists.txt
+++ lld/Common/CMakeLists.txt
@@ -2,13 +2,12 @@
   set(tablegen_deps intrinsics_gen)
 endif()
 
-find_first_existing_vc_file("${LLVM_MAIN_SRC_DIR}" llvm_vc)
 find_first_existing_vc_file("${LLD_SOURCE_DIR}" lld_vc)
 
 set(version_inc "${CMAKE_CURRENT_BINARY_DIR}/VCSVersion.inc")
 set(generate_vcs_version_script 
"${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(lld_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(lld_source_dir ${LLD_SOURCE_DIR})
 endif()
 
Index: clang/lib/Basic/CMakeLists.txt
===
--- clang/lib/Basic/CMakeLists.txt
+++ clang/lib/Basic/CMakeLists.txt
@@ -12,10 +12,10 @@
 
 set(generate_vcs_version_script 
"${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(llvm_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
 endif()
-if(clang_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(clang_source_dir ${CLANG_SOURCE_DIR})
 endif()
 


Index: llvm/include/llvm/Support/CMakeLists.txt
===
--- llvm/include/llvm/Support/CMakeLists.txt
+++ llvm/include/llvm/Support/CMakeLists.txt
@@ -5,7 +5,7 @@
 
 set(generate_vcs_version_script "${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(llvm_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(llvm_source_dir ${LLVM_MAIN_SRC_DIR})
 endif()
 
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -2110,7 +2110,7 @@
 get_filename_component(git_dir ${git_output} ABSOLUTE BASE_DIR ${path})
 # Some branchless cases (e.g. 'repo') may not yet have .git/logs/HEAD
 if (NOT EXISTS "${git_dir}/logs/HEAD")
-  file(WRITE "${git_dir}/logs/HEAD" "")
+  return()
 endif()
 set(${out_var} "${git_dir}/logs/HEAD" PARENT_SCOPE)
   endif()
Index: lld/Common/CMakeLists.txt
===
--- lld/Common/CMakeLists.txt
+++ lld/Common/CMakeLists.txt
@@ -2,13 +2,12 @@
   set(tablegen_deps intrinsics_gen)
 endif()
 
-find_first_existing_vc_file("${LLVM_MAIN_SRC_DIR}" llvm_vc)
 find_first_existing_vc_file("${LLD_SOURCE_DIR}" lld_vc)
 
 set(version_inc "${CMAKE_CURRENT_BINARY_DIR}/VCSVersion.inc")
 set(generate_vcs_version_script "${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
-if(lld_vc AND LLVM_APPEND_VC_REV)
+if(LLVM_APPEND_VC_REV)
   set(lld_source_dir ${LLD_SOURCE_DIR})
 endif()
 
Index: clang/lib/Basic/CMakeLists.txt

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

[PATCH] D79400: [CMAKE] Fix build failure when source directory is read only

21 matches

Site Navigation

Mail list logo

Footer information