llvm dev - Aug 2016 - RFC: A cross platform way of using shell commands in lit tests

Unfortunately the proposal here doesn't get rid of process creation, it
just changes the process being created from a GnuWin32 one to one that we
write ourselves.  Happy to entertain suggestions for how to get rid of
process creation entirely, but it doesn't seem simple to me.

On Tue, Aug 16, 2016 at 11:20 AM Yaron Keren <yaron.keren at gmail.com>
wrote:
> You can use msys2 rather than gnuwin32.
> Nevertheless it would be great to get rid of this dependency, as well as
> speedup the regressions tests. Process creation is slower on Windows than
> Linux and so the tests run much slower.
>
>
> 2016-08-16 21:14 GMT+03:00 Zachary Turner via llvm-dev <
> llvm-dev at lists.llvm.org>:
>
>> That's more like a workaround though.  The real problem is that
file
>> deletion is racy on Windows.  I can't count the number of times
I've
>> encountered this error, only to find out that adding a retry fixed the
>> problem.  Sure, if you can remove opened files then there's no
problem to
>> begin with, but even if you can't, you can still remove the file by
>> retrying again 100ms later or so.  It seems silly to disable a test
which
>> could be testing useful functionality because of some quirk of an OS.
>>
>> If you had your own robust rm command, it can retry a few times,
allowing
>> the test to run everywhere.
>>
>> On Tue, Aug 16, 2016 at 11:07 AM Reid Kleckner via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> On Tue, Aug 16, 2016 at 10:09 AM, Greg Bedwell via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>> Anecdotally, most of the times I've seen people add
"REQUIRES: shell" on
>>>>> a test they really meant "disable on windows". I
think/hope that's
>>>>> usually temporary while they investigate problems, but it
might explain
>>>>> why so many tests say they require shell when they
don't.
>>>>>
>>>>>
>>>> In at least one case I can think of
>>>> ("clang/test/Format/style-on-command-line.cpp") the
"REQUIRES: shell" is
>>>> there to work around annoying intermittent failures on Windows
where 'rm'
>>>> was failing with permission denied errors just occasionally
enough to be a
>>>> problem.  I've definitely seen this happen in a number of
other places
>>>> previously although I can't think whether this affects any
other lit tests
>>>> off-hand, but I know that personally I've had to implement
'robust rm'
>>>> (basically, keep on trying until the error goes away on its
own) on Windows
>>>> in a number of different systems to work around this sort of
problem
>>>> before.  Replacing gnuwin32 with something where we could
easily implement
>>>> something like 'robust <cmd>' versions for
Windows when we encounter these
>>>> sorts of issues would be fantastic, so I'm definitely in
favour of removing
>>>> the dependency.
>>>>
>>>
>>> You can use "REQUIRES: can-remove-opened-file" to express
this
>>> requirement more precisely.
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160816/4688be2d/attachment-0001.html>

One other thing to note is that the dependency problem is going to go away
very soon just a result of moving to git. The normal Windows install of git
comes with all the utilities you need to run the LLVM tests anyway. I
haven't bothered to install gnuwin32 on my new machine.

I think we do want to rewrite a few key utilities, like 'rm', to make
them
reliable on Windows, but for most things like 'grep' and 'sed',
we should
use the standard things. There's no reason to want to have our own subtly
different versions of these tools.

If we do go all the way to avoid process creation, then it becomes
interesting to have our own version of these things, and at that point,
maybe having a fork or checked-in copy of busybox is the way to go.

On Tue, Aug 16, 2016 at 11:23 AM, Zachary Turner <zturner at google.com>
wrote:
> Unfortunately the proposal here doesn't get rid of process creation, it
> just changes the process being created from a GnuWin32 one to one that we
> write ourselves.  Happy to entertain suggestions for how to get rid of
> process creation entirely, but it doesn't seem simple to me.
>
> On Tue, Aug 16, 2016 at 11:20 AM Yaron Keren <yaron.keren at
gmail.com>
> wrote:
>
>> You can use msys2 rather than gnuwin32.
>> Nevertheless it would be great to get rid of this dependency, as well
as
>> speedup the regressions tests. Process creation is slower on Windows
than
>> Linux and so the tests run much slower.
>>
>>
>> 2016-08-16 21:14 GMT+03:00 Zachary Turner via llvm-dev <
>> llvm-dev at lists.llvm.org>:
>>
>>> That's more like a workaround though.  The real problem is that
file
>>> deletion is racy on Windows.  I can't count the number of times
I've
>>> encountered this error, only to find out that adding a retry fixed
the
>>> problem.  Sure, if you can remove opened files then there's no
problem to
>>> begin with, but even if you can't, you can still remove the
file by
>>> retrying again 100ms later or so.  It seems silly to disable a test
which
>>> could be testing useful functionality because of some quirk of an
OS.
>>>
>>> If you had your own robust rm command, it can retry a few times,
>>> allowing the test to run everywhere.
>>>
>>> On Tue, Aug 16, 2016 at 11:07 AM Reid Kleckner via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>> On Tue, Aug 16, 2016 at 10:09 AM, Greg Bedwell via llvm-dev
<
>>>> llvm-dev at lists.llvm.org> wrote:
>>>>
>>>>> Anecdotally, most of the times I've seen people add
"REQUIRES: shell"
>>>>>> on
>>>>>> a test they really meant "disable on
windows". I think/hope that's
>>>>>> usually temporary while they investigate problems, but
it might
>>>>>> explain
>>>>>> why so many tests say they require shell when they
don't.
>>>>>>
>>>>>>
>>>>> In at least one case I can think of
("clang/test/Format/style-on-command-line.cpp")
>>>>> the "REQUIRES: shell" is there to work around
annoying intermittent
>>>>> failures on Windows where 'rm' was failing with
permission denied errors
>>>>> just occasionally enough to be a problem.  I've
definitely seen this happen
>>>>> in a number of other places previously although I can't
think whether this
>>>>> affects any other lit tests off-hand, but I know that
personally I've had
>>>>> to implement 'robust rm' (basically, keep on trying
until the error goes
>>>>> away on its own) on Windows in a number of different
systems to work around
>>>>> this sort of problem before.  Replacing gnuwin32 with
something where we
>>>>> could easily implement something like 'robust
<cmd>' versions for Windows
>>>>> when we encounter these sorts of issues would be fantastic,
so I'm
>>>>> definitely in favour of removing the dependency.
>>>>>
>>>>
>>>> You can use "REQUIRES: can-remove-opened-file" to
express this
>>>> requirement more precisely.
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>
>>>
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160816/ee2bfe4f/attachment.html>

That's only if you use git bash, not cmd.

On Tue, Aug 16, 2016 at 11:38 AM Reid Kleckner <rnk at google.com> wrote:
> One other thing to note is that the dependency problem is going to go away
> very soon just a result of moving to git. The normal Windows install of git
> comes with all the utilities you need to run the LLVM tests anyway. I
> haven't bothered to install gnuwin32 on my new machine.
>
> I think we do want to rewrite a few key utilities, like 'rm', to
make them
> reliable on Windows, but for most things like 'grep' and
'sed', we should
> use the standard things. There's no reason to want to have our own
subtly
> different versions of these tools.
>
> If we do go all the way to avoid process creation, then it becomes
> interesting to have our own version of these things, and at that point,
> maybe having a fork or checked-in copy of busybox is the way to go.
>
> On Tue, Aug 16, 2016 at 11:23 AM, Zachary Turner <zturner at
google.com>
> wrote:
>
>> Unfortunately the proposal here doesn't get rid of process
creation, it
>> just changes the process being created from a GnuWin32 one to one that
we
>> write ourselves.  Happy to entertain suggestions for how to get rid of
>> process creation entirely, but it doesn't seem simple to me.
>>
>> On Tue, Aug 16, 2016 at 11:20 AM Yaron Keren <yaron.keren at
gmail.com>
>> wrote:
>>
>>> You can use msys2 rather than gnuwin32.
>>> Nevertheless it would be great to get rid of this dependency, as
well as
>>> speedup the regressions tests. Process creation is slower on
Windows than
>>> Linux and so the tests run much slower.
>>>
>>>
>>> 2016-08-16 21:14 GMT+03:00 Zachary Turner via llvm-dev <
>>> llvm-dev at lists.llvm.org>:
>>>
>>>> That's more like a workaround though.  The real problem is
that file
>>>> deletion is racy on Windows.  I can't count the number of
times I've
>>>> encountered this error, only to find out that adding a retry
fixed the
>>>> problem.  Sure, if you can remove opened files then there's
no problem to
>>>> begin with, but even if you can't, you can still remove the
file by
>>>> retrying again 100ms later or so.  It seems silly to disable a
test which
>>>> could be testing useful functionality because of some quirk of
an OS.
>>>>
>>>> If you had your own robust rm command, it can retry a few
times,
>>>> allowing the test to run everywhere.
>>>>
>>>> On Tue, Aug 16, 2016 at 11:07 AM Reid Kleckner via llvm-dev
<
>>>> llvm-dev at lists.llvm.org> wrote:
>>>>
>>>>> On Tue, Aug 16, 2016 at 10:09 AM, Greg Bedwell via llvm-dev
<
>>>>> llvm-dev at lists.llvm.org> wrote:
>>>>>
>>>>>> Anecdotally, most of the times I've seen people add
"REQUIRES: shell"
>>>>>>> on
>>>>>>> a test they really meant "disable on
windows". I think/hope that's
>>>>>>> usually temporary while they investigate problems,
but it might
>>>>>>> explain
>>>>>>> why so many tests say they require shell when they
don't.
>>>>>>>
>>>>>>>
>>>>>> In at least one case I can think of
>>>>>>
("clang/test/Format/style-on-command-line.cpp") the "REQUIRES:
shell" is
>>>>>> there to work around annoying intermittent failures on
Windows where 'rm'
>>>>>> was failing with permission denied errors just
occasionally enough to be a
>>>>>> problem.  I've definitely seen this happen in a
number of other places
>>>>>> previously although I can't think whether this
affects any other lit tests
>>>>>> off-hand, but I know that personally I've had to
implement 'robust rm'
>>>>>> (basically, keep on trying until the error goes away on
its own) on Windows
>>>>>> in a number of different systems to work around this
sort of problem
>>>>>> before.  Replacing gnuwin32 with something where we
could easily implement
>>>>>> something like 'robust <cmd>' versions
for Windows when we encounter these
>>>>>> sorts of issues would be fantastic, so I'm
definitely in favour of removing
>>>>>> the dependency.
>>>>>>
>>>>>
>>>>> You can use "REQUIRES: can-remove-opened-file" to
express this
>>>>> requirement more precisely.
>>>>> _______________________________________________
>>>>> LLVM Developers mailing list
>>>>> llvm-dev at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>>
>>>>
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>
>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160816/78df2cdc/attachment.html>