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

[pdhaliwal]

Summary

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)

Two solutions are proposed,

1. Do not depend on creating .git/logs/HEAD. Instead just skip it. This requires rebuilding VCSRevision header for every build.
2. Gracefully handle file(WRITE ...) error, which this patch is providing. No existing behaviour changes.

For more context,
https://reviews.llvm.org/D31985
https://reviews.llvm.org/D57063

[scott.linder]

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()?

[pdhaliwal]

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.

[pdhaliwal]

Added more context to diff.

[scott.linder]

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.

[pdhaliwal]

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.

[phosek]

@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.

[pdhaliwal]

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.

[scott.linder]

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.

[pdhaliwal]

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

[pdhaliwal]

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 [[ https://cmake.org/cmake/help/v3.0/command/execute_process.html | 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.

[scott.linder]

llvm/cmake/modules/AddLLVM.cmake

1945

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.

[pdhaliwal]

llvm/cmake/modules/AddLLVM.cmake

1945

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#

[scott.linder]

LGTM, thanks!

llvm/cmake/modules/AddLLVM.cmake

1916

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?

llvm/include/llvm/Support/CMakeLists.txt

12

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.

[vsapsai]

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

<cmake configuration>
ninja install
ninja clean

Simpler steps are

<cmake configuration>
ninja include/llvm/Support/all
ninja clean

[pdhaliwal]

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

<cmake configuration>
ninja install
ninja clean

Simpler steps are

<cmake configuration>
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.

clang/lib/Basic/CMakeLists.txt

 
 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()
 

lld/Common/CMakeLists.txt

   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()
 

llvm/cmake/modules/AddLLVM.cmake

         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()

llvm/include/llvm/Support/CMakeLists.txt

 
 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()