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

[Zachary Turner via Phabricator via llvm-commits]

zturner marked 7 inline comments as done.
zturner added a comment.

> 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.  In all LLVM cases, the configs do this, and this knowledge has been propagated across all LLVM lit configs by way of copy/paste.  But it's a silent / undocumented requirement that caused me some confusion until I tracked it down.  So it seems better to be resilient in the case of it not being there.

> We often add a redundant directory to the path.

Internally, I believe the method to add a new directory to path will de-duplicate if it's already there.

> 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.

Despite being called `append`, it actually *does* prepend.  I agree this is confusing.

> 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.

This part is definitely true.



================
Comment at: llvm/utils/lit/lit/llvm/config.py:127
+        assert(sys.platform == 'win32')
+        import winreg
+
----------------
akhuang wrote:
> It seems `import winreg` doesn't work in python 2.7
It's apparently called `_winreg` in Python 2.7.  I've fixed this.


================
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):
----------------
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.  


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

https://reviews.llvm.org/D84380