pdhaliwal added a comment. In D79400#2021255 <https://reviews.llvm.org/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
