[libc++][lit] Allow overriding the executor for tests

[arichardson]

This is useful when trying to run multiple tests with different
arguments to the executor script. This is needed in my case since I
do not know the correct ssh connection arguments when building libc++.
The testing script I have spawns multiple QEMU instances that listen on
a given port on localhost and runs lit with the --num-shards/--run-shard
argument. In order to connect each shard to the right QEMU instances I
need to customize the arguments passed to ssh.py (--extra-ssh-args and
--extra-scp-args) but can't do this at configure time since the target
port is only known when running the tests but not when calling CMake.
This change allows me to pass executor=ssh.py <args> to lit once I know
the right hostname/port for running the tests.

This also deprecates the LIB{CXX,CXXABI,UNWIND}_EXECUTOR CMake variable
as the same can be achieved by adding executor=... to the
LIB{CXX,CXXABI,UNWIND}_TEST_PARAMS variable.

[llvmbot]

@llvm/pr-subscribers-libunwind
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-libcxxabi

@llvm/pr-subscribers-libcxx

Changes

This is useful when trying to run multiple tests with different arguments to the executor script. This is needed in my case since I do not know the correct ssh connection arguments when building libc++. The testing script I have spawns multiple QEMU instances that listen on a given port on localhost and runs lit with the --num-shards/--run-shard argument. In order to connect each shard to the right QEMU instances I need to customize the arguments passed to ssh.py (--extra-ssh-args and --extra-scp-args) but can't do this at configure time since the target port is only known when running the tests but not when calling CMake. This change allows me to pass -Dexecutor=ssh.py <args> to lit once I know the right hostname/port for running the tests.

Full diff: https://github.com/llvm/llvm-project/pull/66545.diff

2 Files Affected:

• (modified) libcxx/utils/libcxx/test/dsl.py (+23)
• (modified) libcxx/utils/libcxx/test/params.py (+9)

diff --git a/libcxx/utils/libcxx/test/dsl.py b/libcxx/utils/libcxx/test/dsl.py
index 847cebf5962f6aa..778c15bf881175c 100644
--- a/libcxx/utils/libcxx/test/dsl.py
+++ b/libcxx/utils/libcxx/test/dsl.py
@@ -581,6 +581,29 @@ def pretty(self, config, litParams):
         return "add substitution {} = {}".format(self._key, self._getSub(config))
 
 
+class OverrideSubstitution(AddSubstitution):
+    """
+    This action replaces the given substitution in the Lit configuration.
+
+    The substitution can be a string or a callable, in which case it is called
+    with the configuration to produce the actual substitution (as a string).
+    """
+
+    def applyTo(self, config):
+        key = self._key
+        sub = self._getSub(config)
+        for i, val in enumerate(config.substitutions):
+            if val[0] == key:
+                config.substitutions[i] = (key, sub)
+                return
+        raise ValueError(
+            "Expected substitution {} to already be defined".format(key)
+        )
+
+    def pretty(self, config, litParams):
+        return "replace substitution {} = {}".format(self._key, self._getSub(config))
+
+
 class Feature(object):
     """
     Represents a Lit available feature that is enabled whenever it is supported.
diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index e05c6689964c734..dfe6fc7d11e804d 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -330,5 +330,14 @@ def getModuleFlag(cfg, enable_modules):
             AddCompileFlag("-D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES"),
         ],
     ),
+    Parameter(
+        name="executor",
+        type=str,
+        default="",
+        help="Custom executor to use instead of the configured default.",
+        actions=lambda executor: [] if not executor else [
+          OverrideSubstitution("%{executor}", executor)
+        ],
+    )
 ]
 # fmt: on

[EricWF]

Looks fine to me. I would like @ldionne approval though.

[ldionne]

I think this makes a lot of sense! I have a few suggestions but I definitely like the direction.

[ldionne]

I really like this! Minor nitpick in params.py but this basically LGTM.

[ldionne]

There has to be an executor, so I think we should change to

actions=lambda executor: [AddSubstitution("%{executor}", executor)]

i.e. drop the if.

[arichardson]

Thanks! Will merge once CI completes successfully.

[vvereschaka]

@arichardson

here is a problem with your changes when building the libraries on the windows cross toolchain builders

• https://lab.llvm.org/buildbot/#/builders/60/builds/14086
• https://lab.llvm.org/buildbot/#/builders/119/builds/15283

https://lab.llvm.org/buildbot/#/builders/60/builds/14086/steps/12/logs/stdio

llvm-lit.py: C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\utils\lit\lit\TestingConfig.py:151: fatal: unable to parse config file 'C:/buildbot/as-builder-1/x-armv7l/build/runtimes/runtimes-armv7-unknown-linux-gnueabihf-bins/libunwind/test/cmake-bridge.cfg', traceback: Traceback (most recent call last):
  File "C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\utils\lit\lit\TestingConfig.py", line 139, in load_from_path
    exec(compile(data, path, "exec"), cfg_globals, None)
  File "C:/buildbot/as-builder-1/x-armv7l/build/runtimes/runtimes-armv7-unknown-linux-gnueabihf-bins/libunwind/test/cmake-bridge.cfg", line 7
    config. 'executor="C:/Python310/python.exe" "C:/buildbot/as-builder-1/x-armv7l/llvm-project/libcxx/utils/ssh.py" --host = "[email protected]'"
                                                                                                                                                           ^
SyntaxError: unterminated string literal (detected at line 7)

would you take care of it?

UPDATE: Just let me know if you will need to check you changes before commit. I will test them on the builder locally.

[arichardson]

@arichardson

here is a problem with your changes when building the libraries on the windows cross toolchain builders

• https://lab.llvm.org/buildbot/#/builders/60/builds/14086
• https://lab.llvm.org/buildbot/#/builders/119/builds/15283

https://lab.llvm.org/buildbot/#/builders/60/builds/14086/steps/12/logs/stdio

llvm-lit.py: C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\utils\lit\lit\TestingConfig.py:151: fatal: unable to parse config file 'C:/buildbot/as-builder-1/x-armv7l/build/runtimes/runtimes-armv7-unknown-linux-gnueabihf-bins/libunwind/test/cmake-bridge.cfg', traceback: Traceback (most recent call last):
  File "C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\utils\lit\lit\TestingConfig.py", line 139, in load_from_path
    exec(compile(data, path, "exec"), cfg_globals, None)
  File "C:/buildbot/as-builder-1/x-armv7l/build/runtimes/runtimes-armv7-unknown-linux-gnueabihf-bins/libunwind/test/cmake-bridge.cfg", line 7
    config. 'executor="C:/Python310/python.exe" "C:/buildbot/as-builder-1/x-armv7l/llvm-project/libcxx/utils/ssh.py" --host = "[email protected]'"
                                                                                                                                                           ^
SyntaxError: unterminated string literal (detected at line 7)

would you take care of it?

UPDATE: Just let me know if you will need to check you changes before commit. I will test them on the builder locally.

Sorry about that! It looks like the cmake file quoting needs to be updated. Alternatively I could revert the part of this change that deprecates the executor variable. I will take a look at this first thing tomorrow morning. If it's more urgent, feel free to revert the change.

[ldionne]

@arichardson

Sorry about that! It looks like the cmake file quoting needs to be updated. Alternatively I could revert the part of this change that deprecates the executor variable.

Let's not un-deprecate the executor variables -- it's really great that we're removing some LIBCXX_FOO variables from the CMake build if they are only used for testing.

[arichardson]

@arichardson

here is a problem with your changes when building the libraries on the windows cross toolchain builders

• https://lab.llvm.org/buildbot/#/builders/60/builds/14086
• https://lab.llvm.org/buildbot/#/builders/119/builds/15283

https://lab.llvm.org/buildbot/#/builders/60/builds/14086/steps/12/logs/stdio

llvm-lit.py: C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\utils\lit\lit\TestingConfig.py:151: fatal: unable to parse config file 'C:/buildbot/as-builder-1/x-armv7l/build/runtimes/runtimes-armv7-unknown-linux-gnueabihf-bins/libunwind/test/cmake-bridge.cfg', traceback: Traceback (most recent call last):
  File "C:\buildbot\as-builder-1\x-armv7l\llvm-project\llvm\utils\lit\lit\TestingConfig.py", line 139, in load_from_path
    exec(compile(data, path, "exec"), cfg_globals, None)
  File "C:/buildbot/as-builder-1/x-armv7l/build/runtimes/runtimes-armv7-unknown-linux-gnueabihf-bins/libunwind/test/cmake-bridge.cfg", line 7
    config. 'executor="C:/Python310/python.exe" "C:/buildbot/as-builder-1/x-armv7l/llvm-project/libcxx/utils/ssh.py" --host = "[email protected]'"
                                                                                                                                                           ^
SyntaxError: unterminated string literal (detected at line 7)

would you take care of it?

UPDATE: Just let me know if you will need to check you changes before commit. I will test them on the builder locally.

@vvereschaka Could you try the following patch? It works in my local testing but I don't have the right setup for you toolchain. The problem appears to be that cmake always uses greedy regexes, so I've changed the split logic to instead find the first =.

diff --git a/libcxx/test/CMakeLists.txt b/libcxx/test/CMakeLists.txt
index fd194a36a28f..1416a8343df7 100644
--- a/libcxx/test/CMakeLists.txt
+++ b/libcxx/test/CMakeLists.txt
@@ -39,8 +39,10 @@ if (LLVM_USE_SANITIZER)
 endif()
 
 foreach(param IN LISTS LIBCXX_TEST_PARAMS)
-  string(REGEX REPLACE "(.+)=(.+)" "\\1" name "${param}")
-  string(REGEX REPLACE "(.+)=(.+)" "\\2" value "${param}")
+  string(FIND "${param}" "=" _eq_index)
+  string(SUBSTRING "${param}" 0 ${_eq_index} name)
+  string(SUBSTRING "${param}" ${_eq_index} -1 value)
+  string(SUBSTRING "${value}" 1 -1 value) # strip the leading =
   serialize_lit_param("${name}" "\"${value}\"")
 endforeach()
 
diff --git a/libcxxabi/test/CMakeLists.txt b/libcxxabi/test/CMakeLists.txt
index b65ac3a9d164..1baddf9ab305 100644
--- a/libcxxabi/test/CMakeLists.txt
+++ b/libcxxabi/test/CMakeLists.txt
@@ -53,8 +53,10 @@ else()
 endif()
 
 foreach(param IN LISTS LIBCXXABI_TEST_PARAMS)
-  string(REGEX REPLACE "(.+)=(.+)" "\\1" name "${param}")
-  string(REGEX REPLACE "(.+)=(.+)" "\\2" value "${param}")
+  string(FIND "${param}" "=" _eq_index)
+  string(SUBSTRING "${param}" 0 ${_eq_index} name)
+  string(SUBSTRING "${param}" ${_eq_index} -1 value)
+  string(SUBSTRING "${value}" 1 -1 value) # strip the leading =
   serialize_lit_param("${name}" "\"${value}\"")
 endforeach()
 
diff --git a/libunwind/test/CMakeLists.txt b/libunwind/test/CMakeLists.txt
index 084da0ab8f35..9d128720968c 100644
--- a/libunwind/test/CMakeLists.txt
+++ b/libunwind/test/CMakeLists.txt
@@ -36,8 +36,10 @@ else()
 endif()
 
 foreach(param IN LISTS LIBUNWIND_TEST_PARAMS)
-  string(REGEX REPLACE "(.+)=(.+)" "\\1" name "${param}")
-  string(REGEX REPLACE "(.+)=(.+)" "\\2" value "${param}")
+  string(FIND "${param}" "=" _eq_index)
+  string(SUBSTRING "${param}" 0 ${_eq_index} name)
+  string(SUBSTRING "${param}" ${_eq_index} -1 value)
+  string(SUBSTRING "${value}" 1 -1 value) # strip the leading =
   serialize_lit_param("${name}" "\"${value}\"")
 endforeach()

[vvereschaka]

executorfix.diff.txt

@arichardson ,

here is few updates for your fix (please see attached file):

• added an escaping of quotes for the serialized lit parameter. There could be an executor string something like that "C:/Python310/python.exe" "C:/buildbot/temp/llvm-project/libcxx/utils/ssh.py" [email protected]. We need to replace " with \" because it goes into the python's string.
• the test parameters are processing as a cmake list. We need to properly prepare xxx_LIBxxx_TEST_PARAMS variables in the cmake cache file.

The libunwind and libc++abi tests are passed successfully with these changes. I have started the libc++ tests. It will take about 1 hour, but they already started successfully. These changes are working properly on the win cross toolchain builders.