[libcxx-commits] [PATCH] D78200: [libc++] [test] Generate static_test_env on the fly

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

In D78200#2022470 <https://reviews.llvm.org/D78200#2022470>, @broadwaylamb wrote:

> I'm sorry, I've messed up. I first committed this as rG645ad5badbabdeca31de5c98ea8135c5a6e7d710 <https://reviews.llvm.org/rG645ad5badbabdeca31de5c98ea8135c5a6e7d710>, but then realized that I've committed the previous version of this patch. So I reverted it and recommitted as 52cc8bac7780dbfb90dcc8cfe24696618eeaa06e <https://reviews.llvm.org/rG52cc8bac7780dbfb90dcc8cfe24696618eeaa06e>.
>
> In D78200#2021841 <https://reviews.llvm.org/D78200#2021841>, @EricWF wrote:
>
> > That said, I don't understand how this patch makes these tests pass on Windows.
> >  If Windows doesn't support symlinks, how does dynamically creating them make a difference?
>
>
> This patch is not about making them pass on Windows, but rather about making them pass on Linux in the case when we cross-compile them on Windows.


In that case, would it not be sufficient to have the source tree checked out on the target system, and specify that location while compiling on the host?
I don't see why a change of this size is needed to solve that particular problem.

> 
> 
>> Because this change unknowingly broke tests, I'm going to revert it.
>>  We can recommit it after ensuring there are no other accidental changes.
> 
> Could you please point me to the test failures? I didn't receive any e-mails.

The test I left that comment on was broken. It still passed, but it changed meaning.
It was checking that `exists` on a broken symlink returned false, but your change
turned a broken symlink into a file that didn't exist. So the test continued to pass,
but for the wrong reasons.

> 
> 
>> Unless I'm mistaken, this change is racy. Tests in different files are run in parallel.
>>  One test could be tearing down the static environment while another in trying to construct it.
> 
> This is not the case as of 52cc8bac7780dbfb90dcc8cfe24696618eeaa06e <https://reviews.llvm.org/rG52cc8bac7780dbfb90dcc8cfe24696618eeaa06e>. Each RAII object creates a new temporary directory using mktemps (more precisely, it uses `scoped_test_env` under the hood).

I don't see how that's possible since the static environment was still being referenced via the `StaticEnv` globals, but maybe I didn't look hard enough.

> Again, my apologies for messing up the diff here.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78200/new/

https://reviews.llvm.org/D78200