[PATCH] D46020: [lit, lldbsuite] Update the lldbsuite to correctly run tests on windows and windows server

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: lit/Suite/lldbtest.py:47
+        # of the command.
+        cmd = [sys.executable] + self.dotest_cmd + [testPath, '-p', testFile]
 
----------------
zturner wrote:
> stella.stamenova wrote:
> > zturner wrote:
> > > labath wrote:
> > > > Do we need to worry about picking the correct `python`/`python_d` for release vs. debug builds?
> > > If it's a debug build, `sys.executable` should already be `python_d.exe`.  So this should be correct.
> > > 
> > > My question though, is where does `self.dotest_cmd` come from?  Would it ever be the case that this already has the python executable?  Then you'd have an error because it would be specified twice.  
> > Currently, dotest_cmd is hardcoded in the lldb suite lit.cfg to point to dotest_path (+some arguments) and dotest_path itself is hardcoded in lldb-dotest.in to point to the dotest.py script.
> > 
> > I considered testing whether the command contained python before adding it, but since the command is currently hardcoded to point to dotest.py, it seemed wasteful.
> This seems fine to me then.  I have a mild preference for prepending `sys.executable` from inside of `lit.cfg` and then putting an assertion here that `sys.executable` is present as the first argument, but it's up to you, if you like that too you can do it, otherwise this is fine as is IMO.
> If it's a debug build, sys.executable should already be python_d.exe. So this should be correct.

I was worried that the python executable we use to run lit may not be the same one as we need to use for lldb testing in a specific build configuration (I remember there being some confusion about that in the past). But If you think it fine, then I'm ok as well. (it only matters for VS builds anyway).


Repository:
  rL LLVM

https://reviews.llvm.org/D46020