Page MenuHomePhabricator

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

Authored by broadwaylamb on Oct 1 2019, 7:41 AM.

Details

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.

Event Timeline

broadwaylamb created this revision.Oct 1 2019, 7:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2019, 7:41 AM
ldionne added inline comments.Oct 1 2019, 8:40 AM
libcxx/utils/libcxx/test/config.py
1208–1209

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?

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 marked an inline comment as done.

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

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–1209

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.

Use host system separators when setting PATH in config.py

bcraig added a comment.Oct 1 2019, 2:22 PM

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.

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.

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 added inline comments.Oct 11 2019, 9:38 AM
libcxx/utils/libcxx/test/config.py
1208–1209

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 updated this revision to Diff 224636.EditedOct 11 2019, 10:59 AM

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?

Revert accidentally removed import in executor.py.

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

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 marked 3 inline comments as done.Fri, Oct 18, 8:01 AM
broadwaylamb added inline comments.
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
146–151

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

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

Update the patch now that D69170 has landed.

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

broadwaylamb added a comment.EditedTue, Nov 5, 4:47 AM

@ldionne can you please take a look?

ldionne accepted this revision.Tue, Nov 5, 8:09 AM

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.

This revision is now accepted and ready to land.Tue, Nov 5, 8:09 AM
ldionne requested changes to this revision.Tue, Nov 5, 8:17 AM
ldionne added inline comments.
libcxx/utils/libcxx/test/executor.py
14

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
This revision now requires changes to proceed.Tue, Nov 5, 8:17 AM

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

broadwaylamb marked 2 inline comments as done.Tue, Nov 5, 11:30 AM
broadwaylamb added inline comments.
libcxx/utils/libcxx/test/executor.py
14

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

ldionne requested changes to this revision.Wed, Nov 6, 8:26 AM

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?

This revision now requires changes to proceed.Wed, Nov 6, 8:26 AM

@ldionne should be fixed now.