[llvm] r295768 - [Support] Add a function to check if a file resides locally.

Wed Mar 8 00:49:10 PST 2017

I went ahead and committed r297260 which removes the test.

Thanks,

Jonas

From: Zachary Turner [mailto:zturner at google.com] 
Sent: Friday, March 03, 2017 5:57 PM
To: Simon Dardis; Hahnfeld, Jonas
Cc: llvm-commits at lists.llvm.org
Subject: Re: [llvm] r295768 - [Support] Add a function to check if a file resides locally.

Honestly i think it may just be better to delete the test. I knew this test had the chance of breaking when I originally added it but didn't think it would occur.

There are plenty of examples of untested functions for low level system functions whose results can't be predicted. Imagine getting the system page size or system temp directory for example. 

On Fri, Mar 3, 2017 at 8:49 AM Simon Dardis <Simon.Dardis at imgtec.com> wrote:

I was considering the option of using an environmental variable as well. In our case, half our
buildbots are configured to use remote storage due to the nature of the machines themselves.

I think the environment variable route is the only cross platform way to retain the test in a
useful fashion.

It would also help dealing with tests (in the future) which are by their nature sensitive to the
environment they're running it. Given the majority of buildbots (and developers I presume)
build and test in a 'regular' environment, we wouldn't lose test coverage for the small minority
with more exotic environments.

I do agree it's quite the sledgehammer approach.

Thanks,
Simon

From: Hahnfeld, Jonas [mailto:Hahnfeld at itc.rwth-aachen.de]
Sent: 03 March 2017 16:20
To: zturner at google.com
Cc: Simon Dardis; llvm-commits at lists.llvm.org
Subject: Re: [llvm] r295768 - [Support] Add a function to check if a file resides locally.

In the latter case, #1 doesn't work either. We could introduce an option or a variable in the environment to enable this specific test and set it on a few build bots, but that seems a bit overkill...

Am Freitag, den 03.03.2017, 15:27 +0000 schrieb Zachary Turner:
#2 would introduce enough additional file system code into the test that the code building up to the important part is more likely to fail than the important part.

I could also imagine someone installing their entire os on an NFS so perhaps there is just nothing we can do
On Thu, Mar 2, 2017 at 11:15 PM Hahnfeld, Jonas <Hahnfeld at itc.rwth-aachen.de> wrote:
Two more ideas after thinking about the problem a bit more:
1)      We could test with a file in $TMP and hope that it is local.
2)      On *nix systems, we could parse all mounted paths and blacklist certain filesystems, namely NFS to begin with.
I think both methods may be error prone in some cases. The only other solution is removing the test which I also quite don’t like that much.

From: Zachary Turner [mailto:zturner at google.com]
Sent: Friday, March 03, 2017 7:56 AM
To: Hahnfeld, Jonas
Cc: Simon Dardis; llvm-commits at lists.llvm.org
Subject: Re: [llvm] r295768 - [Support] Add a function to check if a file resides locally.

My only idea in that case is to just delete the test sadly :-/

I can't think of a way to reliably test this if we can't assume we're on a local filesystem
On Thu, Mar 2, 2017 at 2:04 AM Hahnfeld, Jonas <Hahnfeld at itc.rwth-aachen.de> wrote:
Hi Zachary,

This change essentially breaks tests when anyone is building on NFS as I do.
Simon XFAIL'd the test for the mips buildbots in r295840 but that leaves the
problem for anyone else.

Do you have a clever solution for that?

Regards,
Jonas

> -----Original Message-----
> From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On Behalf
> Of Zachary Turner via llvm-commits
> Sent: Tuesday, February 21, 2017 9:56 PM
> To: llvm-commits at lists.llvm.org
> Subject: [llvm] r295768 - [Support] Add a function to check if a file
> resides
> locally.
>
> Author: zturner
> Date: Tue Feb 21 14:55:47 2017
> New Revision: 295768
>
> URL: http://llvm.org/viewvc/llvm-project?rev=295768 <http://llvm.org/viewvc/llvm-project?rev=295768&view=rev> &view=rev
> Log:
> [Support] Add a function to check if a file resides locally.
>
> Differential Revision: https://reviews.llvm.org/D30010
>
> Modified:
>     llvm/trunk/include/llvm/Support/FileSystem.h
>     llvm/trunk/include/llvm/Support/MemoryBuffer.h
>     llvm/trunk/lib/Support/MemoryBuffer.cpp
>     llvm/trunk/lib/Support/Unix/Path.inc
>     llvm/trunk/lib/Support/Windows/Path.inc
>     llvm/trunk/unittests/Support/Path.cpp

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170308/d4b78564/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5868 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170308/d4b78564/attachment.bin>