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

[Zachary Turner via Phabricator via llvm-commits]

zturner accepted this revision.
zturner added inline comments.


================
Comment at: lit/Suite/lldbtest.py:47
+        # of the command.
+        cmd = [sys.executable] + self.dotest_cmd + [testPath, '-p', testFile]
 
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D46020