[PATCH] D88850: [lit, windows] Fix the search for git tools on Windows to check the path first

Tue Oct 6 12:09:29 PDT 2020

aganea added inline comments.

================
Comment at: llvm/utils/lit/lit/llvm/config.py:23
         # Tweak PATH for Win32 to decide to use bash.exe or not.
         if sys.platform == 'win32':
             # Seek necessary tools in directories and set to $PATH.
----------------
stella.stamenova wrote:
> aganea wrote:
> > stella.stamenova wrote:
> > > @aganea: From the discussion on zturner's change: Do you know `if sys.platform == 'win32'` is equivalent to `platform.system() == 'windows'`? If it is, we can clean this up. If it is not, we need to bring back `features.add('system-windows')`. As far as I can tell, they should be the same, but we are not using them that way.
> > `sys.platform == 'win32'` should be equivalent to `platform.system() == 'windows'`: https://stackoverflow.com/a/54837707 - were you thinking of an edge case?
> In my experience, there are frequently edge cases around 32b vs. 64b vs. wow64 and I don't know enough about python's platform and sys.platform to be sure that there isn't one here. Certainly, the code is not written as if the two conditions are equivalent.
By quickly browsing through the Python source code (both 2.7.18 and 3.9.0), it seems `sys.platform` always returns `win32` (see PC/pyconfig.h, #define PLATFORM), although values of `win16, dos` were historically supported, but they aren't anymore. `platform.system()` always returns `Windows` regardless of the CPU architecture or the bitness. We can safely assume they are interchangeable when running on Windows.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88850