-
Notifications
You must be signed in to change notification settings - Fork 195
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
Make xtask work on Windows, add run-example
command
#1215
Conversation
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.
Code looks good to me! Just a small nitpick on my side.
I do wonder if we want to unify (heh) how to run examples, like do we want to document that this is the way, or do we also allow users to cd into examples and get their hands dirty?
cc: @JurajSadel regarding documenting this
@@ -146,6 +146,11 @@ pub fn build_documentation( | |||
builder = builder.arg("--open"); | |||
} | |||
|
|||
// If targeting an Xtensa device, we must use the '+esp' toolchain modifier: | |||
if target.starts_with("xtensa") { |
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.
In the back of my head I'm wondering if this will cause issues down the line, but I don't have any real argument not to do this right now, I suppose we could always remove it if it does.
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 can't really think of a situation where this would be problematic, we're in control of the Xtensa toolchain and this repository, so any changes made will be made by us. I think it's fine (at least for now).
I'd say this could be what we tell users since they don't need to care about using the esp toolchain or which features to set. More experienced users probably will skip the README and just |
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.
LGTM, thanks for this! Sorry for always forgetting about Windows users 😁
Since Windows is a little special there are various things where we need to do things in a special way.
Additionally, this adds the
run-example
command. That makes it easy to run an example without worrying about the features. We can also get nicer errors when trying to run something on an unsupported chip.