Running the hook for multiple files causes an issue when downloading the binary for the first time
Opened this issue · 3 comments
buildifier...............................................................Failed
- hook id: buildifier
- exit code: 126
/root/.cache/pre-commit/repo1ap53cxa/buildifier-wrapper.sh: line 53: /root/.cache/pre-commit/buildifier/linux-amd64-6.0.0/buildifier/buildifier-fix: Text file busy
mv: cannot create regular file '/root/.cache/pre-commit/buildifier/linux-amd64-6.0.0/buildifier/buildifier-fix': File exists
It is critical when running it in CI, for example, in Jenkins.
The command line I used.
pre-commit run --all-files
Assuming that there is a BUILD file in the root directory of the cloned repo, I am resolving this issue by running the following command but not ideal.
pre-commit run buildifier --files BUILD.bazel &&
pre-commit run --all-files
Are you on the most up to date version? I've only seen this issue when running multiple hooks in parallel, but I think that has mostly been resolved
I am starting to see this from time to time in our CI pipeline (v6.4). I believe the core issue is in the lines buildifer-wrapper.sh#L57-L70 which is still in the latest version.
There are a couple of things that might contribute to the problem.
First, $binary is being removed when $tmp_binary has an invalid checksum, shouldn't $tmp_binary be removed instead? I'm not sure if you can ever get to this point other than a malicious actor modifying the binary, so this might not contribute to the problem.
Second, if $binary does need to be written to the cache, I don't think this code will actually address the race condition. Consider the case where $TMPDIR (used by mktemp) is on a different filesystem than $HOME (where $binary is finally written). This mv will result in copy rather than just an update to the inode entry, resulting in a larger concurrency window than anticipated. Perhaps set tmp_binary=$(mktemp --tmpdir=$binary_dir).