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

Mon Aug 3 10:23:16 PDT 2020

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

I have a couple responses, but nothing worth blocking this path for.  LGTM, and thanks for doing this!

I'm already working on an update to the LLVM/Windows developer docs to pull the GnuWin requirement along with some other bits.

In D84380#2191018 <https://reviews.llvm.org/D84380#2191018>, @zturner wrote:

>> I'm not sure the first branch in the new code adds any value.
>
> The first branch is important because `lit_tools_path` is up to the individual local config to define.  In other words, if the user does not specifically write `config.lit_tools_path = Foo/Bar` then this will be undefined and they'll get strange errors.

Ah, I didn't realize that was a bug in the old code.

If there is no `config.lit_tools_dir` (or if it isn't a directory that contains the tools), then the new code is going to skip checking PATH and just go searching for the Git tools.  I guess that's fine, but it might be surprising to someone who still has GnuWin in their path.

================
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):
----------------
zturner wrote:
> amccarth wrote:
> > 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).
> I've left this as is.  On my local installation, which I installed using all defaults and without explicitly trying to elevate when double clicking the installer, It's under HKLM.  
tl;dr:  Not a big deal, since this would be rare and unusual.

In the rare case that one version is installed for all users (HKLM), but another version is installed for this user (HKCU), then ideally the HKCU version would be selected.  Searching HKLM won't.

(If you managed to install to HKLM, then the installer process was running as Administrator.  Maybe you kicked it off from an already-elevated command console.)

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

https://reviews.llvm.org/D84380