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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I kind of think that we should do away with the static test env entirely, and always create it on the fly when we need it. It seems like this would be cleaner, and if we made `.Root` a member variable of `static_test_env`, it would be impossible to forget to create it. Right now, it's not really obvious how the static test env is created, and with this patch it's possible to forget to create it properly altogether.

My preference would be that `static_test_env` sets up the whole static test environment in the current directory, and that tests that need it create the environment as a local variable. We could also remove it from `// FILE_DEPENDENCIES` lines in the relevant tests. WDYT?



================
Comment at: libcxx/test/support/filesystem_test_helper.h:135-142
+/// We do it here instead of storing them in the repo to be
+/// cross-toolchain-friendly. The primary use case for this are Windows-hosted
+/// cross-toolchains.
+/// Windows doesn't really have a concept of symlinks. So, when the monorepo
+/// is cloned, those symlinks turn to ordinary text files.
+/// If we cross-compiled libc++ for some symlink-friendly system
+/// (e. g. Linux) and ran tests on the target system, some tests would fail.
----------------
I normally love comments, but I don't think the explanation part of why we're doing it this way now belongs in that comment. This is for the git history to tell instead.


================
Comment at: libcxx/test/support/filesystem_test_helper.h:146
+  static_test_env() {
+    for (const auto *link = std::begin(StaticEnv::SymlinkList);
+         link != std::end(StaticEnv::SymlinkList);
----------------
range-based for loop?


================
Comment at: libcxx/test/support/filesystem_test_helper.h:150
+      int ret = ::symlink(link->first.c_str(), link->second.c_str());
+      assert(ret == 0);
+    }
----------------
What about the bad symlink -- won't it fail here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78200