<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">Sorry for the late reply, this is what happens when you let email pile up and read newest-first...</div><div class="gmail_quote"><br></div><div class="gmail_quote">On Mon, Apr 3, 2017 at 11:56 AM, Rafael Espíndola via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 31 March 2017 at 20:36, Reid Kleckner via Phabricator<br>
<span class=""><<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br>
> rnk added a comment.<br>
><br>
> chdir isn't thread safe, and lit still supports running the tests with threads.<br>
<br>
</span>Oh, I missed this. I thought we were always using multiprocessing these days.<br>
<span class=""><br>
> It looks like there are portability issues with requiring multiprocessing everywhere, so I think this change would require conditionally launching a child python process to implement the internal shell. That also kind of sucks, though, since python process startup isn't fast.<br>
<br>
</span>Is the multiprocess module still broken on those platforms? I just<br>
tried deleting all the logic around<br>
<br>
-        if jobs != 1 and use_processes and multiprocessing:<br>
-            try:<br>
<br>
so that lit always uses multiprocess and all tests pass on freebsd.<br>
<br>
Even if we can't use the multiprocess module, we could still use a<br>
process pool, no? Tests using subprocess would still fork a new<br>
python, but that is not the common case.</blockquote><div><br></div><div>I agree, let's try to do this. I could easily imagine putting a try/finally around test execution that resets cwd, os.environ, and anything else we support setting in the shell once we commit to multiprocessing. I'm still testing that multiprocessing.Pool patch, but hopefully that'll stick soon.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> I think you're right, if we want to implement subshells and ulimit, we need to create a process and manipulate its environment, cwd, etc. What we have stops just short of that, since I've basically assumed that we will never implement shell variable expansion or subshells. I'd be happy if lit could do more basic environment variable manipulation (export), but subshells felt like a bridge too far. I figured tests that needed this stuff could call bash directly and have a `REQUIRES: bash` line or something.<br>
<br>
</span>There is REQUIRES: shell already. We would like to at least be able to<br>
run the posix tests on windows. I was able to do it, except for the<br>
use of ulimit in<br>
compiler-rt/test/asan/<wbr>TestCases/Posix/deep_call_<wbr>stack.cc, which is ok<br>
for us since we don't have an direct ulimit equivalent and just have<br>
to take our chances with stuck bots.</blockquote><div><br></div><div>I was imagining that the 'shell' and 'bash' requirements mean different things. 'shell' means this today: these RUN lines only work in a real shell implementation. 'bash' would mean: the bash executable must be available on PATH, and this test will use it to do fancy shell things that lit doesn't understand.</div><div><br></div><div>It's actually very common on Windows for bash to be available from MSys, git for Windows, or whatever, but it's slow, so I'd rather use the internal shell. And if it's not available, we won't get test coverage for some small subset of tests.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> Do you think we can reasonably implement variable expansion? The last time I looked at it, it seemed too hard, and I worked around the problem by adding more lit substitutions. Maybe it would be OK if we just implemented the ${} expansion syntax?<br>
<br>
</span>What were the problems with it? I tried implementing export and it<br>
seems to work, but it didn't go very far since it hit "export PATH=<br>
...:$PATH".<br></blockquote><div><br></div><div>I don't remember exactly. I think the trouble was just that we currently parse the command line up front before executing it, but a real shell will parse one command, run it, substitute variables into the next command, parse it, and then run it. That felt like a big model change to me at the time, but we could implement it the same way we implement globbing, where we make a special token that gets expanded late.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
So, how about:<br>
<br>
* Put this change on hold.<br>
* Try using the multiprocess module everywhere to see if it is still broken.<br>
* Remove some uses of subprocesses.<br>
* Implement export with the current setup.<br>
* Try to implement variable expansion with the current setup.<br>
* Come back to this revision if we find out we can use multiple process.<br></blockquote><div><br></div><div>Sounds good to me. </div></div></div></div>