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

Wed May 6 05:20:33 PDT 2020

broadwaylamb marked an inline comment as done.
broadwaylamb added a comment.

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.

> 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.

> 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).

Again, my apologies for messing up the diff here.

================
Comment at: libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.is_directory/is_directory.pass.cpp:66
 {
     const path p = StaticEnv::DNE;
     TEST_CHECK(is_directory(p) == false);
----------------
EricWF wrote:
> This test is no longer correct.
> I assume this change broke or changed the meaning of other tests as well.
> 
> If we're creating the static test env using a RAII guard,
> that RAII guard should be the only way to name the members of the static environment
> and the old `StaticEnv::Foo` variables should be removed.
> 
Now it should be fine.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78200