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

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 >

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

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:

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

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/

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

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/

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

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/

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

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

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

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 a

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

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/Suppor

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

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

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

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

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

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

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

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 dependen

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

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,

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

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

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

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 ch

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

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 revie

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

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 heade

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

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/CM

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

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`

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

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 configur

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

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/