[libcxx-commits] [PATCH] D68275: [libcxx] [test] Query the target platform, not the host one

[Sergej Jaskiewicz via Phabricator via libcxx-commits]

broadwaylamb added inline comments.
Herald added a reviewer: mclow.lists.


================
Comment at: libcxx/utils/libcxx/test/config.py:1208
         else:
-            split_char = ';' if self.is_windows else ':'
+            split_char = ';' if self.target_info.is_windows() else ':'
             dest_env['PATH'] = '%s%s%s' % (new_path, split_char,
----------------
broadwaylamb wrote:
> ldionne wrote:
> > 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?
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68275