[PATCH] D144638: [lit] Detect Inconsistent File Access Times

Tue Feb 28 00:39:45 PST 2023

jhenderson added inline comments.

================
Comment at: llvm/utils/lit/lit/llvm/config.py:171
+            # in the tests that do the same thing.
+            (_, try_touch_err) = self.get_process_output(["touch", "-a", "-t", "199505050555.55", f.name])
+            if try_touch_err != "":
----------------
lenary wrote:
> michaelplatings wrote:
> > lenary wrote:
> > > michaelplatings wrote:
> > > > It looks like this command will be run on Windows. I think it will fail and cause False to be returned, which is the desired result, but this appears to be by accident rather than design. Therefore I'm inclined to agree with @int3 that a hard-coded check would be preferable.
> > > I am going to add the hardcoded checks, but I think `touch` is available in windows, it should be in the same directory as all the git binaries.
> > This is definitely tangential to the change, but in case it's useful to know: conventionally only `C:\Program Files\Git\bin` is added to the path on Windows, not `C:\Program Files\Git\usr\bin`. `C:\Program Files\Git\bin` only contains `bash.exe`, `git.exe` and `sh.exe`.
> Something weirder is happening in `_find_git_windows_unix_tools` (from https://reviews.llvm.org/D84380), but I think it's probably right just to early exit with false on Windows.
I've got no objection to explicitly omitting Windows, but just wanted to point out that LLVM requires various Unix-like tools to be available according to https://llvm.org/docs/GettingStarted.html#software. Whilst `touch` isn't explicitly specified, I'd be surprised if you managed to get access to that explicit list without including `touch` inadvertently (it's also worth noting that there are several other tests that use `touch` and other utilities that are intended to work on Windows). I thought that `Git\usr\bin` was recommended to be included in people's paths for Windows, as the way to get access to these tools, but I couldn't find that recommendation, so maybe that's just our downstream recommendation.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144638