Make remote-test-client and remote-test-server compatible with windows #72672

seritools · 2020-05-27T20:01:51Z

compiletest and remote-test-client:

The command line for remote-test-client was changed slightly to allow cross-platform compatible paths. The old way of supplying the support libs was by joining their paths with the executable path
with :. This caused Windows-style paths to be split after the directory letter. Now, the number of support libs is provided as a parameter as well, and the support lib paths are split off from the regular args in the client.

remote-test-server:

Marked Unix-only parts as such and implemented Windows alternatives
On Windows LD_LIBRARY_PATH doesn't exist. Libraries are loaded from PATH though, so that's the way around it.
Tiny cleanup: Command::args/envs instead of manually looping over them
The temp path for Windows has to be set via environment variable, since there isn't a global temp directory that would work on every machine (as a static string)

rust-highfive · 2020-05-27T20:01:54Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Mark-Simulacrum

I can't be sure this is correct for Windows, but it largely seems fine to me. Left one nit.

src/tools/remote-test-server/src/main.rs

nikomatsakis · 2020-05-28T16:50:30Z

Good example where I would want to ping the windows group, once it's setup. For now, cc @retep998 and @rylev to check briefly the assertions about PATH and LD_LIBRARY_PATH above.

r=me otherwise.

seritools · 2020-05-28T17:16:15Z

Just for reference: DLL Search Order for Desktop Applications

EDIT: Similar handling for DLL loading here

rylev · 2020-05-28T17:20:18Z

Pinging @kennykerr and @sivadeilra as well.

retep998 · 2020-05-28T19:00:02Z

The PATH handling looks fine to me.

kennykerr · 2020-05-28T19:06:51Z

Looks good.

Mark-Simulacrum · 2020-05-28T20:05:43Z

@bors r+

bors · 2020-05-28T20:05:44Z

📌 Commit 94b7d27d3b7b59a55cf7ee66709cebeb25ea99ae has been approved by Mark-Simulacrum

tblah · 2020-05-29T09:39:40Z

src/tools/remote-test-server/src/main.rs

    dst
 }

+#[cfg(not(windows))]
+fn set_permissions(path: &Path) {
+    t!(fs::set_permissions(&dst, Permissions::from_mode(0o755)));


I think dst should be path here

Argh, yes! Really gotta setup WSL2 or a VM to test this, will do that later today.

(Is there a way to quickly run the equivalent of cargo check for different targets that don't support building form the host?)

You can use cross
e.g.

cross check --target x86_64-unknown-linux-gnu

Thanks, will check it out later today

seritools · 2020-05-29T10:10:05Z

What's the correct procedure here? I guess bors is gonna fail the merge when trying to test anything that's tested via the remote server. Can the new commit be re-submitted before it has to do all of that? 😅

( @nikomatsakis )

Mark-Simulacrum · 2020-05-29T10:27:58Z

It's automatically removed from queue if new commits are pushed, though there is the edge case of already being in a rollup. I think we're all good here though, so let's @bors r+

bors · 2020-05-29T10:27:59Z

📌 Commit 36d6791 has been approved by Mark-Simulacrum

seritools · 2020-05-29T10:29:43Z

Gotcha, thanks!

…rk-Simulacrum Make remote-test-client and remote-test-server compatible with windows `compiletest` and `remote-test-client`: The command line for `remote-test-client` was changed slightly to allow cross-platform compatible paths. The old way of supplying the support libs was by joining their paths with the executable path with `:`. This caused Windows-style paths to be split after the directory letter. Now, the number of support libs is provided as a parameter as well, and the support lib paths are split off from the regular args in the client. `remote-test-server`: - Marked Unix-only parts as such and implemented Windows alternatives - On Windows `LD_LIBRARY_PATH` doesn't exist. Libraries are loaded from `PATH` though, so that's the way around it. - Tiny cleanup: `Command::args`/`envs` instead of manually looping over them - The temp path for Windows has to be set via environment variable, since there isn't a global temp directory that would work on every machine (as a static string)

RalfJung · 2020-05-30T21:06:53Z

Failed in #72786 (comment).
@bors r-

seritools · 2020-05-31T13:46:21Z

Wow, the error seems weird :S Can't see any reason why this shouldn't run on arm-unknown-linux-gnueabihf. (Exact CI log line here)

I've manually compiled and opened for that target it via cross inside a linux/WSL2 VM and it behaves just like it should, only panicking if the parameter is missing or invalid. I don't see however how that parameter could be either of that, it's unconditionally added by compiletest. I didn't find any other location where the remote-test-client is called, either :(

While testing I've noticed that the client has a help text that I didn't update before, so I've done that to align it with the new parameters.

Any ideas?

r? @Mark-Simulacrum

Mark-Simulacrum · 2020-05-31T14:38:11Z

Oh, I forgot -- I think we also need to update

rust/src/bootstrap/test.rs

Line 1773 in 4512721

format!("{} run", builder.tool_exe(Tool::RemoteTestClient).display()),

and

rust/src/bootstrap/test.rs

Line 1891 in 4512721

let tool = builder.tool_exe(Tool::RemoteTestClient);

which both call remote-test-client outside of compiletest.

I'm not sure what the second place there is doing but the first one is using cargo's support for test runners, loosely documented here -- https://doc.rust-lang.org/nightly/cargo/reference/config.html?highlight=runner#targettriplerunner -- though as we're setting the environment variable I suspect we can't use arrays.

seritools · 2020-05-31T14:59:38Z

Ohh, now everything makes sense!

The arguments will be passed to the runner with the executable to run as the last argument.

Because of this I think we have to move the submodule count before the executable, should be doable :)

It doesn't seem like the cargo runner has a way of supplying support libs anyways, so setting it to 0 should be fine.

About the second occurrence (l. 1891): That one is for the spawn-emulator subcommand which is unchanged though all of this.

`compiletest` and `remote-test-client`: The command line for `remote-test-client` was changed slightly to allow cross-platform compatible paths. The old way of supplying the support libs was by joining their paths with the executable path with `:`. This caused Windows-style paths to be split after the directory letter. Now, the number of support libs is provided as a parameter as well, and the support lib paths are split off from the regular args in the client. `remote-test-server`: - Marked Unix-only parts as such and implemented Windows alternatives - On Windows `LD_LIBRARY_PATH` doesn't exist. Libraries are loaded from `PATH` though, so that's the way around it. - Tiny cleanup: `Command::args`/`envs` instead of manually looping over them - The temp path for Windows has to be set via environment variable, since there isn't a global temp directory that would work on every machine (as a static string)

Since cargo appends executable/args, the support_lib count parameter has to come first.

seritools · 2020-05-31T17:55:59Z

Alright! Let's give it another shot if everything looks good.
@Mark-Simulacrum

Mark-Simulacrum · 2020-05-31T21:49:58Z

@bors r+ rollup=never

bors · 2020-05-31T21:49:59Z

📌 Commit 036da3a has been approved by Mark-Simulacrum

Dylan-DPC-zz · 2020-06-02T01:21:54Z

@bors p=1

bors · 2020-06-02T07:54:43Z

⌛ Testing commit 036da3a with merge eeaf497...

bors · 2020-06-02T11:33:33Z

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing eeaf497 to master...

rust-highfive assigned nikomatsakis May 27, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 27, 2020

Mark-Simulacrum reviewed May 27, 2020

View reviewed changes

src/tools/remote-test-server/src/main.rs Outdated Show resolved Hide resolved

seritools force-pushed the remote-test-windows branch from d566ef7 to 94b7d27 Compare May 28, 2020 08:08

seritools requested a review from Mark-Simulacrum May 28, 2020 13:59

nikomatsakis approved these changes May 28, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2020

Mark-Simulacrum mentioned this pull request May 28, 2020

Remote testing fixes #72704

Merged

tblah reviewed May 29, 2020

View reviewed changes

seritools force-pushed the remote-test-windows branch from 94b7d27 to 36d6791 Compare May 29, 2020 09:57

Dylan-DPC-zz mentioned this pull request May 29, 2020

Rollup of 12 pull requests #72736

Closed

RalfJung mentioned this pull request May 30, 2020

Rollup of 10 pull requests #72786

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 30, 2020

seritools force-pushed the remote-test-windows branch from 36d6791 to f07a093 Compare May 31, 2020 13:29

rust-highfive assigned Mark-Simulacrum and unassigned nikomatsakis May 31, 2020

seritools added 4 commits May 31, 2020 17:23

Unify temp path generation for non-android

41d540f

Fix set_permissions call for non-windows

577ac2f

Update help text of remote-test-client to reflect changed command

0199fdc

seritools force-pushed the remote-test-windows branch from f07a093 to 5f179c1 Compare May 31, 2020 15:24

Make remote-test-client work as cargo runner again

036da3a

Since cargo appends executable/args, the support_lib count parameter has to come first.

seritools force-pushed the remote-test-windows branch from 5f179c1 to 036da3a Compare May 31, 2020 15:36

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 31, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 2, 2020

bors merged commit eeaf497 into rust-lang:master Jun 2, 2020

seritools deleted the remote-test-windows branch June 2, 2020 12:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make remote-test-client and remote-test-server compatible with windows #72672

Make remote-test-client and remote-test-server compatible with windows #72672

seritools commented May 27, 2020

rust-highfive commented May 27, 2020

Mark-Simulacrum left a comment

nikomatsakis commented May 28, 2020

seritools commented May 28, 2020 •

edited

Loading

rylev commented May 28, 2020

retep998 commented May 28, 2020

kennykerr commented May 28, 2020

Mark-Simulacrum commented May 28, 2020

bors commented May 28, 2020

tblah May 29, 2020

seritools May 29, 2020

tblah May 29, 2020

seritools May 29, 2020

seritools commented May 29, 2020

Mark-Simulacrum commented May 29, 2020

bors commented May 29, 2020

seritools commented May 29, 2020

RalfJung commented May 30, 2020

seritools commented May 31, 2020 •

edited

Loading

Mark-Simulacrum commented May 31, 2020

seritools commented May 31, 2020

seritools commented May 31, 2020 •

edited

Loading

Mark-Simulacrum commented May 31, 2020

bors commented May 31, 2020

Dylan-DPC-zz commented Jun 2, 2020

bors commented Jun 2, 2020

bors commented Jun 2, 2020

Make remote-test-client and remote-test-server compatible with windows #72672

Make remote-test-client and remote-test-server compatible with windows #72672

Conversation

seritools commented May 27, 2020

rust-highfive commented May 27, 2020

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

nikomatsakis commented May 28, 2020

seritools commented May 28, 2020 • edited Loading

rylev commented May 28, 2020

retep998 commented May 28, 2020

kennykerr commented May 28, 2020

Mark-Simulacrum commented May 28, 2020

bors commented May 28, 2020

tblah May 29, 2020

Choose a reason for hiding this comment

seritools May 29, 2020

Choose a reason for hiding this comment

tblah May 29, 2020

Choose a reason for hiding this comment

seritools May 29, 2020

Choose a reason for hiding this comment

seritools commented May 29, 2020

Mark-Simulacrum commented May 29, 2020

bors commented May 29, 2020

seritools commented May 29, 2020

RalfJung commented May 30, 2020

seritools commented May 31, 2020 • edited Loading

Mark-Simulacrum commented May 31, 2020

seritools commented May 31, 2020

seritools commented May 31, 2020 • edited Loading

Mark-Simulacrum commented May 31, 2020

bors commented May 31, 2020

Dylan-DPC-zz commented Jun 2, 2020

bors commented Jun 2, 2020

bors commented Jun 2, 2020

seritools commented May 28, 2020 •

edited

Loading

seritools commented May 31, 2020 •

edited

Loading

seritools commented May 31, 2020 •

edited

Loading