From 1ad5f2019781c08981cb71b21fb8a1dbc7a45a15 Mon Sep 17 00:00:00 2001 From: chrysn Date: Tue, 30 Jan 2024 14:25:31 +0100 Subject: [PATCH 1/6] doc: Update versioning / markers --- README.md | 10 ++++++++-- src/lib.rs | 10 ++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index d986e0c..94e8222 100644 --- a/README.md +++ b/README.md @@ -60,11 +60,17 @@ and efforts are made to not make breaking changes even while in the 0.x phase. Note that as it passes on RIOT internals, any of the SemVer guarantees only hold when built on the *same* RIOT -- once the underlying C code is changed, all bets are off. -Users of `riot-rs` can introspect its markers (see `build.rs`) -to influence which symbols to use. +Users of `riot-rs` can introspect the `DEP_RIOT_SYS_...` variables +that are available to crates that set `links = "riot-sys"` +to affect the symbols those crates use. +Typical variables to inspect are `DEP_RIOT_SYS_BINDGEN_OUTPUT_FILE` +(to determine whether a symbol is imported in the first place, eg. when RIOT renames something) +and `DEP_RIOT_SYS_CFLAGS` which includes the enabled modules. #### Markers +**Deprecated, see below**. + Some decisions of downstream crates need to depend on whether some feature is around in RIOT. For many things that's best checked on module level, but some minor items have no module to mark the feature, and checking for versions by numers is not fine-grained enough, diff --git a/src/lib.rs b/src/lib.rs index 0ec9030..7af03c0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -56,11 +56,17 @@ //! Note that as it passes on RIOT internals, //! any of the SemVer guarantees only hold when built on the *same* RIOT -- //! once the underlying C code is changed, all bets are off. -//! Users of `riot-rs` can introspect its markers (see `build.rs`) -//! to influence which symbols to use. +//! Users of `riot-rs` can introspect the `DEP_RIOT_SYS_...` variables +//! that are available to crates that set `links = "riot-sys"` +//! to affect the symbols those crates use. +//! Typical variables to inspect are `DEP_RIOT_SYS_BINDGEN_OUTPUT_FILE` +//! (to determine whether a symbol is imported in the first place, eg. when RIOT renames something) +//! and `DEP_RIOT_SYS_CFLAGS` which includes the enabled modules. //! //! ### Markers //! +//! **Deprecated, see below**. +//! //! Some decisions of downstream crates need to depend on whether some feature is around in //! RIOT. For many things that's best checked on module level, but some minor items have no //! module to mark the feature, and checking for versions by numers is not fine-grained enough, From 0e64a42d1902c86a087fc0ac8cf8b936f33b3c78 Mon Sep 17 00:00:00 2001 From: chrysn Date: Tue, 30 Jan 2024 14:25:35 +0100 Subject: [PATCH 2/6] build.rs: Drop InCode variant This has been causing warnings all over the place, just because it was a marker variant that was currently not in use. As no markers will be added any more as per the deprecation note, this can go already. --- build.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/build.rs b/build.rs index 0c380f6..77ac271 100644 --- a/build.rs +++ b/build.rs @@ -642,8 +642,6 @@ fn main() { /// This is equivalent to not having the marker in the first place, except that their /// presence serves as a reminder to not reuse that marker name. Never, - /// A marker that is set if the given string is found in the bindgen output. - InCode(&'static str), /// A marker that is set if its name is found in the bindgen output. Shorthand for /// Text(name). NameInCode, @@ -673,7 +671,6 @@ fn main() { ]; for (needle, name) in markers { let found = match needle { - InCode(s) => bindgen_output.contains(s), NameInCode => bindgen_output.contains(name), Always => true, Never => false, From 870cf531a62579140561b1c162b428f32c2ea6e0 Mon Sep 17 00:00:00 2001 From: chrysn Date: Tue, 30 Jan 2024 15:55:29 +0100 Subject: [PATCH 3/6] build: Move away from deprecated shlex function --- Cargo.toml | 2 +- build.rs | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index aca4f7e..48ccb92 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,7 +25,7 @@ riot-rs-core = { version = "< 0.2.0", optional = true } [build-dependencies] bindgen = "^0.64" -shlex = "^1" +shlex = "^1.3" serde_json = "1" serde = { version = "1", features = [ "derive" ] } regex = "1" diff --git a/build.rs b/build.rs index 77ac271..d057245 100644 --- a/build.rs +++ b/build.rs @@ -119,7 +119,8 @@ fn main() { cc = consensus_cc .expect("Entries are present in compile_commands.json") .to_string(); - cflags = shlex::join(consensus_cflag_groups.unwrap().iter().flatten().map(|s| *s)); + cflags = shlex::try_join(consensus_cflag_groups.unwrap().iter().flatten().map(|s| *s)) + .expect("Input is not expected to contain NUL characters"); let usemodule = { #[cfg(not(feature = "riot-rs"))] From 833a53395352ff5018c68f918fa152b9d41c825c Mon Sep 17 00:00:00 2001 From: chrysn Date: Wed, 31 Jan 2024 12:49:16 +0100 Subject: [PATCH 4/6] build: Remove support for explicitly passed CC/CFLAGS --- build.rs | 225 +++++++++++++++++++++++++++---------------------------- 1 file changed, 109 insertions(+), 116 deletions(-) diff --git a/build.rs b/build.rs index d057245..551d07d 100644 --- a/build.rs +++ b/build.rs @@ -29,134 +29,127 @@ fn main() { let compile_commands_json = "DEP_RIOT_BUILD_COMPILE_COMMANDS_JSON"; println!("cargo:rerun-if-env-changed=BUILDING_RIOT_RS"); - println!("cargo:rerun-if-env-changed=RIOT_CC"); - println!("cargo:rerun-if-env-changed=RIOT_CFLAGS"); println!("cargo:rerun-if-env-changed={}", &compile_commands_json); - if let Ok(commands_json) = env::var(compile_commands_json) { - println!("cargo:rerun-if-changed={}", commands_json); - let commands_file = std::fs::File::open(&commands_json) - .expect(&format!("Failed to open {}", &commands_json)); + let Ok(commands_json) = env::var(compile_commands_json) else { + panic!("Passing RIOT_COMPILE_COMMANDS_JSON is mandatory; passing RIOT_CC / RIOT_CFLAGS is not supported any more."); + }; + + println!("cargo:rerun-if-changed={}", commands_json); + let commands_file = + std::fs::File::open(&commands_json).expect(&format!("Failed to open {}", &commands_json)); - #[derive(Debug, serde::Deserialize)] - struct Entry { - arguments: Vec, + #[derive(Debug, serde::Deserialize)] + struct Entry { + arguments: Vec, + } + let parsed: Vec = serde_json::from_reader(commands_file) + .expect(&format!("Failed to parse {}", &compile_commands_json)); + + // We need to find a consensus list -- otherwise single modules like stdio_uart that + // defines anything odd for its own purpose can throw things off. (It's not like the actual + // ABI compatibility should suffer from them, for any flags like enum packing need to be + // the same systemwide anyway for things to to go very wrong) -- but at any rate, finding + // some consensus is to some extent necessary here). + // + // This is relatively brittle, but still better than the previous approach of just taking + // the first entry. + // + // A good long-term solution might be to take CFLAGS as the build system produces them, but + // pass them through the LLVMization process of create_compile_commands without actually + // turning them into compile commands. + let mut consensus_cc: Option<&str> = None; + let mut consensus_cflag_groups: Option>> = None; + for entry in parsed.iter() { + if let Some(consensus_cc) = consensus_cc.as_ref() { + assert!(consensus_cc == &entry.arguments[0]) + } else { + consensus_cc = Some(&entry.arguments[0]); } - let parsed: Vec = serde_json::from_reader(commands_file) - .expect(&format!("Failed to parse {}", &compile_commands_json)); - - // We need to find a consensus list -- otherwise single modules like stdio_uart that - // defines anything odd for its own purpose can throw things off. (It's not like the actual - // ABI compatibility should suffer from them, for any flags like enum packing need to be - // the same systemwide anyway for things to to go very wrong) -- but at any rate, finding - // some consensus is to some extent necessary here). - // - // This is relatively brittle, but still better than the previous approach of just taking - // the first entry. - // - // A good long-term solution might be to take CFLAGS as the build system produces them, but - // pass them through the LLVMization process of create_compile_commands without actually - // turning them into compile commands. - let mut consensus_cc: Option<&str> = None; - let mut consensus_cflag_groups: Option>> = None; - for entry in parsed.iter() { - if let Some(consensus_cc) = consensus_cc.as_ref() { - assert!(consensus_cc == &entry.arguments[0]) - } else { - consensus_cc = Some(&entry.arguments[0]); + let arg_iter = entry.arguments[1..] + .iter() + .map(|s| s.as_str()) + // Anything after -c is not CFLAGS but concrete input/output stuff. + .take_while(|&s| s != "-c" && s != "-MQ"); + // Heuristically grouping them to drop different arguments as whole group + let mut cflag_groups = vec![]; + for mut arg in arg_iter { + if arg.starts_with("-I") { + // -I arguments are given inconsistently with and without trailing slashes; + // removing them keeps them from being pruned from the consensus set + arg = arg.trim_end_matches('/'); } - let arg_iter = entry.arguments[1..] - .iter() - .map(|s| s.as_str()) - // Anything after -c is not CFLAGS but concrete input/output stuff. - .take_while(|&s| s != "-c" && s != "-MQ"); - // Heuristically grouping them to drop different arguments as whole group - let mut cflag_groups = vec![]; - for mut arg in arg_iter { - if arg.starts_with("-I") { - // -I arguments are given inconsistently with and without trailing slashes; - // removing them keeps them from being pruned from the consensus set - arg = arg.trim_end_matches('/'); - } - if arg.starts_with('-') { - cflag_groups.push(vec![arg]); - } else { - cflag_groups - .last_mut() - .expect("CFLAG options all start with a dash") - .push(arg); - } + if arg.starts_with('-') { + cflag_groups.push(vec![arg]); + } else { + cflag_groups + .last_mut() + .expect("CFLAG options all start with a dash") + .push(arg); } - if let Some(consensus_cflag_groups) = consensus_cflag_groups.as_mut() { - if &cflag_groups != consensus_cflag_groups { - // consensus is in a good ordering, so we'll just strip it down - *consensus_cflag_groups = consensus_cflag_groups - .drain(..) - .filter(|i| { - let mut keep = cflag_groups.contains(i); - // USEMODULE_INCLUDES are sometimes not in all of the entries; see note - // on brittleness above. - keep |= i[0].starts_with("-I"); - // Left as multiple lines to ease hooking in with debug statements when - // something goes wrong again... - keep - }) - .collect(); - // Hot-fixing the merging algorithm to even work when an (always to be kept) -I - // is not in the initial set - for group in cflag_groups.drain(..) { - if group[0].starts_with("-I") { - if !consensus_cflag_groups.contains(&group) { - consensus_cflag_groups.push(group); - } + } + if let Some(consensus_cflag_groups) = consensus_cflag_groups.as_mut() { + if &cflag_groups != consensus_cflag_groups { + // consensus is in a good ordering, so we'll just strip it down + *consensus_cflag_groups = consensus_cflag_groups + .drain(..) + .filter(|i| { + let mut keep = cflag_groups.contains(i); + // USEMODULE_INCLUDES are sometimes not in all of the entries; see note + // on brittleness above. + keep |= i[0].starts_with("-I"); + // Left as multiple lines to ease hooking in with debug statements when + // something goes wrong again... + keep + }) + .collect(); + // Hot-fixing the merging algorithm to even work when an (always to be kept) -I + // is not in the initial set + for group in cflag_groups.drain(..) { + if group[0].starts_with("-I") { + if !consensus_cflag_groups.contains(&group) { + consensus_cflag_groups.push(group); } } } - } else { - consensus_cflag_groups = Some(cflag_groups); } + } else { + consensus_cflag_groups = Some(cflag_groups); } - cc = consensus_cc - .expect("Entries are present in compile_commands.json") - .to_string(); - cflags = shlex::try_join(consensus_cflag_groups.unwrap().iter().flatten().map(|s| *s)) - .expect("Input is not expected to contain NUL characters"); - - let usemodule = { - #[cfg(not(feature = "riot-rs"))] - { - println!("cargo:rerun-if-env-changed=RIOT_USEMODULE"); - env::var("RIOT_USEMODULE").expect(&format!( - "RIOT_USEMODULE is required when {} is given", - &compile_commands_json, - )) - } - #[cfg(feature = "riot-rs")] - { - println!("cargo:rerun-if-env-changed=DEP_RIOT_BUILD_DIR"); - let riot_builddir = - env::var("DEP_RIOT_BUILD_DIR").expect("DEP_RIOT_BUILD_DIR unset?"); - get_riot_var(&riot_builddir, "USEMODULE") - } - }; - - for m in usemodule.split(" ") { - // Hack around https://github.com/RIOT-OS/RIOT/pull/16129#issuecomment-805810090 - write!( - cflags, - " -DMODULE_{}", - m.to_uppercase() - // avoid producing MODULE_BOARDS_COMMON_SAMDX1-ARDUINO-BOOTLOADER - .replace('-', "_") - ) - .unwrap(); + } + cc = consensus_cc + .expect("Entries are present in compile_commands.json") + .to_string(); + cflags = shlex::try_join(consensus_cflag_groups.unwrap().iter().flatten().map(|s| *s)) + .expect("Input is not expected to contain NUL characters"); + + let usemodule = { + #[cfg(not(feature = "riot-rs"))] + { + println!("cargo:rerun-if-env-changed=RIOT_USEMODULE"); + env::var("RIOT_USEMODULE").expect(&format!( + "RIOT_USEMODULE is required when {} is given", + &compile_commands_json, + )) } - } else { - cc = env::var("RIOT_CC") - .expect("Please pass in RIOT_CC; see README.md for details.") - .clone(); - cflags = env::var("RIOT_CFLAGS") - .expect("Please pass in RIOT_CFLAGS; see README.md for details."); + #[cfg(feature = "riot-rs")] + { + println!("cargo:rerun-if-env-changed=DEP_RIOT_BUILD_DIR"); + let riot_builddir = env::var("DEP_RIOT_BUILD_DIR").expect("DEP_RIOT_BUILD_DIR unset?"); + get_riot_var(&riot_builddir, "USEMODULE") + } + }; + + for m in usemodule.split(" ") { + // Hack around https://github.com/RIOT-OS/RIOT/pull/16129#issuecomment-805810090 + write!( + cflags, + " -DMODULE_{}", + m.to_uppercase() + // avoid producing MODULE_BOARDS_COMMON_SAMDX1-ARDUINO-BOOTLOADER + .replace('-', "_") + ) + .unwrap(); } // pass CC and CFLAGS to dependees From 0248e7d9e35d9e2933ae3ff784f1e415cfc81c6f Mon Sep 17 00:00:00 2001 From: chrysn Date: Wed, 31 Jan 2024 16:02:52 +0100 Subject: [PATCH 5/6] Don't `links="riot-sys"` any more The CC/CFLAGS are thus not exported any more (riot-wrappers doesn't need them since [79]), and the modules list is not neede any more because it was only taken in to be forwarded. [79]: https://github.com/RIOT-OS/rust-riot-wrappers/pull/79 --- Cargo.toml | 2 -- build.rs | 35 ----------------------------------- 2 files changed, 37 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 48ccb92..63d5839 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,8 +11,6 @@ keywords = ["riot", "riot-os", "iot", "bindings"] categories = ["external-ffi-bindings", "no-std"] license = "LGPL-2.1" -links = "riot-sys" - [dependencies] cty = "^0.2" c2rust-asm-casts = "0.2" diff --git a/build.rs b/build.rs index 551d07d..6b43254 100644 --- a/build.rs +++ b/build.rs @@ -123,41 +123,6 @@ fn main() { cflags = shlex::try_join(consensus_cflag_groups.unwrap().iter().flatten().map(|s| *s)) .expect("Input is not expected to contain NUL characters"); - let usemodule = { - #[cfg(not(feature = "riot-rs"))] - { - println!("cargo:rerun-if-env-changed=RIOT_USEMODULE"); - env::var("RIOT_USEMODULE").expect(&format!( - "RIOT_USEMODULE is required when {} is given", - &compile_commands_json, - )) - } - #[cfg(feature = "riot-rs")] - { - println!("cargo:rerun-if-env-changed=DEP_RIOT_BUILD_DIR"); - let riot_builddir = env::var("DEP_RIOT_BUILD_DIR").expect("DEP_RIOT_BUILD_DIR unset?"); - get_riot_var(&riot_builddir, "USEMODULE") - } - }; - - for m in usemodule.split(" ") { - // Hack around https://github.com/RIOT-OS/RIOT/pull/16129#issuecomment-805810090 - write!( - cflags, - " -DMODULE_{}", - m.to_uppercase() - // avoid producing MODULE_BOARDS_COMMON_SAMDX1-ARDUINO-BOOTLOADER - .replace('-', "_") - ) - .unwrap(); - } - - // pass CC and CFLAGS to dependees - // this requires a `links = "riot-sys"` directive in Cargo.toml. - // Dependees can then access these as DEP_RIOT_SYS_CC and DEP_RIOT_SYS_CFLAGS. - println!("cargo:CC={}", &cc); - println!("cargo:CFLAGS={}", &cflags); - println!("cargo:rerun-if-changed=riot-bindgen.h"); let cflags = shlex::split(&cflags).expect("Odd shell escaping in RIOT_CFLAGS"); From c69e3d40618100fde1b4ba909036e55489bdd071 Mon Sep 17 00:00:00 2001 From: chrysn Date: Wed, 31 Jan 2024 16:10:19 +0100 Subject: [PATCH 6/6] Refactor build.rs --- Cargo.toml | 1 - build.rs | 24 ++++++++++-------------- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 63d5839..4622c3d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,7 +23,6 @@ riot-rs-core = { version = "< 0.2.0", optional = true } [build-dependencies] bindgen = "^0.64" -shlex = "^1.3" serde_json = "1" serde = { version = "1", features = [ "derive" ] } regex = "1" diff --git a/build.rs b/build.rs index 6b43254..c07f032 100644 --- a/build.rs +++ b/build.rs @@ -1,5 +1,4 @@ extern crate bindgen; -extern crate shlex; use bindgen::builder; use std::env; @@ -9,9 +8,6 @@ use std::path::PathBuf; use serde_json::json; fn main() { - let cc; - let mut cflags; - #[cfg(not(feature = "riot-rs"))] if env::var("BUILDING_RIOT_RS").is_ok() { println!(""); @@ -117,19 +113,19 @@ fn main() { consensus_cflag_groups = Some(cflag_groups); } } - cc = consensus_cc + let cc = consensus_cc .expect("Entries are present in compile_commands.json") .to_string(); - cflags = shlex::try_join(consensus_cflag_groups.unwrap().iter().flatten().map(|s| *s)) - .expect("Input is not expected to contain NUL characters"); println!("cargo:rerun-if-changed=riot-bindgen.h"); - let cflags = shlex::split(&cflags).expect("Odd shell escaping in RIOT_CFLAGS"); - let cflags: Vec = cflags - .into_iter() - .filter(|x| { - match x.as_ref() { + let cflags: Vec<&str> = consensus_cflag_groups + .unwrap() + .iter() + .flatten() + .map(|v| *v) + .filter(|&x| { + match x { // These will be in riotbuild.h as well, and better there because bindgen emits // consts for data from files but not from defines (?) x if x.starts_with("-D") => false, @@ -339,9 +335,9 @@ fn main() { panic!("riot-sys only accepts clang style CFLAGS. RIOT can produce them using the compile_commands tool even when using a non-clang compiler, such as GCC."); }; - let arguments: Vec<_> = core::iter::once("any-cc".to_string()) + let arguments: Vec<_> = core::iter::once("any-cc") .chain(cflags.into_iter()) - .chain(core::iter::once(c2rust_infile.to_string())) + .chain(core::iter::once(c2rust_infile)) .collect(); let compile_commands = json!([{ "arguments": arguments,