Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run non-native tests on real device #41268

Merged
merged 1 commit into from
May 4, 2017
Merged

Conversation

mmatyas
Copy link
Contributor

@mmatyas mmatyas commented Apr 13, 2017

After #40733, I've made some hacks to the QEMU client-server tools to allow running the tests on a real device when cross compiling Rust. The address and port of the remote server can be set using an environment variable.

I've made this mainly for local testing purposes, if you're interested in merging this, I'd clean it a bit more (eg. renaming the functions from qemu- to something else). I'm not asking for CI integration or adding ARM boards to the build system; it's just that I used these modifications and I was wondering if you'd find them useful too.

@mmatyas
Copy link
Contributor Author

mmatyas commented Apr 13, 2017

cc @alexcrichton

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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.

@alexcrichton
Copy link
Member

Seems reasonable to me! What other cleanups did you have in mind?

@mmatyas
Copy link
Contributor Author

mmatyas commented Apr 14, 2017

I was mainly thinking about renaming qemu-test-{client,server} (as it would work without QEMU after all), and some function calls. I'd probably need to document the new env var somewhere too, or alternatively replace it with a new flag, similarly to -qemu-armhf-rootfs.

@alexcrichton
Copy link
Member

Ok sounds great to me! Feel free to just ping me here when that is sorted out

@shepmaster shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017
@shepmaster
Copy link
Member

Thanks for the cool work @mmatyas! Do you think you will still be able to make the suggested changes to the PR?

@mmatyas
Copy link
Contributor Author

mmatyas commented Apr 21, 2017

Yeah, it's almost done. I was experimenting with setting the device address from an env var, then from a configure parameter to see which works better. Both have their pros/cons, but in the end personally I've found the config parameter solution cleaner. I'm going to finish this next week, sorry for the delay!

@alexcrichton
Copy link
Member

Oh no worries @mmatyas, thanks for the update!

@mmatyas
Copy link
Contributor Author

mmatyas commented Apr 27, 2017

Well it turned out to be a way bigger patch than I originally expected, but now it's nicely integrated into the build system. Some descriptions/comments are still missing or incomplete, I'll fix that. There's also some code duplication (eg. remote_push_libs). Feel free to point out any problems!

@alexcrichton
Copy link
Member

Oh sorry about that! I think this'll end up having a lot of overlap with #41575 I think? In #41575 the duplication between qemu/android is being reduced, perhaps this could tack on to that and avoid the duplication in a few locations?

@mmatyas
Copy link
Contributor Author

mmatyas commented Apr 28, 2017

Ah, yeah it seems we have an overlap; should I wait until #41575 gets sorted out / merged?

@alexcrichton
Copy link
Member

Sure yeah, I just r+'d that PR so if you want to go ahead and rebase on top you can probably start handling the rebase conflicts immediately

@bors
Copy link
Contributor

bors commented Apr 28, 2017

☔ The latest upstream changes (presumably #41575) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

Ah merged anyway now!

@mmatyas
Copy link
Contributor Author

mmatyas commented May 4, 2017

Rebased, minimalized and dropped half of the patch; also now it uses an environment variable. If TEST_DEVICE_ADDR is defined, it tries to connect to it, if not, it uses the QEMU emulator. I've also added an optional parameter to the server to support the different TCP binding requirement for the two use cases.

@mmatyas
Copy link
Contributor Author

mmatyas commented May 4, 2017

There's also an unused verbose flag, to see the running tests on the remote device. Implementing that could be a follow up PR.

@alexcrichton
Copy link
Member

@bors: r+

Looks good to me, thanks!

@bors
Copy link
Contributor

bors commented May 4, 2017

📌 Commit b194def has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented May 4, 2017

⌛ Testing commit b194def with merge 838e9c5...

bors added a commit that referenced this pull request May 4, 2017
Run non-native tests on real device

After #40733, I've made some hacks to the QEMU client-server tools to allow running the tests on a real device when cross compiling Rust. The address and port of the remote server can be set using an environment variable.

I've made this mainly for local testing purposes, if you're interested in merging this, I'd clean it a bit more (eg. renaming the functions from `qemu-` to something else). I'm not asking for CI integration or adding ARM boards to the build system; it's just that I used these modifications and I was wondering if you'd find them useful too.
@bors
Copy link
Contributor

bors commented May 4, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 838e9c5 to master...

@bors bors merged commit b194def into rust-lang:master May 4, 2017
bors added a commit that referenced this pull request Jul 20, 2017
Allow remote testing remotely when `TEST_DEVICE_ADDR` is set

Remote testing was added in #41268, but at the moment it's only enabled if QEMU is also available or we're testing Android. This patch also allows remote testing if the environment variable `TEST_DEVICE_ADDR` is set, as required by `remote-test-client` [[1](https://github.com/rust-lang/rust/blob/master/src/tools/remote-test-client/src/main.rs#L28), [2](https://github.com/rust-lang/rust/blob/master/src/tools/remote-test-client/src/main.rs#L61)]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants