[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

1208 ↗

(On Diff #222614)

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

1208 ↗

(On Diff #222614)

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

1208 ↗

(On Diff #222614)

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 ↗

(On Diff #224641)

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 ↗

(On Diff #224641)

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

libcxx/utils/libcxx/test/executor.py

146

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

12

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

12

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/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 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 = 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]