[libcxx] [test] Query the target platform, not the host one

[broadwaylamb]

Summary

target_info is inferred to WindowsLocalTI on Windows hosts unless
specified otherwise. In the latter case, it doesn't make sense to use
Windows-specific settings if the target is not Windows.

This change should not break anything, because target_info is inferred
based on what platform.system() returns. self.is_windows was set based
on the same platform.system() call.

[ldionne]

libcxx/utils/libcxx/test/config.py

1207–1208

Are you sure this is right? This relates to the platform where the lit tests are going to run, which is the current platform and not necessarily the target, no? Of course, those two have to be compatible but they're not necessarily the same, are they?

[arichardson]

Looks good to me but I'm not sure about the PATH changes.

This patch reminds me that I have lots of local changes in the CHERI version of libc++ to improve running tests on a remote platform via SSH. I'll try to upload them soon.

[broadwaylamb]

[libcxx] [test] Use host system separators when setting PATH in config.py

[broadwaylamb]

In D68275#1690163, @arichardson wrote:

Looks good to me but I'm not sure about the PATH changes.

This patch reminds me that I have lots of local changes in the CHERI version of libc++ to improve running tests on a remote platform via SSH. I'll try to upload them soon.

I think it would be a huge help, because running libc++ tests on a remote target is exactly what I'm trying to accomplish.

I've addressed the PATH issue.

libcxx/utils/libcxx/test/config.py

1207–1208

I've just looked through the config.py file and it seems that this add_path method was only ever called on Windows.

But you're right, we probably should ask about the host here.

[broadwaylamb]

Use host system separators when setting PATH in config.py

[bcraig]

In D68275#1690334, @broadwaylamb wrote:

I think it would be a huge help, because running libc++ tests on a remote target is exactly what I'm trying to accomplish.

Be sure to investigate LIBCXX_EXECUTOR then. I've used that to run tests on a local QEMU-like emulator before. It seems like it would be useful for doing remote execution as well.

[broadwaylamb]

In D68275#1690421, @bcraig wrote:

Be sure to investigate LIBCXX_EXECUTOR then. I've used that to run tests on a local QEMU-like emulator before. It seems like it would be useful for doing remote execution as well.

I'm already using it. The problem is that my host system is Windows, and my target system is Linux, which is not the most well-trodden path here as far as I can see.

[ldionne]

This looks roughly okay to me, but I don't maintain the Windows side of things (I don't really run tests on Windows except when required to). I would wait for @EricWF or other folks that work on Windows more often to give you a thumbs up before committing this.

[broadwaylamb]

libcxx/utils/libcxx/test/config.py

1207–1208

After some thinking I came to realization that this PATH should relate to the target platform after all.

This add_path method is only ever called to modify the environment where the tests are executed. This is why the variable is called exec_env.

This exec_env is then passed to an Executor (for example, LocalExecutor or SSHExecutor). Seems like it's its only purpose.

We set the exec_env to dict(os.environ) in the __init__, but I think there has to be a better way, because right now when I'm ssh'ing from Windows to Linux, the Windows environment is leaking to the Linux environment, which is absolutely not what should happen.

Instead we probably should ask the Executor for the current environment, and then somehow merge it with self.exec_env. LocalExecutor will just call to os.environ, and SSHExecutor will do nothing.

[broadwaylamb]

Don't leak host environment when executing tests on a remote target.

The execution environment that is built in the Configuration object (I'm referring to self.exec_env in config.py) doesn't pick up the current host environment anymore, but rather is built from scratch, and is only merged with the current environment by an Executor right before running the test:

• The LocalExecutor just merges it with the host environment

• The SSHExecutor passes the execution environment to the target as-is, except for the PATH variable. We update the target PATH variable instead of overwriting it.

@EricWF can you please take a look at this?

[broadwaylamb]

Revert accidentally removed import in executor.py.

[broadwaylamb]

Still can't get used to Phabricator workflow, sorry for spam.

[broadwaylamb]

Can anyone please review this patch?

I tried to make it as nonintrusive as possible, however, it's not that simple to do when the infrastructure was not designed for cross-compilation setup in the first place.

[broadwaylamb]

libcxx/utils/libcxx/test/config.py

73

We're now looking at the host environemnt in the _merge_environments method of an Executor object (see executor.py). Only the local executor calls os.environ.

204

An executor object now knows more about its target, and can make proper adjustments based on it.

libcxx/utils/libcxx/test/executor.py

145

This is so that the path separators are handled correctly when we're connecting from Windows to Unix

[broadwaylamb]

This patch now depends on D69170, which should be committed first.

[broadwaylamb]

Update the patch now that D69170 has landed.

@ldionne @jroelofs @EricWF do you happen to have some time to review this?

[broadwaylamb]

@ldionne can you please take a look?

[ldionne]

This looks fine to me, but like I said I don't run the tests on Windows so I can't test locally. Since no one else seems objected, I'll commit this and folks can chime in if they're unhappy. Otherwise we're just blocking you for no good reason.

[ldionne]

libcxx/utils/libcxx/test/executor.py

11

Woops, it looks like this is a new dependency that wasn't there before (or maybe this is Python3 only)? Are you able to remove that dependency? It breaks running the tests on my system (so other systems too, probably):

from pathlib import PurePosixPath
ImportError: No module named pathlib

[broadwaylamb]

Don't use the pathlib module as it is Python 3 only. Use the posixpath and ntpath modules instead.

[broadwaylamb]

libcxx/utils/libcxx/test/executor.py

11

I've used posixpath instead, that should work with Python 2 now.

[ldionne]

When running the tests locally, I see errors that are probably a result of messing around with the environment:

clang-10: error: unable to execute command: Executable "ld" doesn't exist!
clang-10: error: linker command failed with exit code 1 (use -v to see invocation)

Have you tested this on Linux/MacOS by just running ninja -C <build> check-libcxx?

[broadwaylamb]

Fix ShTests

[broadwaylamb]

@ldionne should be fixed now.

[broadwaylamb]

@ldionne can you verify this?

[broadwaylamb]

@ldionne ping

[broadwaylamb]

@ldionne huge thanks for your time!

libcxx/utils/libcxx/test/config.py

def __init__(self, lit_config, config):
         self.project_obj_root = None
         self.libcxx_src_root = None
         self.libcxx_obj_root = None
         self.cxx_library_root = None
         self.cxx_runtime_root = None
         self.abi_library_root = None
         self.link_shared = self.get_lit_bool('enable_shared', default=True)
         self.debug_build = self.get_lit_bool('debug_build',   default=False)
-        self.exec_env = dict(os.environ)
+        self.exec_env = dict()
         self.use_target = False
         self.use_system_cxx_lib = False
         self.use_clang_verify = False
         self.long_tests = None
         self.execute_external = False
 
     def get_lit_conf(self, name, default=None):
         val = self.lit_config.params.get(name, None)
def make_static_lib_name(self, name):
         """Return the full filename for the specified library name"""
         if self.target_info.is_windows():
             assert name == 'c++'  # Only allow libc++ to use this function for now.
             return 'lib' + name + '.lib'
         else:
             return 'lib' + name + '.a'
 
     def configure(self):
+        self.configure_target_info()
         self.configure_executor()
         self.configure_use_system_cxx_lib()
-        self.configure_target_info()
         self.configure_cxx()
         self.configure_triple()
         self.configure_deployment()
         self.configure_src_root()
         self.configure_obj_root()
         self.configure_cxx_stdlib_under_test()
         self.configure_cxx_library_root()
         self.configure_use_clang_verify()
def configure_executor(self):
                 # that the user wants it at the end, but we have no
                 # way of getting at that easily.
                 selt.lit_config.fatal("Cannot infer how to create a Valgrind "
                                       " executor.")
         else:
             te = LocalExecutor()
             if self.lit_config.useValgrind:
                 te = ValgrindExecutor(self.lit_config.valgrindArgs, te)
+
+        te.target_info = self.target_info
+        
         self.executor = te
 
     def configure_target_info(self):
         self.target_info = make_target_info(self)
 
     def configure_cxx(self):
         # Gather various compiler parameters.
         cxx = self.get_lit_conf('cxx_under_test')
def configure_deployment(self):
                 self.config.available_features.add('dylib-has-no-filesystem')
                 self.lit_config.note("the deployment target does not support <filesystem>")
         else:
             self.cxx.flags += ['-D_LIBCPP_DISABLE_AVAILABILITY']
 
     def configure_env(self):
         self.target_info.configure_env(self.exec_env)
 
     def add_path(self, dest_env, new_path):
-        if 'PATH' not in dest_env:
-            dest_env['PATH'] = new_path
-        else:
-            split_char = ';' if platform.system() == 'Windows' else ':'
-            dest_env['PATH'] = '%s%s%s' % (new_path, split_char,
-                                           dest_env['PATH'])
+        self.target_info.add_path(dest_env, new_path)

libcxx/utils/libcxx/test/executor.py

 #===----------------------------------------------------------------------===##
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 #
 #===----------------------------------------------------------------------===##
 
-import platform
 import os
 
+from pathlib import PurePosixPath
+
 from libcxx.test import tracing
 from libcxx.util import executeCommand
 
-
 class Executor(object):
+    def __init__(self):
+        self.target_info = None
+
     def run(self, exe_path, cmd, local_cwd, file_deps=None, env=None):
         """Execute a command.
             Be very careful not to change shared state in this function.
             Executor objects are shared between python processes in `lit -jN`.
         Args:
             exe_path: str:    Local path to the executable to be run
             cmd: [str]:       subprocess.call style command
             local_cwd: str:   Local path to the working directory
             file_deps: [str]: Files required by the test
             env: {str: str}:  Environment variables to execute under
         Returns:
             cmd, out, err, exitCode
         """
         raise NotImplementedError
 
+    def _merge_environments(self, current_env, updated_env):
+        """Merges two execution environments.
+
+        If both environments contain the PATH variables, they are also merged
+        using the proper separator.
+        """
+        result_env = dict(current_env)
+        for k, v in updated_env.items():
+            if k == 'PATH' and self.target_info:
+                self.target_info.add_path(result_env, v)
+            else:
+                result_env[k] = v
+        return result_env
+
 
 class LocalExecutor(Executor):
     def __init__(self):
         super(LocalExecutor, self).__init__()
         self.is_windows = platform.system() == 'Windows'
 
     def run(self, exe_path, cmd=None, work_dir='.', file_deps=None, env=None):
         cmd = cmd or [exe_path]
         if work_dir == '.':
             work_dir = os.getcwd()
+
+        if env:
+            env = self._merge_environments(os.environ, env)
+
         out, err, rc = executeCommand(cmd, cwd=work_dir, env=env)
         return (cmd, out, err, rc)
 
 
 class PrefixExecutor(Executor):
     """Prefix an executor with some other command wrapper.
 
     Most useful for setting ulimits on commands, or running an emulator like
class TimeoutExecutor(PrefixExecutor):
     """
     def __init__(self, duration, chain):
         super(TimeoutExecutor, self).__init__(
             ['timeout', duration], chain)
 
 
 class RemoteExecutor(Executor):
     def __init__(self):
+        super(RemoteExecutor, self).__init__()
         self.local_run = executeCommand
 
     def remote_temp_dir(self):
         return self._remote_temp(True)
 
     def remote_temp_file(self):
         return self._remote_temp(False)
 
def delete_remote(self, remote):
             # TODO: Log failure to delete?
             pass
 
     def run(self, exe_path, cmd=None, work_dir='.', file_deps=None, env=None):
         target_exe_path = None
         target_cwd = None
         try:
             target_cwd = self.remote_temp_dir()
-            target_exe_path = os.path.join(target_cwd, 'libcxx_test.exe')
+            target_exe_path = str(PurePosixPath(target_cwd) / 'libcxx_test.exe')
             if cmd:
                 # Replace exe_path with target_exe_path.
                 cmd = [c if c != exe_path else target_exe_path for c in cmd]
             else:
                 cmd = [target_exe_path]
 
             srcs = [exe_path]
             dsts = [target_exe_path]
class SSHExecutor(RemoteExecutor):
 
     def _copy_in_file(self, src, dst):
         scp = self.scp_command
         remote = self.host
         remote = self.user_prefix + remote
         cmd = [scp, '-p', src, remote + ':' + dst]
         self.local_run(cmd)
 
+    def _env_command(self, env):
+        if not env:
+            return []
+
+        env_cmd = ['env']
+
+        for k, v in env.items():
+            if k == 'PATH':
+                # Pick up the existing paths, so we don't lose any commands
+                if self.target_info and self.target_info.is_windows():
+                    env_cmd.append('PATH="%s;%PATH%"' % v)
+                else:
+                    env_cmd.append('PATH="%s:$PATH"' % v)
+            else:
+                env_cmd.append('"%s"="%s"' % (k, v.replace('\\', '\\\\')))
+
+        return env_cmd
+
     def _execute_command_remote(self, cmd, remote_work_dir='.', env=None):
         remote = self.user_prefix + self.host
         ssh_cmd = [self.ssh_command, '-oBatchMode=yes', remote]
-        if env:
-            env_cmd = ['env'] + ['%s="%s"' % (k, v) for k, v in env.items()]
-        else:
-            env_cmd = []
+        env_cmd = self._env_command(env)
         remote_cmd = ' '.join(env_cmd + cmd)
         if remote_work_dir != '.':
             remote_cmd = 'cd ' + remote_work_dir + ' && ' + remote_cmd
         out, err, rc = self.local_run(ssh_cmd + [remote_cmd])
         return (remote_cmd, out, err, rc)

libcxx/utils/libcxx/test/target_info.py

class DefaultTargetInfo(object):
 
     def add_cxx_compile_flags(self, flags): pass
     def add_cxx_link_flags(self, flags): pass
     def configure_env(self, env): pass
     def allow_cxxabi_link(self): return True
     def add_sanitizer_features(self, sanitizer_type, features): pass
     def use_lit_shell_default(self): return False
 
+    def add_path(self, dest_env, new_path):
+        if not new_path:
+            return
+        if 'PATH' not in dest_env:
+            dest_env['PATH'] = new_path
+        else:
+            split_char = ';' if self.is_windows() else ':'
+            dest_env['PATH'] = '%s%s%s' % (new_path, split_char,
+                                           dest_env['PATH'])
+
 
 def test_locale(loc):
     assert loc is not None
     default_locale = locale.setlocale(locale.LC_ALL)
     try:
         locale.setlocale(locale.LC_ALL, loc)
         return True
     except locale.Error: