RSDK-1975 - autogen header file #156
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,3 +8,5 @@ bin/ | |
|
|
||
| # goutils | ||
| tests/goutils/ | ||
|
|
||
| viam_rust_utils.h | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| extern crate cbindgen; | ||
|
|
||
| use std::{collections::HashMap, env}; | ||
|
|
||
| use cbindgen::{Config, ExportConfig, FunctionConfig, ParseConfig, RenameRule, StructConfig}; | ||
|
|
||
| fn main() { | ||
| let crate_dir = env::var("CARGO_MANIFEST_DIR").unwrap(); | ||
|
|
||
| // mangled nalgebra type outputs, mapped to a prefixed snake case variant | ||
| let nalgebra_types = vec![ | ||
| ("ArrayStorage_f64__3__1", "nalgebra_array_storage_f64__3__1"), | ||
| ( | ||
| "Matrix_f64__U3__U1__ArrayStorage_f64__3__1", | ||
| "nalgebra_matrix_f64_3_1", | ||
| ), | ||
| ("Vector3_f64", "nalgebra_vector3_f64"), | ||
| ("ArrayStorage_f64__4__1", "nalgebra_array_storage_f64__4__1"), | ||
| ( | ||
| "Matrix_f64__U4__U1__ArrayStorage_f64__4__1", | ||
| "nalgebra_matrix_f64_4_1", | ||
| ), | ||
| ("Vector4_f64", "nalgebra_vector4_f64"), | ||
| ("Quaternion_f64", "nalgebra_quaternion_f64"), | ||
| ( | ||
| "Unit_Matrix_f64__U3__U1__ArrayStorage_f64__3__1", | ||
| "nalgebra_unit_matrix_f64_3_1", | ||
| ), | ||
| ("UnitVector3_f64", "nalgebra_unit_vector3_f64"), | ||
| ("Rotation_f64__3", "nalgebra_rotation_f64_3"), | ||
| ( | ||
| "Matrix_f64__Const_3_____Const_3_____ArrayStorage_f64__3__3", | ||
| "nalgebra_matrix_f64_3_3", | ||
| ), | ||
| ("ArrayStorage_f64__3__3", "nalgebra_array_storage_f64__3__3"), | ||
| ("SMatrix_f64__3__3", "nalgebra_smatrix_f64_3_3"), | ||
| ("Rotation_f64__3", "nalgebra_rotation_f64_3"), | ||
| ("Rotation3_f64", "nalgebra_rotation3_f64"), | ||
| ]; | ||
|
|
||
| // mangled viam type outputs, mapped to a prefixed snake case variant | ||
| let viam_types = vec![ | ||
| ("DialFfi", "viam_dial_ffi"), | ||
| ("AxisAngle", "viam_axis_angle"), | ||
| ("EulerAngles", "viam_euler_angles"), | ||
| ("OrientationVector", "viam_orientation_vector"), | ||
| ]; | ||
|
|
||
| let rename = vec![nalgebra_types, viam_types] | ||
| .into_iter() | ||
| .flatten() | ||
| .map(|(a, b)| (a.to_string(), b.to_string())) | ||
| .collect::<HashMap<String, String>>(); | ||
|
|
||
| cbindgen::Builder::new() | ||
| .with_crate(crate_dir) | ||
| .with_config(Config { | ||
| parse: ParseConfig { | ||
| parse_deps: true, | ||
| include: Some(vec!["nalgebra".to_string()]), | ||
| ..Default::default() | ||
| }, | ||
| export: ExportConfig { | ||
| rename, | ||
| ..Default::default() | ||
| }, | ||
| structure: StructConfig { | ||
| rename_associated_constant: RenameRule::SnakeCase, | ||
| rename_fields: RenameRule::SnakeCase, | ||
| ..Default::default() | ||
| }, | ||
| function: FunctionConfig { | ||
| deprecated: Some( | ||
| "/// @deprecated please use `viam_`-prefixed function instead".to_string(), | ||
| ), | ||
| ..Default::default() | ||
| }, | ||
| ..Default::default() | ||
| }) | ||
| .with_cpp_compat(true) | ||
| .with_language(cbindgen::Language::C) | ||
| .with_autogen_warning( | ||
| "/* Warning, this file is autogenerated by cbindgen. Don't modify this manually. */", | ||
| ) | ||
| .generate() | ||
| .expect("Unable to generate bindings") | ||
| .write_to_file("viam_rust_utils.h"); // Output to target directory | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
possibly overly defensive but I opted for unique header files for each architecture in case they differ in includes or otherwise.
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.
maybe cc @acmorrow but I think we're fine just using one header for all of them. i'm not sure the level of control you have over the generated code, but if there are indeed differing includes this is something you'd want to do with a single header file that has
#ifdefblocksThere 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.
Yes, client needs to be able to name one include and get the right things. If there are architecture dependencies, there should be a per-arch header and then they should be unified with a top-level header.
A suggestion if you are worried: generate for each platform, then checksum or diff them. If they compare identical, just ship the first one. If they differ, fail the build.
Then you don't need a wrapper header at all, unless and until a divergence emerges.
Uh oh!
There was an error while loading. Please reload this page.
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.
Hmm unfortunately it's a bit awkward to force in more complicated code blocks to the generated code, not impossible but might be awkward.
I do think though that having multiple headers isn't too bad (at least for the SDK), since we can just enforce that the right one is downloaded in CMakeLists.txt
edit: this comment was made before seeing Drew's reply above!)
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.
Hmm, a question on that front just for my own understanding. Are we planning on checking in the header file to the repo? If so, what happens if someone has their own copy of the
rust_utilsbinary that they want to use (a thing we currently support), but it differs in some way from the checked in header? And if we're not checking it in but rather downloading it when we download therust_utilsbinary (or expecting a user to provide their own along with their ownrust_utilsbinary) then what's the danger with having that download seek out the architecture-specific header and then save it as to the generic.../rust_utils.hpath?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.
From my perspective, there really shouldn't be any architecture dependent code in this header. It should be identical across all. If architecture variation slipped in, I'd consider that an error.
Shifting the burden to the consumer to select the right header seems somewhat unfair.
Regarding whether we will check this in, my expectation is that no, we wouldn't. I'd instead expect to see the one header published along with the release.
That's why my suggestion was to verify at build time that the header is bit identical on every platform where we generate it. That way, we will notice if they ever diverge, and can publish any one of them as "the header".