This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Add an option to ssh.py for using a different temp path
ClosedPublic

Authored by mstorsjo on Mar 5 2021, 3:10 AM.

Details

Summary

If cross testing on Windows via WSL (at least with WSL 1), the Windows
executables can't be executed if they are in WSL specific directories
(like /tmp).

Diff Detail

Event Timeline

mstorsjo created this revision.Mar 5 2021, 3:10 AM
mstorsjo requested review of this revision.Mar 5 2021, 3:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2021, 3:10 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
curdeius accepted this revision as: curdeius.Mar 5 2021, 6:10 AM
curdeius added a subscriber: curdeius.

LGTM.

ldionne requested changes to this revision.Mar 5 2021, 6:20 AM
ldionne added a subscriber: ldionne.

Is there a way to get the temp directory path on Windows? We could use that instead -- I'd love to avoid adding more arguments to this script.

Otherwise, we could just switch to mktemp -d and let it select the directory name itself (i.e. don't try to provide a template).

This revision now requires changes to proceed.Mar 5 2021, 6:20 AM

Is there a way to get the temp directory path on Windows? We could use that instead -- I'd love to avoid adding more arguments to this script.

Adding more arguments to this script really shouldn't be a burden for the rest of the projects; these arguments are only touched by the users who need them, setting them manually within -DLIBCXX_EXECUTOR="path/to/ssh.py --host=myremote --tempdir=/some/temp/dir" (where one need to have --host anyway).

Otherwise, we could just switch to mktemp -d and let it select the directory name itself (i.e. don't try to provide a template).

And no, there's not really any trivial way of finding the right temp path - especially not a generic one. Remember, this is ssh.py running on the build host, which is going to fire off a ssh command to a remote system, instructing it to create a directory there. Anything smarter would require firing off more ssh commands to launch other processes, to query things.

In my particular setup, the remote system is a WSL (linux personality on top of windows) system, which has got both its own "native linux" parts of the filesystem and the rest of the regular windows filesystem mounted. It can call windows executables transparently, just provided that they reside in the windows part of the filesystem. Just calling mktemp would pick a linux default (like /tmp) within the linux part of the filesystem, and there's most probably no generic way to from there query what the temp path within the windows half of the system would be, unless you again resort to highly non-generic things like launching a windows executable in a custom path (/mnt/c/<whatever>) to query things from that ecosystem.

So this extra option, which IMO should be very little extra burden to anybody else, allows sidestepping all of that, and other users need not know anything about the contortions to this setup, only that you can - if you want to - choose what path on the remote system it uses for dumping files.

Adding more arguments to this script really shouldn't be a burden for the rest of the projects; these arguments are only touched by the users who need them, setting them manually within -DLIBCXX_EXECUTOR="path/to/ssh.py --host=myremote --tempdir=/some/temp/dir" (where one need to have --host anyway).

With experience from cross testing in other projects, this kind of parameter is pretty much a requirement anyway. When building and executing tests locally, you never need to specify the path where to do things, as that's implicit from where you do the build (or an explicit argument to cmake). But in any case when executing things on a remote system, one would want to allow the user to specify where in the filesystem of the remote things would be placed, not assuming anything about either being able to use the same path on both systems, or hardcoded assumptions about where to place things on the remote.

ldionne accepted this revision.Mar 5 2021, 7:03 AM
This revision is now accepted and ready to land.Mar 5 2021, 7:03 AM