[libcxx-commits] [PATCH] D84040: [libcxx][lit] Fix incorrect lambda capture in hasLocale checks

[Alexander Richardson via Phabricator via libcxx-commits]

arichardson marked an inline comment as done.
arichardson added inline comments.


================
Comment at: libcxx/utils/libcxx/test/dsl.py:55
 
-def _makeConfigTest(config):
+def _makeConfigTest(config, testPrefix=None):
   sourceRoot = os.path.join(config.test_exec_root, '__config_src__')
----------------
bjope wrote:
> bjope wrote:
> > I get errors like this after this patch:
> > 
> > ```
> > llvm-lit: /repo/llvm-upstream/llvm/build-all-builtins/bin/../../utils/lit/lit/TestingConfig.py:101: fatal: unable to parse config file '/repo/llvm-upstream/llvm/build-all-builtins/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/libunwind/test/lit.site.cfg', traceback: Traceback (most recent call last):
> >   File "/repo/llvm-upstream/llvm/build-all-builtins/bin/../../utils/lit/lit/TestingConfig.py", line 88, in load_from_path
> >     exec(compile(data, path, 'exec'), cfg_globals, None)
> >   File "/repo/llvm-upstream/llvm/build-all-builtins/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/libunwind/test/lit.site.cfg", line 58, in <module>
> >     configuration.configure()
> >   File "/repo/llvm-upstream/libcxx/utils/libcxx/test/config.py", line 148, in configure
> >     self.lit_config
> >   File "/repo/llvm-upstream/libcxx/utils/libcxx/test/newconfig.py", line 20, in configure
> >     feature = param.getFeature(config, lit_config.params)
> >   File "/repo/llvm-upstream/libcxx/utils/libcxx/test/dsl.py", line 387, in getFeature
> >     value = self._parse(param) if param is not None else getDefault()
> >   File "/repo/llvm-upstream/libcxx/utils/libcxx/test/dsl.py", line 386, in <lambda>
> >     getDefault = lambda: self._default(config) if callable(self._default) else self._default
> >   File "/repo/llvm-upstream/libcxx/utils/libcxx/test/params.py", line 17, in <lambda>
> >     default=lambda cfg: next(s for s in reversed(_allStandards) if hasCompileFlag(cfg, '-std='+s)),
> >   File "/repo/llvm-upstream/libcxx/utils/libcxx/test/params.py", line 17, in <genexpr>
> >     default=lambda cfg: next(s for s in reversed(_allStandards) if hasCompileFlag(cfg, '-std='+s)),
> >   File "/repo/llvm-upstream/libcxx/utils/libcxx/test/dsl.py", line 125, in hasCompileFlag
> >     with _makeConfigTest(config) as test:
> >   File "/repo/llvm-upstream/libcxx/utils/libcxx/test/dsl.py", line 62, in _makeConfigTest
> >     prefix=testPrefix)
> >   File "/usr/lib64/python2.7/tempfile.py", line 458, in NamedTemporaryFile
> >     (fd, name) = _mkstemp_inner(dir, prefix, suffix, flags)
> >   File "/usr/lib64/python2.7/tempfile.py", line 237, in _mkstemp_inner
> >     file = _os.path.join(dir, pre + name + suf)
> > TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'
> > 
> > }
> > ```
> > 
> > So using prefix=None does not work, at least not on my system. But if I change this to `testPrefix=''` it seems to work better.
> Could this perhaps be related to using `/usr/lib64/python2.7/tempfile.py`.
> 
> No idea really why it picks tempfile.py from that place. I had expect it to use python 3.5.0, as earlier in the log I've got:
> 
> ```
> -- Found Python3: /app/vbuild/RHEL6-x86_64/python/3.5.0/bin/python3.5 (found version "3.5.0") found components: Interpreter
> ```
> as well as
> 
> ```
> cd /repo/llvm-upstream/clang/bindings/python && /app/cmake/3.16.4/bin/cmake -E env CLANG_LIBRARY_PATH=/repo/llvm-upstream/llvm/build-all-builtins/lib /app/vbuild/RHEL7-x86_64/python/3.5.0/bin/python3.5 -m unittest discover
> ```
> 
Maybe this is happening because llvm-lit defaults to using "python" which is usually python2. (https://github.com/llvm/llvm-project/blob/9b19400004dfee9d07a90aa11d448bade9ee71a2/llvm/utils/llvm-lit/llvm-lit.in#L1) rather than the binary that was detected at CMake time.

It appears to work if I use `python3 ./bin/llvm-lit projects/libcxx/test` but I get the same error with `./bin/llvm-lit projects/libcxx/test`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84040