[libcxx-commits] [PATCH] D93582: [libc++] [libc++abi] Correctly quote source and target names in tests. NFC.

[Saleem Abdulrasool via Phabricator via libcxx-commits]

compnerd requested changes to this revision.
compnerd added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/test/CMakeLists.txt:85
     "TargetInfo to use when setting up test environment.")
-set(LIBCXX_EXECUTOR "${Python3_EXECUTABLE} ${CMAKE_CURRENT_LIST_DIR}/../utils/run.py" CACHE STRING
+set(LIBCXX_EXECUTOR "${Python3_EXECUTABLE} '${CMAKE_CURRENT_LIST_DIR}/../utils/run.py'" CACHE STRING
     "Executor to use when running tests.")
----------------
I believe that this doesn't work properly on Windows.  The quoting rules on Windows are quite different.  Using `"` instead of `'` would work.  However, depending on how this is used, CMake should be able to handle the quoting appropriately per platform.


================
Comment at: libcxx/utils/libcxx/test/dsl.py:98
     out, err, exitCode, timeoutInfo = _executeScriptInternal(test, ['%{build}'])
-    _executeScriptInternal(test, ['rm %t.exe'])
+    _executeScriptInternal(test, ['rm "%t.exe"'])
     return exitCode == 0
----------------
Please use `pipes.quote` for the argument quoting.


================
Comment at: libcxx/utils/libcxx/test/format.py:73
     _checkBaseSubstitutions(substitutions)
-    substitutions.append(('%{build}', '%{cxx} %s %{flags} %{compile_flags} %{link_flags} -o %t.exe'))
-    substitutions.append(('%{run}', '%{exec} %t.exe'))
+    substitutions.append(('%{build}', '%{cxx} "%s" %{flags} %{compile_flags} %{link_flags} -o "%t.exe"'))
+    substitutions.append(('%{run}', '%{exec} "%t.exe"'))
----------------
Please use `pipes.quote` for the quoted parameters (and the same throughout the rest of the lit configuration).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93582