add custom export of c bindings #39
Conversation
This commit uses `expect` instead of `unwrap` on Result<_, _> in order to provide better error information to developer.
This commit separates the implementation of Kconfig extraction from the generation of cargo configuration at build time. With this commit, the result of Kconfig extraction can be reused anywhere else, providing a set of enabled Kconfig options.
This commit provides a selective generation of bindings based on Kconfig options. Then, only enabled features has exported bindings to Rust. As a result, one developer can see early during the Rust application build if the correct Kconfig options has been enabled. Before this commit, most problems appeared during C and Rust linking time.
jpeeters
changed the title
|
Hi @d3zd3z 👋🏼 ! Sorry to ping you. This is my first PR and I don't know who to ask for review. Do you have time to check it out? |
| use std::env; | ||
| use std::fs::File; | ||
| use std::path::Path; | ||
|
|
||
| use regex::Regex; | ||
|
|
||
| /// Extract boolean Kconfig entries. This must happen in any crate that wishes to access the | ||
| /// configuration settings. | ||
| pub fn extract_kconfig_bool_options(path: &str) -> anyhow::Result<HashSet<String>> { |
There was a problem hiding this comment.
I notice you have also changed other Result to anyhow::Result. We should probably have a discussion/argument on discord as to which is better. I generally prefer the use in simple things like this.
| /// Export boolean Kconfig entries. This must happen in any crate that wishes to access the | ||
| /// configuration settings. | ||
| pub fn export_bool_kconfig() { | ||
| pub fn export_kconfig_bool_options() { | ||
| let dotconfig = env::var("DOTCONFIG").expect("DOTCONFIG must be set by wrapper"); | ||
|
|
||
| // Ensure the build script is rerun when the dotconfig changes. | ||
| println!("cargo:rerun-if-env-changed=DOTCONFIG"); | ||
| println!("cargo-rerun-if-changed={}", dotconfig); |
There was a problem hiding this comment.
I have seen some behavior that also suggests that this construct isn't working. Not really related to this change, but there have been times I have changed a kconfig but found that the values seen in rust didn't change.
| @@ -11,14 +11,12 @@ | |||
| // This builds a program that is run on the compilation host before the code is compiled. It can | |||
| // output configuration settings that affect the compilation. | |||
|
|
|||
| use anyhow::Result; | |||
There was a problem hiding this comment.
As above, I tend to prefer the use of anyhow::Result. Are there general guidelines on this?
There was a problem hiding this comment.
I use anyhow::Result with the fully qualified path in return types in order to prevent confusion from using ::core::result::Result. This a practice I see several time in different project but we can change to what you think is better.
There was a problem hiding this comment.
I think you might have influenced me to be a bit more explicit. I'm not sure, though, about the zephyr crate itself, which defines an Error and Result type itself. I think it makes sense there to have Result be the Zephyr one, and then when a different one is needed, be explicit.
| @@ -12,6 +12,6 @@ | |||
| // output configuration settings that affect the compilation. | |||
|
|
|||
| fn main() { | |||
| zephyr_build::export_bool_kconfig(); | |||
| zephyr_build::export_kconfig_bool_options(); | |||
There was a problem hiding this comment.
This is a breaking API change to zephyr_build. This really makes me think we shouldn't be trying to match the crate version numbers with the zephyr numbers, as Zephyr doesn't do semantic versioning (at all). If our crates were all a "0.*" version, the API changes wouldn't matter.
There was a problem hiding this comment.
Yes this is definitely a breaking change. I suppose that the the project (i.e. zephyr-lang-rust) is young enough to allow with breakages at the moment.
I agree that if Zephyr is not using semver, we should not try to match the Zephyr version but have a compatibility table.
There was a problem hiding this comment.
I'll make a PR that reverses the Zephyr version number matching, going back to the 0.x versions, until we get some stability.
zephyr-build/src/lib.rs
Outdated
| let flags: HashSet<String> = | ||
| BufReader::new(input) | ||
| .lines() | ||
| .fold(HashSet::new(), |mut set, line| { |
There was a problem hiding this comment.
Using fold with a mutable set feels, well, weird to me. Not strongly enough to ask you to change it. But, I would have just used a loop to make it clear the thing was mutating a single set.
Now, on the other hand:
.lines()
.filter(|line| config_y.is_match(line))
.collect()is even more concise, well with the error handling fix. In general, I tend to find as things like this get more complicated, the comprehension versions can get a bit unwieldy.
There was a problem hiding this comment.
I understand. I think your proposal is more readable. I'll make the change.
This commit provides a selective generation of bindings based on Kconfig
options. Then, only enabled features has exported bindings to Rust.
As a result, one developer can see early during the Rust application
build if the correct Kconfig options has been enabled.
Before this commit, most problems appeared during C and Rust linking
time.