Skip to content
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.

Sign up for GitHub

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

add custom export of c bindings #39

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

add custom export of c bindings #39

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7d4af3e
zephyr-sys: use expect instead of unwrap
jpeeters Dec 9, 2024
efd9538
zephyr-build: separate kconfig extraction from cargo build config
jpeeters Dec 9, 2024
554bca1
zephyr-sys: provide selective binding exports based on kconfig
jpeeters Dec 9, 2024
6d1ab8a
zephyr-sys: add missing zyphyr-build crate as dependency
jpeeters Dec 9, 2024
7a539a4
zephyr-sys: fix missing export bindings for log items
jpeeters Dec 9, 2024
7a51798
zephyr-build: refactor iteration on .config lines
jpeeters Dec 20, 2024
9d5d375
samples: fix use of renamed function from zephyr-build
jpeeters Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions zephyr-build/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,5 @@ Provides utilities for accessing Kconfig and devicetree information.
# Whether these need to be vendored is an open question. They are not
# used by the core Zephyr tree, but are needed by zephyr applications.
[dependencies]
anyhow = "1.0.93"
regex = "1.10.3"
37 changes: 27 additions & 10 deletions zephyr-build/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,30 +12,47 @@
// output configuration settings that affect the compilation.

use std::io::{BufRead, BufReader, Write};
use std::collections::HashSet;
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>> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let config_y = Regex::new(r"^(CONFIG_.*)=y$").expect("hardcoded regex is always valid");

let input = File::open(path)?;
let flags: HashSet<String> =
BufReader::new(input)
.lines()
.fold(HashSet::new(), |mut set, line| {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. I think your proposal is more readable. I'll make the change.

if let Ok(line) = &line {
if let Some(caps) = config_y.captures(line) {
set.insert(caps[1].into());
}
}
set
});

Ok(flags)
}

/// 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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


let config_y = Regex::new(r"^(CONFIG_.*)=y$").unwrap();

let file = File::open(&dotconfig).expect("Unable to open dotconfig");
for line in BufReader::new(file).lines() {
let line = line.expect("reading line from dotconfig");
if let Some(caps) = config_y.captures(&line) {
println!("cargo:rustc-cfg={}", &caps[1]);
}
}
extract_kconfig_bool_options(&dotconfig)
.expect("failed to extract flags from .config")
.iter()
.for_each(|flag| println!("cargo:rustc-cfg={flag}"));
}

/// Capture bool, numeric and string kconfig values in a 'kconfig' module.
Expand Down
1 change: 1 addition & 0 deletions zephyr-sys/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@ Zephyr low-level API bindings.
anyhow = "1.0"
bindgen = { version = "0.69.4", features = ["experimental"] }
# zephyr-build = { version = "3.7.0", path = "../zephyr-build" }
zephyr-build = { version = "3.7.0", path = "../zephyr-build" }
110 changes: 84 additions & 26 deletions zephyr-sys/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, I tend to prefer the use of anyhow::Result. Are there general guidelines on this?

Copy link
Author

@jpeeters jpeeters Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


use bindgen::Builder;

use std::env;
use std::path::{Path, PathBuf};

fn main() -> Result<()> {
use bindgen::Builder;

fn main() -> anyhow::Result<()> {
// Determine which version of Clang we linked with.
let version = bindgen::clang_version();
println!("Clang version: {:?}", version);
Expand Down Expand Up @@ -50,36 +48,60 @@ fn main() -> Result<()> {
// println!("includes: {:?}", env::var("INCLUDE_DIRS"));
// println!("defines: {:?}", env::var("INCLUDE_DEFINES"));

let out_path = PathBuf::from(env::var("OUT_DIR").unwrap());
let wrapper_path = PathBuf::from(env::var("WRAPPER_FILE").unwrap());
let out_path = PathBuf::from(env::var("OUT_DIR").expect("missing output directory"));
let wrapper_path = PathBuf::from(env::var("WRAPPER_FILE").expect("missing wrapper file"));

// Bindgen everything.
let bindings = Builder::default()
.header(Path::new("wrapper.h").canonicalize().unwrap().to_str().unwrap())
.use_core()
.clang_arg(&target_arg);
.clang_arg("-DRUST_BINDGEN")
.clang_arg(format!("-I{}/lib/libc/minimal/include", zephyr_base))
.clang_arg(&target_arg)
.header(
Path::new("wrapper.h")
.canonicalize()
.unwrap()
.to_str()
.unwrap(),
)
.use_core();

let bindings = define_args(bindings, "-I", "INCLUDE_DIRS");
let bindings = define_args(bindings, "-D", "INCLUDE_DEFINES");

let bindings = bindings
.wrap_static_fns(true)
.wrap_static_fns_path(wrapper_path)
// <inttypes.h> seems to come from somewhere mysterious in Zephyr. For us, just pull in the
// one from the minimal libc.
.clang_arg("-DRUST_BINDGEN")
.clang_arg(format!("-I{}/lib/libc/minimal/include", zephyr_base))
.derive_copy(false)
.allowlist_function("k_.*")
.allowlist_function("gpio_.*")
.allowlist_function("sys_.*")
.allowlist_function("z_log.*")
.allowlist_function("bt_.*")
.wrap_static_fns_path(wrapper_path);

let bindings = bindings.derive_copy(false);

let bindings = bindings
// Deprecated
.blocklist_function("sys_clock_timeout_end_calc")
.parse_callbacks(Box::new(bindgen::CargoCallbacks::new()));

let dotconfig = env::var("DOTCONFIG").expect("missing DOTCONFIG path");
let options = zephyr_build::extract_kconfig_bool_options(&dotconfig)
.expect("failed to extract kconfig boolean options");

let bindings = bindings
// Kernel
.allowlist_item("E.*")
.allowlist_item("K_.*")
.allowlist_item("LOG_.*")
.allowlist_item("Z_.*")
.allowlist_item("ZR_.*")
.allowlist_item("LOG_LEVEL_.*")
// Deprecated
.blocklist_function("sys_clock_timeout_end_calc")
.parse_callbacks(Box::new(bindgen::CargoCallbacks::new()))
.allowlist_function("k_.*")
.allowlist_function("z_log.*")
// Bluetooth
.allowlist_item_if("CONFIG_BT_.*", || options.contains("CONFIG_BT"))
.allowlist_function_if("bt_.*", || options.contains("CONFIG_BT"))
// GPIO
.allowlist_item_if("CONFIG_GPIO_.*", || options.contains("CONFIG_GPIO"))
.allowlist_function_if("gpio_.*", || options.contains("CONFIG_GPIO"))
// UART
.allowlist_item_if("CONFIG_UART_.*", || options.contains("CONFIG_SERIAL"))
.allowlist_function_if("uart_.*", || options.contains("CONFIG_SERIAL"))
// Generate
.generate()
.expect("Unable to generate bindings");

Expand All @@ -90,8 +112,44 @@ fn main() -> Result<()> {
Ok(())
}

trait BuilderExt {
type B;

fn allowlist_function_if<P>(self, pattern: &str, pred: P) -> Self::B
where
P: FnOnce() -> bool;

fn allowlist_item_if<P>(self, pattern: &str, pred: P) -> Self::B
where
P: FnOnce() -> bool;
}

impl BuilderExt for Builder {
type B = Builder;

fn allowlist_function_if<P>(self, pattern: &str, pred: P) -> Self::B
where
P: FnOnce() -> bool,
{
if pred() {
return self.allowlist_function(pattern);
}
self
}

fn allowlist_item_if<P>(self, pattern: &str, pred: P) -> Self::B
where
P: FnOnce() -> bool,
{
if pred() {
return self.allowlist_item(pattern);
}
self
}
}

fn define_args(bindings: Builder, prefix: &str, var_name: &str) -> Builder {
let text = env::var(var_name).unwrap();
let text = env::var(var_name).expect("missing environment variable");
let mut bindings = bindings;
for entry in text.split(" ") {
if entry.is_empty() {
Expand Down
5 changes: 4 additions & 1 deletion zephyr-sys/wrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,12 @@ extern int errno;

#include <zephyr/kernel.h>
#include <zephyr/kernel/thread_stack.h>

#include <zephyr/drivers/gpio.h>
#include <zephyr/logging/log.h>
#include <zephyr/drivers/uart.h>

#include <zephyr/bluetooth/bluetooth.h>
#include <zephyr/logging/log.h>

/*
* bindgen will output #defined constant that resolve to simple numbers. There are some symbols
Expand Down
2 changes: 1 addition & 1 deletion zephyr/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,6 @@
// output configuration settings that affect the compilation.

fn main() {
zephyr_build::export_bool_kconfig();
zephyr_build::export_kconfig_bool_options();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::build_kconfig_mod();
}
Loading