feat!: add target flag #6
Conversation
This commit adds support for specifying the target triple that blackjack uses when generating the Bazel rules. BREAKING CHANGE: The `-t,--target` argument is mandatory. Existing tooling fails to run successfully due to it missing. To achieve the same behavior as in v0.1.0, the target value should be set to `x86_64-unknown-linux-gnu`. Closes #5
|
Superseeds #3 |
`pico-args` was missing as a dependency. Thus, the crate did compile locally using cargo but failed with bazel.
| @@ -7,6 +7,11 @@ use std::path::{Path, PathBuf}; | |||
| const CARGO_TOML_RUNFILES_PATH: &str = "Cargo.toml"; | |||
| const CARGO_RUNFILES_PATH: &str = "external/blackjack_cargo/cargo"; | |||
|
|
|||
| struct Args { | |||
There was a problem hiding this comment.
With this change we'll have exactly one allowed flag, and it is also a required one. I think it might be simpler to just use std::env::args for now, we can add an argument parser if the flag situation becomes more complicated in future.
| bazel run //:main | ||
| popd | ||
|
|
||
| pushd tests/workspace | ||
| echo | bazel run //:blackjack | ||
| echo | bazel run //:blackjack -- --target="$target" | ||
| bazel run //crate1 | ||
| bazel run //crate2 | ||
| popd |
There was a problem hiding this comment.
I would like to test that the --target flag actually makes a difference when generating code. Adding support for Mac and Windows in CI might be a little complicated, but perhaps a simpler approach would be to add a test workspace that compiles on both linux and wasm, but generates a different cargo_dependencies.bzl file for both targets. You could use the time crate as in example, it depends on the stdweb crate only when the target arch is set to wasm32.
Wasm is quite well supported by rules_rust, see https://bazelbuild.github.io/rules_rust/#webassembly. This should run inside the standard ubuntu CI runner.
There was a problem hiding this comment.
Actually, just going with Linux, Mac OS and Windows seems pretty easy to do in CI (#8 for reference), so maybe just use that for the tests?
There was a problem hiding this comment.
A simple crate to test with might be: https://docs.rs/os_info/3.0.0/os_info/index.html
| @@ -48,7 +48,7 @@ load("@blackjack//:blackjack.bzl", "blackjack") | |||
| blackjack(name = "blackjack") | |||
| ``` | |||
|
|
|||
| Now run `bazel run //:blackjack`. You'll find a newly created `cargo_dependencies.bzl` file next to your `Cargo.toml`. | |||
| Now run `bazel run //:blackjack -- --target <triple>`. The `<triple>` argument value is a Rust target triple. You'll find a newly created `cargo_dependencies.bzl` file next to your `Cargo.toml`. | |||
There was a problem hiding this comment.
Maybe: 'The <triple> argument value is the Rust target triple the dependencies will be built for'
| @@ -103,6 +103,3 @@ prefix = "blackjack_crates_io" | |||
| ``` | |||
|
|
|||
| Now instead of `@crates_io_serde//:serde`, use `blackjack_crates_io_serde//:serde`. | |||
|
|
|||
| # Things that don't work yet (but would gladly accept a PR for) | |||
| * Support for Windows and Mac. For the moment everything assumes your host and target is `x86_64-unknown-linux-gnu` | |||
There was a problem hiding this comment.
Maybe we should leave a note here saying that Mac and Windows support is supported, but untested.
There was a problem hiding this comment.
https://github.com/SirWindfield/blackjack/pull/1
some more progress
works on mac
There was a problem hiding this comment.
I haven't had a lot of time lately for programming-related projects :( Would be nice if @wildarch could review https://github.com/SirWindfield/blackjack/pull/1 and see if it is more inline with the changes that he requested 😄
Notable changes as far as I can see:
cargois downloaded depending on the type of platform it runs. All tier 1 platforms are supported (Linux, macOS, Windows).- The path to the
Cargo.tomlfile is now configurable. Looks like you don't have tocdinto folders now and can invoke the commands from any level (?)
There was a problem hiding this comment.
I've just reviewed the PR, looks great! 😄
|
I'm going to close this PR now that #9 makes it obsolete. Thanks for all your efforts @sirwindfield! This was very helpful in putting #9 together 😄 |
|
No problem! Glad that it worked out in the end! I might have some time for the Windows part :) |
This commit adds support for specifying the target triple that blackjack
uses when generating the Bazel rules.
BREAKING CHANGE: The
-t,--targetargument is mandatory. Existingtooling fails to run successfully due to it missing. To achieve the same
behavior as in v0.1.0, the target value should be set to
x86_64-unknown-linux-gnu.Closes #5