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

[Sergej Jaskiewicz via Phabricator via libcxx-commits]

broadwaylamb marked 2 inline comments as done.
broadwaylamb added a comment.

In D78200#2023484 <https://reviews.llvm.org/D78200#2023484>, @EricWF wrote:

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


Checking out the source tree on the target machine is not an option simply because the size of the monorepo checkout is close to the size of the remaining disk space on our board.

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

I've completely removed the `StaticEnv` namespace, so I'm pretty sure those globals are not referenced anymore.

> If we don't want the static env, we should remove it entirely.
>  Meaning, we should convert each of these tests to use dynamic_test_env and setup the required files like the remainder of the tests do.

Would you be OK with this solution?



================
Comment at: libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.current_path/current_path.pass.cpp:54
 {
-    const path new_path = StaticEnv::Dir;
+    CWDGuard guard;
+    static_test_env static_env;
----------------
EricWF wrote:
> You are now using `current_path` in the setup for the `current_path` tests.
> This is not OK. You cannot use the entity under test in said entities tests.
> 
> Please revert the patch.
Good point.

Would it be okay to use POSIX functions in the `CWDGuard` implementation instead?


================
Comment at: libcxx/test/support/filesystem_test_helper.h:306
+  fs::path OldCWD;
+  CWDGuard() : OldCWD(fs::current_path()) { }
+  ~CWDGuard() { fs::current_path(OldCWD); }
----------------
ldionne wrote:
> EricWF wrote:
> > I'm not OK with changing the tests working directory during the test.
> Some tests already do this. This is just moving `CwdGuard` from `canonical.pass.cpp` to this header so it can be reused.
I'm not sure I understand what you mean. Some tests that test the `current_path()` function would inevitably change the working directory, it's just that we want to set it back so they don't affect other tests.

And if we're going to use dynamic_test_env, this will still be the case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78200