[lit] Pass more environment variables through on Windows

[zturner]

Summary

This arose when I was trying to have a substitution which invoked a python script P, and that python script tried to invoke clang-cl (or even cl). Since we invoke P with a custom environment, it doesn't inherit the environment of the parent, and then when we go to invoke clang-cl, it's unable to find the MSVC installation directory. There were many more I could have passed through which are set by vcvarsall, but I tried to keep it simple and only pass through the important ones.

[rnk]

lgtm, seems like a simplification. I guess the behavior change is that now we don't have empty INCLUDE etc variables hanging around in the environment?

[zturner]

I don't think that ever actually happened, or at least I never observed it. The real behavior change is that we're now passing through "LIB", "VCINSTALLDIR", "VCToolsInstallDir", "VSINSTALLDIR", "WindowsSdkDir", and "WindowsSdkVersion", whereas before we were not propagating them into the child environment at all. clang-cl reads these, so it could have an observable impact on clang-cl (but any observable impact should be beneficial, since before it would actually just fail if a test tried to include a system header or link against a system library)

[thakis]

In D54963#1310190, @zturner wrote:

I don't think that ever actually happened, or at least I never observed it. The real behavior change is that we're now passing through "LIB", "VCINSTALLDIR", "VCToolsInstallDir", "VSINSTALLDIR", "WindowsSdkDir", and "WindowsSdkVersion", whereas before we were not propagating them into the child environment at all. clang-cl reads these, so it could have an observable impact on clang-cl (but any observable impact should be beneficial, since before it would actually just fail if a test tried to include a system header or link against a system library)

At least for clang tests, tests reading system libraries are usually a bug -- tests are supposed to be hermetic, cross-platform, etc. It sounds like now it's easier to accidentally use a system header?

[andrewng]

llvm/trunk/utils/lit/lit/TestingConfig.py

37

Was the change from PYTHONUNBUFFERED to PYTHONBUFFERED intentional? AFAIK PYTHONBUFFERED is not an environment variable recognised by Python.

llvm/trunk/utils/lit/lit/TestingConfig.py

def fromdefaults(litConfig):
             'LLVM_DISABLE_CRASH_REPORT' : '1',
             }
 
         pass_vars = ['LIBRARY_PATH', 'LD_LIBRARY_PATH', 'SYSTEMROOT', 'TERM',
                      'CLANG', 'LD_PRELOAD', 'ASAN_OPTIONS', 'UBSAN_OPTIONS',
                      'LSAN_OPTIONS', 'ADB', 'ANDROID_SERIAL',
                      'SANITIZER_IGNORE_CVE_2016_2143', 'TMPDIR', 'TMP', 'TEMP',
                      'TEMPDIR', 'AVRLIT_BOARD', 'AVRLIT_PORT',
-                     'FILECHECK_DUMP_INPUT_ON_FAILURE', 'FILECHECK_OPTS']
+                     'FILECHECK_DUMP_INPUT_ON_FAILURE', 'FILECHECK_OPTS',
+                     'VCINSTALLDIR', 'VCToolsinstallDir', 'VSINSTALLDIR',
+                     'WindowsSdkDir', 'WindowsSDKLibVersion']
+
+        if sys.platform == 'win32':
+            pass_vars.append('INCLUDE')
+            pass_vars.append('LIB')
+            pass_vars.append('PATHEXT')
+            environment['PYTHONBUFFERED'] = '1'
+
         for var in pass_vars:
             val = os.environ.get(var, '')
             # Check for empty string as some variables such as LD_PRELOAD cannot be empty
             # ('') for OS's such as OpenBSD.
             if val:
                 environment[var] = val
 
-        if sys.platform == 'win32':
-            environment.update({
-                    'INCLUDE' : os.environ.get('INCLUDE',''),
-                    'PATHEXT' : os.environ.get('PATHEXT',''),
-                    'PYTHONUNBUFFERED' : '1',
-                    'TEMP' : os.environ.get('TEMP',''),
-                    'TMP' : os.environ.get('TMP',''),
-                    })
-
         # Set the default available features based on the LitConfig.
         available_features = []
         if litConfig.useValgrind:
             available_features.append('valgrind')
             if litConfig.valgrindLeakCheck:
                 available_features.append('vg_leak')
 
         return TestingConfig(None,