-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Xtask #2688
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
Xtask #2688
Conversation
This follows the approach in https://github.com/matklad/cargo-xtask and allow us to run `cargo xtask install-tools` from the project root to install the tools that comprehensive rust depends on, rather than using a bash script which is not truly cross platform.
Hi @egithinji. Thank you very much for drafting this pull request! I really like the idea of having this cross platform task execution tool! |
Thanks @michael-kerscher for reviewing! Yes that sounds like a good candidate for an additional task. Happy to collaborate on this further. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me now
} | ||
|
||
fn execute_task() -> Result<(), DynError> { | ||
let task = env::args().nth(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already depend on Clap, so you could use it here (feel free to do this in a later PR!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have pushed a new PR (#2707) for this and the other suggestions.
.arg("install") | ||
.args(args) | ||
.status() | ||
.expect("Failed to execute cargo install"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious: have you tested how the errors look if you provoke an error? Like by installing something which doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I tested manually the various scenarios (missing task, unkown task, and internal error e.g. typo 'mdbok instead of mdbook' in the install-tools logic). We get appropriate error messages and behaviour in each case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks great, so I decided to merge it now. Thanks for the ping, @egithinji! All my small comments are just that: small ideas for how I think this could be improved in small ways. I hope you'll pick up some of them in new PRs! |
Some minor improvements to an already-merged PR (#2688) on the task automation via xtask. Main ones being: - Adding more explanatory comments about what the xtask package is and what it does - Using Clap for CLI arg parsing - Using Anyhow for error handling --------- Co-authored-by: Eric Githinji <[email protected]>
Background
Comprehensive Rust requires a number of tools to be installed (e.g. mdbook and mdbook-course). As mentioned in #2509 (and discussed in #2469) it would be nice to have a cross platform command for installing these dependencies. Currently these are installed using a shell script (
install-mdbook.sh
) but this isn't truly cross platform e.g. for Windows users.Xtask
xtask outlines an approach for automating tasks in a Rust project. It involves using cargo's aliasing feature to allow us to run commands like
cargo xtask <some task>
to perform adhoc tasks via a Rust binary that we might otherwise need a shell script for.In this PR we add support for a
cargo xtask install-tools
command that will replace theinstall-mdbook.sh
script and install the dependent tools. We can potentially extend it to support for other tasks e.g.cargo xtask fmt
.