[PATCH] D84380: [lit] Support running tests on Windows without GnuWin32.

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth requested changes to this revision.
amccarth added a comment.
This revision now requires changes to proceed.

I'm okay with the idea.  I haven't used GnuWin32 in long while since I already have Git's usr/bin in my PATH.  See detailed comments inline.

In the big picture, this unspools like this:

Old code:

  if lit_tool_path is an absolute directory
      if it contains all of the tools needed
          append lit_tool_path to the PATH  (other tools may exist earlier in the PATH)
      else
          fail
  else if the PATH has a directory that has all of the tools needed
      append that directory to the PATH  (but it's already in the PATH!)
  else
      fail

New code:

  if lit_tool_path is defined
      if lit_tool_path is an absolute directory
          if it contains all of the tools needed
              append lit_tool_path to the PATH  (other tools may exist earlier in the PATH)
          else if Git/usr/bin exists and contains all the tools needed
              append Git/usr/bin to the PATH  (other tools may exist earlier in the PATH, Git/usr/bin may already be in the PATH)
          else
              fail
      else if the PATH has a directory that has all of the tools needed
          append that directory to the PATH  (but it's already in the PATH!)
      else if Git/usr/bin exists and contains all the tools needed
          append Git/usr/bin to the PATH  (other tools may exist earlier in the PATH)
      else
          fail
  else if Git/usr/bin exists and contains all the tools needed
      append Git/usr/bin to the PATH  (other tools may exist earlier in the PATH)
  else
      fail

I'm not sure the first branch in the new code adds any value.

I have concerns about both the old and the new code:

1. We often add a redundant directory to the path.
2. We append to PATH rather than prepend, which is both right and wrong.  It's right in that we don't pre-empt other programs, but it's wrong in that we don't guarantee that we get the versions of the tools we want.
3. We require all of the tools to come from a single directory.  If grep.exe exists in one place in the path but diff.exe is in another, it's a failure.  Furthermore, given #2, if we find them both in the same directory, that doesn't mean both (or either) will be invoked from the same directory.

These larger concerns are outside the bounds of this patch, so I won't block on those.



================
Comment at: llvm/utils/lit/lit/llvm/config.py:25
             # For tests that require Windows to run.
             features.add('system-windows')
 
----------------
BTW, this looks redundant, as it's set below in a cascaded if starting about line 53.


================
Comment at: llvm/utils/lit/lit/llvm/config.py:27
 
             # Seek sane tools in directories and set to $PATH.
+            path = None
----------------
Not part of your change, but "sane" seems unnecessarily judgmental.  Perhaps "necessary" conveys the point.

And then maybe make a variable with the list of tools we need so that it's kind of documented in one place.  Something like:

`tools_needed = ['cmp.exe', 'grep.exe', 'sed.exe', 'diff.exe', 'echo.exe']`

And then we can pass that in to the couple of places we need it.


================
Comment at: llvm/utils/lit/lit/llvm/config.py:34
+                                                    ['cmp.exe', 'grep.exe', 'sed.exe',
+                                                    'diff.exe', 'echo.exe'])
+            if path is None:
----------------
Extending the list of tools to check for is great.  This would be the first place to use `tools_needed` as I mentioned in the previous comment.


================
Comment at: llvm/utils/lit/lit/llvm/config.py:36
+            if path is None:
+                path = self._find_git_windows_unix_tools()
             if path is not None:
----------------
You could also pass `tools_needed` here.


================
Comment at: llvm/utils/lit/lit/llvm/config.py:130
+        # Search both the 64 and 32-bit hives, as well as HKLM + HKCU
+        masks = [winreg.KEY_WOW64_32KEY, winreg.KEY_WOW64_64KEY]
+        hives = [winreg.HKEY_LOCAL_MACHINE, winreg.HKEY_CURRENT_USER]
----------------
I wonder if it's necessary to search both WOW64 hives explicitly.

* If lit is a 64-bit process (on 64-bit Windows), then a regular search of HKCU or HKLM would find Git, regardless Git's bitness--it wouldn't have to make two distinct searches.  (And, if both Gits were installed, you'd get the 64-bit one with a regular search, but the 32-bit one with this explicit technique.)

* If lit is a 32-bit process on 32-bit Windows, you're screwed because the WOW nodes won't exist.  Unless the lookup ignores those bits...?

* If lit is a 32-bit process on 64-bit Windows, a regular search wouldn't find a 64-bit Git installation.  That's the only case this solves.

Registry searches can take a long time.  So let's say we want to optimize for the case where Git is installed; if it isn't installed, we're going to fail anyway because we only got here because the tools don't exist elsewhere.

So I think I'd use `masks = [0, winreg.KEY_WOW64_64]`.  In the common case, we'll find Git on the first try, regardless of its bitness.  In the less-common case, we'll make a last ditch attempt which may end up repeating part of a search we've already done.


================
Comment at: llvm/utils/lit/lit/llvm/config.py:131
+        masks = [winreg.KEY_WOW64_32KEY, winreg.KEY_WOW64_64KEY]
+        hives = [winreg.HKEY_LOCAL_MACHINE, winreg.HKEY_CURRENT_USER]
+        for mask, hive in itertools.product(masks, hives):
----------------
I'd probably switch these to check HKCU first, as that's the one that would usually take precedence, and it's also the one the user has easier control over (if they decide to install Git).


================
Comment at: llvm/utils/lit/lit/llvm/config.py:135
+                with winreg.OpenKey(hive, r"SOFTWARE\GitForWindows", access=winreg.KEY_READ | mask) as key:
+                    try:
+                        install_root, _ = winreg.QueryValueEx(key, 'InstallPath')
----------------
Is the inner try-block necessary?  If the query below fails without the inner try block, wouldn't the exception just propagate to the outer `except: continue`?


================
Comment at: llvm/utils/lit/lit/llvm/config.py:146
+                    if any(not os.path.exists(x) for x in files):
+                        continue
+
----------------
There's already a function that does this part in util.py.  I think all you need to do is:

```
if not lit.util.checkToolsPath(candidate_path, tools_needed):
    continue
```

(assuming `tools_needed` was passed in to this function as I suggested earlier).


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

https://reviews.llvm.org/D84380