[PATCH] D31550: Let the OS take care of the current working directory

[Reid Kleckner via llvm-commits]

Sorry for the late reply, this is what happens when you let email pile up
and read newest-first...

On Mon, Apr 3, 2017 at 11:56 AM, Rafael Espíndola via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> On 31 March 2017 at 20:36, Reid Kleckner via Phabricator
> <reviews at reviews.llvm.org> wrote:
> > rnk added a comment.
> >
> > chdir isn't thread safe, and lit still supports running the tests with
> threads.
>
> Oh, I missed this. I thought we were always using multiprocessing these
> days.
>
> > 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.
>
> Is the multiprocess module still broken on those platforms? I just
> tried deleting all the logic around
>
> -        if jobs != 1 and use_processes and multiprocessing:
> -            try:
>
> so that lit always uses multiprocess and all tests pass on freebsd.
>
> Even if we can't use the multiprocess module, we could still use a
> process pool, no? Tests using subprocess would still fork a new
> python, but that is not the common case.


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.

> 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.
>
> There is REQUIRES: shell already. We would like to at least be able to
> run the posix tests on windows. I was able to do it, except for the
> use of ulimit in
> compiler-rt/test/asan/TestCases/Posix/deep_call_stack.cc, which is ok
> for us since we don't have an direct ulimit equivalent and just have
> to take our chances with stuck bots.


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.

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.

> 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?
>
> What were the problems with it? I tried implementing export and it
> seems to work, but it didn't go very far since it hit "export PATH=
> ...:$PATH".
>

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.


> So, how about:
>
> * Put this change on hold.
> * Try using the multiprocess module everywhere to see if it is still
> broken.
> * Remove some uses of subprocesses.
> * Implement export with the current setup.
> * Try to implement variable expansion with the current setup.
> * Come back to this revision if we find out we can use multiple process.
>

Sounds good to me.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170406/e311778e/attachment.html>