This is an archive of the discontinued LLVM Phabricator instance.

Copy data files to the remote runner.
ClosedPublic

Authored by danalbert on Mar 6 2015, 11:37 AM.

Details

Summary

The data files for any given test will be in the same directory as the
source with a file name that matches *.dat. To make these available to
tests running remotely (such as over adb or ssh), copy them into the
test's remote working directory.

Note that we will perform more copies than we actually need. The data
files in the directory may only be used by one of the tests, but will
be copied for all tests in the same directory.

This patch also moves the remote test binary into the working
directory (previously it was only invoked from the working directory
rather than existing in it).

Diff Detail

Event Timeline

danalbert updated this revision to Diff 21381.Mar 6 2015, 11:37 AM
danalbert retitled this revision from to Copy data files to the remote runner..
danalbert updated this object.
danalbert edited the test plan for this revision. (Show Details)
danalbert added reviewers: jroelofs, EricWF.
danalbert set the repository for this revision to rL LLVM.
danalbert added a subscriber: Unknown Object (MLST).
EricWF edited edge metadata.Mar 6 2015, 12:17 PM

LGTM as long as the inline comment is addressed.

Is there rational for just not requiring that the source tree be checked out on the target already? I haven't looked to hard at this executor stuff but that seems like it might solve some of these problems.

test/libcxx/test/executor.py
117

Is is possible that two tests could be run in parallel and try and use the same path?

jroelofs added inline comments.Mar 8 2015, 11:08 AM
test/libcxx/test/executor.py
115

This feels like it's at the wrong level of abstraction. Shouldn't the "which files do I copy in" part be determined by the caller?

117

As long as the implementation of remote_temp_dir provides uniquing between the processes, then this will be fine. For the SSHExecutor, that is implemented by 'mktemp -d', which DTRT. I don't remember off the top of my head what the LocalExecutor does. It's definitely worth adding another "parallelism gotcha" note to the Executor interface covering this situation.

danalbert updated this revision to Diff 21628.Mar 10 2015, 1:37 PM
danalbert edited edge metadata.

Make determining file dependencies the caller's responsibility.

test/libcxx/test/executor.py
115

Good point. New version moves this responsibility up.

jroelofs edited edge metadata.Mar 10 2015, 1:54 PM

LGTM with a couple of small tweaks.

test/libcxx/test/executor.py
13–14

Need to update the docs to reference the new parameter.

117

Should probably

s/libcxx_test/libcxx_test\.exe/

... even though Linux and Darwin don't care about the extension. It makes the intent slightly clearer.

Perhaps this should go in a separate commit? This line LGTM if you want to commit that now.

128

It would be better if this were rolled in with the above call to copy_in, that way the copy could later be implemented with compression/decompression steps at either end.

test/libcxx/test/format.py
118

I would much rather these be explicitly called out in the test's metadata, but this is fine for now. Perhaps add a TODO?

danalbert updated this revision to Diff 21643.Mar 10 2015, 2:57 PM
danalbert edited edge metadata.

Address review comments

jroelofs accepted this revision.Mar 10 2015, 3:05 PM
jroelofs edited edge metadata.

LGTM

test/libcxx/test/executor.py
128

sorry, what I meant was:

to_copy = ([exe_path], [target_exe])
if file_deps is not None:
    dev_paths = [os.path.join(target_cwd, os.path.basename(f))
                 for f in file_deps]
    to_copy[0] += file_deps
    to_copy[1] += dev_paths
self.copy_in(to_copy[0], to_copy[1])
This revision is now accepted and ready to land.Mar 10 2015, 3:05 PM
danalbert updated this revision to Diff 21652.Mar 10 2015, 3:33 PM
danalbert edited edge metadata.

Bundle more files

This revision was automatically updated to reflect the committed changes.