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

Add support for custom target-specific runners #3954

Merged
merged 4 commits into from May 13, 2017

Conversation

Projects
None yet
7 participants
@RReverser
Contributor

RReverser commented Apr 26, 2017

When target.$triple.runner is specified, it will be used for any execution commands by cargo including cargo run, cargo test and cargo bench. The original file is passed to the runner executable as a first argument.

This allows to run tests when cross-comping Rust projects.

This is not a complete solution and might be extended in future for better ergonomics to support passing extra arguments to the runner itself or overriding runner from the command line, but it should already unlock major existing use cases.

Fixes #1411
Resolves #3626

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Apr 26, 2017

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

rust-highfive commented Apr 26, 2017

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

@RReverser

This comment has been minimized.

Show comment
Hide comment
@RReverser
Contributor

RReverser commented Apr 26, 2017

@rust-highfive rust-highfive assigned alexcrichton and unassigned brson Apr 26, 2017

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Apr 26, 2017

Contributor

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

Contributor

bors commented Apr 26, 2017

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

@malbarbo

This comment has been minimized.

Show comment
Hide comment
@malbarbo

malbarbo Apr 26, 2017

Contributor

or overriding runner from the command line

Any config option can be override defining an environment variable, in this case CARGO_TARGET_TRIPLE_RUNNER.

Contributor

malbarbo commented Apr 26, 2017

or overriding runner from the command line

Any config option can be override defining an environment variable, in this case CARGO_TARGET_TRIPLE_RUNNER.

@RReverser

This comment has been minimized.

Show comment
Hide comment
@RReverser

RReverser Apr 26, 2017

Contributor

@malbarbo Ah, TIL. Perfect, thanks! (although I rather meant something that was suggested in other issues, like just --with=...)

Contributor

RReverser commented Apr 26, 2017

@malbarbo Ah, TIL. Perfect, thanks! (although I rather meant something that was suggested in other issues, like just --with=...)

@RReverser

This comment has been minimized.

Show comment
Hide comment
@RReverser

RReverser Apr 26, 2017

Contributor

Rebased and squashed documentation with code.

Contributor

RReverser commented Apr 26, 2017

Rebased and squashed documentation with code.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 26, 2017

Member

Looks good to me! I'm ok punting on CLI support for now, but I do think that we'll eventually add it (it's more ergonomic for the gdb/strace use case).

I wonder though if we can perhaps soup up the configuration here, where a change may otherwise be backwards incompatible. For example:

  • If the configuration is a string, we split it on spaces and pass those as arguments.
  • If the configuration is a string array, we use that instead

That way we can support passing arguments, doing so in an ergonomic fashion for easy cases, and then also supporting the fully general case.

Member

alexcrichton commented Apr 26, 2017

Looks good to me! I'm ok punting on CLI support for now, but I do think that we'll eventually add it (it's more ergonomic for the gdb/strace use case).

I wonder though if we can perhaps soup up the configuration here, where a change may otherwise be backwards incompatible. For example:

  • If the configuration is a string, we split it on spaces and pass those as arguments.
  • If the configuration is a string array, we use that instead

That way we can support passing arguments, doing so in an ergonomic fashion for easy cases, and then also supporting the fully general case.

@RReverser

This comment has been minimized.

Show comment
Hide comment
@RReverser

RReverser Apr 26, 2017

Contributor

I've been thinking about space/array thing but I'm not sure how adding it later might be backwards incompatible?

Contributor

RReverser commented Apr 26, 2017

I've been thinking about space/array thing but I'm not sure how adding it later might be backwards incompatible?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 3, 2017

Member

Ah sorry meant to respond sooner. My intention is that we interepret strings as space-separated lists of arguments, so the interpretation today (just an opaque name for a binary) would be backwards incompatible with the space-separated interpretation. For example

runner = 'C:\Program Files\nodejs\bin\node'

Today that'd be valid but if we started interpreting spaces as separators it'd break. Instead that'd ideally be specified as:

runner = ['C:\Program Files\nodejs\bin\node']
Member

alexcrichton commented May 3, 2017

Ah sorry meant to respond sooner. My intention is that we interepret strings as space-separated lists of arguments, so the interpretation today (just an opaque name for a binary) would be backwards incompatible with the space-separated interpretation. For example

runner = 'C:\Program Files\nodejs\bin\node'

Today that'd be valid but if we started interpreting spaces as separators it'd break. Instead that'd ideally be specified as:

runner = ['C:\Program Files\nodejs\bin\node']
@RReverser

This comment has been minimized.

Show comment
Hide comment
@RReverser

RReverser May 3, 2017

Contributor

@alexcrichton Ah yeah. I just thought - if we don't add support for space-separated arguments, then

runner = 'path to executable'

could be valid now and

runner = ['path to executable', ...]

could be added later without breaking backward compatibility (basically supporting two syntaxes).

Anyway, yeah, adding arguments should be quite trivial - I'll try to do that soon.

Contributor

RReverser commented May 3, 2017

@alexcrichton Ah yeah. I just thought - if we don't add support for space-separated arguments, then

runner = 'path to executable'

could be valid now and

runner = ['path to executable', ...]

could be added later without breaking backward compatibility (basically supporting two syntaxes).

Anyway, yeah, adding arguments should be quite trivial - I'll try to do that soon.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 9, 2017

Member

ping @RReverser, do you think you'll be able to update this PR? If not no worries, I can take it over!

Member

alexcrichton commented May 9, 2017

ping @RReverser, do you think you'll be able to update this PR? If not no worries, I can take it over!

@RReverser

This comment has been minimized.

Show comment
Hide comment
@RReverser

RReverser May 9, 2017

Contributor

😟 I need to parallelize myself. I definitely will (as well as for the other PR). If this is urgent, feel free to take over, otherwise, I should be able to finish a little bit later this week.

Contributor

RReverser commented May 9, 2017

😟 I need to parallelize myself. I definitely will (as well as for the other PR). If this is urgent, feel free to take over, otherwise, I should be able to finish a little bit later this week.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 9, 2017

Member

Oh no worries! Just wanted to ping :)

Member

alexcrichton commented May 9, 2017

Oh no worries! Just wanted to ping :)

RReverser added some commits Apr 26, 2017

Add support for custom target-specific runners
When `target.$triple.runner` is specified, it will be used for any execution commands by cargo including `cargo run`, `cargo test` and `cargo bench`. The original file is passed to the runner executable as a first argument.

This allows to run tests when cross-comping Rust projects.

This is not a complete solution, and might be extended in future for better ergonomics to support passing extra arguments to the runner itself or overriding runner from command line, but it should already unlock major existing usecases.

Fixes #1411
Resolves #3626
@RReverser

This comment has been minimized.

Show comment
Hide comment
@RReverser

RReverser May 12, 2017

Contributor

Added runner params support (can be passed by using a list or space-separated string).

Contributor

RReverser commented May 12, 2017

Added runner params support (can be passed by using a list or space-separated string).

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 12, 2017

Member

@bors: r+

Looks great to me!

Member

alexcrichton commented May 12, 2017

@bors: r+

Looks great to me!

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 12, 2017

Contributor

📌 Commit 851a284 has been approved by alexcrichton

Contributor

bors commented May 12, 2017

📌 Commit 851a284 has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 12, 2017

Contributor

⌛️ Testing commit 851a284 with merge e04d4d5...

Contributor

bors commented May 12, 2017

⌛️ Testing commit 851a284 with merge e04d4d5...

bors added a commit that referenced this pull request May 12, 2017

Auto merge of #3954 - RReverser:run-with, r=alexcrichton
Add support for custom target-specific runners

When `target.$triple.runner` is specified, it will be used for any execution commands by cargo including `cargo run`, `cargo test` and `cargo bench`. The original file is passed to the runner executable as a first argument.

This allows to run tests when cross-comping Rust projects.

This is not a complete solution and might be extended in future for better ergonomics to support passing extra arguments to the runner itself or overriding runner from the command line, but it should already unlock major existing use cases.

Fixes #1411
Resolves #3626
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 12, 2017

Contributor

💔 Test failed - status-travis

Contributor

bors commented May 12, 2017

💔 Test failed - status-travis

@Mark-Simulacrum

This comment has been minimized.

Show comment
Hide comment
@Mark-Simulacrum

Mark-Simulacrum May 12, 2017

Member

Tidy failure: src/cargo/util/config.rs: pub fn get_path_and_args(&self, key: &str) -> CargoResult<Option<Value<(PathBuf, Vec<String>)>>> { is over 100 characters.

Member

Mark-Simulacrum commented May 12, 2017

Tidy failure: src/cargo/util/config.rs: pub fn get_path_and_args(&self, key: &str) -> CargoResult<Option<Value<(PathBuf, Vec<String>)>>> { is over 100 characters.

@RReverser

This comment has been minimized.

Show comment
Hide comment
@RReverser

RReverser May 13, 2017

Contributor

I will run tidy before submitting PRs...
I will run tidy before submitting PRs...
I will run tidy before submitting PRs...
I will run tidy before submitting PRs...
I will run...

Contributor

RReverser commented May 13, 2017

I will run tidy before submitting PRs...
I will run tidy before submitting PRs...
I will run tidy before submitting PRs...
I will run tidy before submitting PRs...
I will run...

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton
Member

alexcrichton commented May 13, 2017

@bors: r+

Thanks @RReverser!

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 13, 2017

Contributor

📌 Commit 0f1c687 has been approved by alexcrichton

Contributor

bors commented May 13, 2017

📌 Commit 0f1c687 has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 13, 2017

Contributor

⌛️ Testing commit 0f1c687 with merge 3fa09c2...

Contributor

bors commented May 13, 2017

⌛️ Testing commit 0f1c687 with merge 3fa09c2...

bors added a commit that referenced this pull request May 13, 2017

Auto merge of #3954 - RReverser:run-with, r=alexcrichton
Add support for custom target-specific runners

When `target.$triple.runner` is specified, it will be used for any execution commands by cargo including `cargo run`, `cargo test` and `cargo bench`. The original file is passed to the runner executable as a first argument.

This allows to run tests when cross-comping Rust projects.

This is not a complete solution and might be extended in future for better ergonomics to support passing extra arguments to the runner itself or overriding runner from the command line, but it should already unlock major existing use cases.

Fixes #1411
Resolves #3626
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 13, 2017

Contributor

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

Contributor

bors commented May 13, 2017

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

@bors bors merged commit 0f1c687 into rust-lang:master May 13, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@RReverser

This comment has been minimized.

Show comment
Hide comment
@RReverser

RReverser May 13, 2017

Contributor

Thanks for review!

Contributor

RReverser commented May 13, 2017

Thanks for review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment