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

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160816/8151ac98/attachment.html>

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/1a09b406/attachment.html>

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>

Yes, the lit internal shell already interprets several shell commands by
itself, such as env. These are the processes meant.


2016-08-16 21:32 GMT+03:00 Robinson, Paul <paul.robinson at sony.com>:
> You can't get rid of process creation entirely (the software-under-test
> obviously needs to be in its own processes) but if we go toward lit
> interpreting more/all shell-like commands natively, instead of running an
> external program, that helps a lot.
>
> --paulr
>
>
>
> *From:* llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] *On Behalf Of
*Zachary
> Turner via llvm-dev
> *Sent:* Tuesday, August 16, 2016 11:24 AM
> *To:* Yaron Keren
> *Cc:* Zachary Turner via llvm-dev
> *Subject:* Re: [llvm-dev] 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/e0fec73d/attachment.html>

> The real problem is that file deletion is racy on Windows.
That's actually not strictly accurate. I filed a bug about this a while
ago: https://llvm.org/bugs/show_bug.cgi?id=27386

The reasons are really complex/subtle, and you can see more at the bug
tracker, or: Niall Douglas "Racing The File System"
https://youtu.be/uhRWMGBjlO8 ...I haven't gotten to writing the actual
patch yet. If anybody wants to pick it up, go right ahead.

Note: sent twice because I am unreasonably bad at mailing lists, and
replied wrong on the first try.

On Aug 16, 2016 2:16 PM, "Zachary Turner via llvm-dev" <
llvm-dev at lists.llvm.org> wrote:
> 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/20160817/c3b64e60/attachment.html>