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

add support for target.'cfg(..)'.runner #5959

Merged
merged 2 commits into from
Sep 9, 2018
Merged

Conversation

japaric
Copy link
Member

@japaric japaric commented Sep 1, 2018

cfg can be used to reduce the number of runners one needs to type in
.cargo/config. So instead of writing this:

[target.thumbv6m-none-eabi]
runner = "arm-none-eabi-gdb"

[target.thumbv7m-none-eabi]
runner = "arm-none-eabi-gdb"

[target.thumbv7em-none-eabi]
runner = "arm-none-eabi-gdb"

[target.thumbv7em-none-eabihf]
runner = "arm-none-eabi-gdb"

one can write:

[target.'cfg(all(target_arch = "arm", target_os = "none"))']
runner = "arm-none-eabi-gdb"

Handling of edge cases:

  • When target.$triple.runner matches it will be chosen regardless of the
    existence of others target.'cfg(..)'.runners.

  • If more than one target.'cfg(..)'.runner matches the target the command will
    fail.

closes #5946


Does this sound like a reasonable feature / implementation?

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@@ -116,7 +116,7 @@ impl<'cfg> Compilation<'cfg> {
rustc_process: rustc,
host: bcx.host_triple().to_string(),
target: bcx.target_triple().to_string(),
target_runner: LazyCell::new(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this eager which means that in the case of cfg collision cargo run will immediately fail.

Alternatively, if we kept this lazy in that scenario cargo run will first build the crate and then emit an error message about the runners (I think).

I don't know if making this eager has any perceivable effects on existing uses of cargo run.

Which behavior do we want here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah I think eagerly loading it here should be fine!

let mut matching_runner = None;

for t in table.val.keys() {
if t.starts_with("cfg(") && t.ends_with(')') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it's probably duplicating the logic when matching RUSTFLAGS as well, could those two code paths perhaps be deduplicated?

@bors
Copy link
Contributor

bors commented Sep 6, 2018

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

@japaric
Copy link
Member Author

japaric commented Sep 8, 2018

I'm writing the embedded Rust book and this would simplify instructions in a few places. Do you think this could still make it into 1.30-beta? In any case, I'll address the review comment as soon as I can.

`cfg` can be used to reduce the number of `runner`s one needs to type in
`.cargo/config`. So instead of writing this:

``` toml
[target.thumbv6m-none-eabi]
runner = "arm-none-eabi-gdb"

[target.thumbv7m-none-eabi]
runner = "arm-none-eabi-gdb"

[target.thumbv7em-none-eabi]
runner = "arm-none-eabi-gdb"

[target.thumbv7em-none-eabihf]
runner = "arm-none-eabi-gdb"
```

one can write:

``` toml
[target.'cfg(all(target_arch = "arm", target_os = "none"))']
runner = "arm-none-eabi-gdb"
```

Handling of edge cases:

- When `target.$triple.runner` matches it will be chosen regardless of the
  existence of others `target.'cfg(..)'.runner`s.

- If more than one `target.'cfg(..)'.runner` matches the target the command will
  fail.

closes rust-lang#5946
@japaric
Copy link
Member Author

japaric commented Sep 8, 2018

Rebased and addressed the review comment.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 9, 2018

📌 Commit 33a6d99 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Sep 9, 2018

⌛ Testing commit 33a6d99 with merge 41f98f3...

bors added a commit that referenced this pull request Sep 9, 2018
add support for `target.'cfg(..)'.runner`

`cfg` can be used to reduce the number of `runner`s one needs to type in
`.cargo/config`. So instead of writing this:

``` toml
[target.thumbv6m-none-eabi]
runner = "arm-none-eabi-gdb"

[target.thumbv7m-none-eabi]
runner = "arm-none-eabi-gdb"

[target.thumbv7em-none-eabi]
runner = "arm-none-eabi-gdb"

[target.thumbv7em-none-eabihf]
runner = "arm-none-eabi-gdb"
```

one can write:

``` toml
[target.'cfg(all(target_arch = "arm", target_os = "none"))']
runner = "arm-none-eabi-gdb"
```

Handling of edge cases:

- When `target.$triple.runner` matches it will be chosen regardless of the
  existence of others `target.'cfg(..)'.runner`s.

- If more than one `target.'cfg(..)'.runner` matches the target the command will
  fail.

closes #5946

---

Does this sound like a reasonable feature / implementation?
@bors
Copy link
Contributor

bors commented Sep 9, 2018

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

@bors bors merged commit 33a6d99 into rust-lang:master Sep 9, 2018
@ehuss ehuss added this to the 1.30.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

runner setting is ignored when set via target.cfg(..)
5 participants