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

Set target using config file #2335

Merged
merged 1 commit into from
Feb 5, 2016
Merged

Conversation

fpgaminer
Copy link
Contributor

Fixed #2332.

This PR adds build.target to the Cargo config file, which behaves in the same way as passing --target to Cargo. Example .cargo/config:

[build]
target = "thumbv6m-none-eabi"

Similar to how --jobs overrides build.jobs, --target will override build.target.

I added documentation to config.md, and a test to test_cargo_cross_compile.rs. I couldn't get cross compile working on my machine for cargo test. Hopefully travis passes it.

This is my first PR against Cargo; sorry if I missed any procedures.

@rust-highfive
Copy link

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

Thanks for the PR! This implementation all looks good to me, I'm just not sure if we want to implement it just yet. I'll bring this up with the @rust-lang/tools team to see if others have opinions as well.

While that happens, can you be sure to update the documentation as well to reflect that this option exists?

@brson
Copy link
Contributor

brson commented Feb 2, 2016

Makes sense to me.

@fpgaminer
Copy link
Contributor Author

While that happens, can you be sure to update the documentation as well to reflect that this option exists?

Would you mind pointing me at the documentation that needs to be updated? This pull request already updates src/doc/config.md, which is the only relevant documentation I could find in this repo (grepping for jobs shows that as the only place that config.build.jobs is documented, which I used as the template for config.build.target).

@alexcrichton
Copy link
Member

Oh oops sorry I overlooked that by accident, src/doc/config.md is what I was thinking of!

@alexcrichton alexcrichton added the relnotes Release-note worthy label Feb 5, 2016
@alexcrichton
Copy link
Member

@bors: r+ 7442050

Thanks again @fpgaminer!

@bors
Copy link
Collaborator

bors commented Feb 5, 2016

⌛ Testing commit 7442050 with merge a1c3ab3...

bors added a commit that referenced this pull request Feb 5, 2016
Fixed #2332.

This PR adds `build.target` to the Cargo config file, which behaves in the same way as passing `--target` to Cargo.  Example `.cargo/config`:

```
[build]
target = "thumbv6m-none-eabi"
```

Similar to how `--jobs` overrides `build.jobs`, `--target` will override `build.target`.

I added documentation to `config.md`, and a test to `test_cargo_cross_compile.rs`.  I couldn't get cross compile working on my machine for `cargo test`.  Hopefully travis passes it.

This is my first PR against Cargo; sorry if I missed any procedures.
@bors
Copy link
Collaborator

bors commented Feb 5, 2016

@bors bors merged commit 7442050 into rust-lang:master Feb 5, 2016
@VikingMew
Copy link

Can we set this as a crate specific parameter?

@andrewdavidmackenzie
Copy link

Looks like this is not going in? Any alternative (similar) option progressing or already added?

@ehuss
Copy link
Contributor

ehuss commented Jan 28, 2019

@andrewdavidmackenzie I'm not sure what you mean. This was added a few years ago. Are you having some kind of issue? If so, it would be probably be best to open a new issue.

@andrewdavidmackenzie
Copy link

Well, Alex commented:
"I'm just not sure if we want to implement it just yet."

I didn't see any merge for it and when I tried it I got an "unused key" warning from cargo.

I'm on mobile now, but will check again later.

@andrewdavidmackenzie
Copy link

Is this intended only for binary builds in Cargo?

I can't find any mention of it in the Cargo manifest book:
https://doc.rust-lang.org/cargo/reference/manifest.html

When I try what's described in this issue:

Here is my Cargo.toml of a library with functions I want compiled to wasm:

[package]
name = "reverse"
version = "0.1.0"
authors = ["Andrew Mackenzie <andrew@mackenzie-serres.net>"]

[build]
target = "wasm32-unknown-unknown"

[lib]
name = "reverse"
crate-type = ["cdylib"]

# This field points at where the crate is located, relative to the `Cargo.toml`.
path = "src/reverse.rs"

[dependencies]
serde_json = "1.0"

[workspace]
exclude = [".."]

When I build it cargo tells me:

$  cargo build
warning: /Users/andrew/workspace/flow/samples/reverse-echo/Cargo.toml: unused manifest key: build
    Finished dev [unoptimized + debuginfo] target(s) in 0.43s

@ehuss
Copy link
Contributor

ehuss commented Jan 28, 2019

The value goes into the config file .cargo/config documented at https://doc.rust-lang.org/cargo/reference/config.html.

@andrewdavidmackenzie
Copy link

OK, I understand now. I guess it makes more sense there than in the manifest?
Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Release-note worthy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants