-
Notifications
You must be signed in to change notification settings - Fork 747
Boards: Add a build_scripts crate #4172
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
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.
This feels like an improvement over our current build.rs story to me.
For final PR, it might make sense to push this to crates.io and have the default template / example point there instead of the git version? (Or at least a comment suggesting such for out of tree use cases). I guess out of tree folks could also pin a hash, but that doesn't feel very ergonomic for them.
No, I suppose not. I think this came up on the release issue already, but I do increasingly think it makes sense to have a process where we publish versions of each of our crates on releases. It's functionally the same thing, I understand, but I think the optics are more clearly 'this is a released artifact to use and build on'. |
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 like this. I think it's pretty clean and I like using crates for this.
I don't have a feel for what the alternatives are, but I don't really see downsides here.
boards/build_scripts/README.md
Outdated
|
||
```toml | ||
# Cargo.toml | ||
... |
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.
... | |
# ... |
This will get the markdown diff to render correctly without all the red
All boards can include this as a dev dependency to use the common build.rs functions.
Include updates on using build_scripts, but also more generally the document was out of date.
I made some progress on this, including fixing the lack of CI is failing because OT is copying linker scripts around. That was in part the motivation for this PR, as having a crate should make it easier for different boards that need custom support to implement that support without introducing additional complexity for boards that do not need it (as might happen when all we have is a single shared build.rs file). But I'm not sure what to do at this point or why we need to copy around linker scripts. |
I pushed a fix for the Makefiles. Most of those layout copies were completely unnecessary and just a result of having been copied from the OT Makefile. The OT Makefile only needed the test_layout.ld copy to get the tests to work, not the other one that was breaking the build. |
Pull Request Overview
Writing a suitable build.rs file is subtle and repetitive. Historically, we have copied the same build.rs file into each board crate, and we recently moved to having one copy of the build.rs file in the boards/ directory and then pointing to it from each board's Cargo.toml. This is the next iteration by putting our build.rs logic in a proper crate which can be included by each board and called from the board's build.rs.
This has two key benefits:
This is a draft PR for now to see if people like how this looks before switching all boards.
Testing Strategy
Compiling the nrf52840dk.
TODO or Help Wanted
Thoughts? I don't really have a sense if this makes the linker script detection/inclusion more fragile, less fragile, or the same.
I also renamed the linker script to make it more clear this is for Tock.
Documentation Updated
/docs
, or no updates are required.Probably need to update the out-of-tree board guide if this is something we go with.
Formatting
make prepush
.