bindgen-cli: alternate use of bindgen - v1 #12139
Draft
Conversation
This file contains 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
jasonish
force-pushed
the
bindgen-cli/v1
branch
3 times, most recently
from
b64eeb7
to
4869aff
Compare
jasonish
commented
Nov 20, 2024
Comment on lines
+2273
to
+2276
| AC_PATH_PROG([BINDGEN], [bindgen], [no]) | ||
| if test "x$BINDGEN" = "xno"; then | ||
| AC_MSG_ERROR([bindgen required]) | ||
| fi |
There was a problem hiding this comment.
The idea is that when building from a distribution archive this wouldn't be required. Just like cbindgen.
jasonish
force-pushed
the
bindgen-cli/v1
branch
2 times, most recently
from
eac1b70
to
e4ccf11
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12139 +/- ##
===========================================
+ Coverage 49.77% 75.05% +25.27%
===========================================
Files 909 848 -61
Lines 257884 155587 -102297
===========================================
- Hits 128367 116775 -11592
+ Misses 129517 38812 -90705
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
ERROR: ERROR: QA failed on build_asan. Pipeline 23468 |
jasonish
force-pushed
the
bindgen-cli/v1
branch
from
e4ccf11
to
1d5f58c
Compare
|
ERROR: ERROR: QA failed on build_asan. Pipeline 23469 |
Add a minimal integration of bindgen to the build. Bindgen is integrated at compile time with a "build.rs" file that for now only generates AppLayerEventType Rust bindings. This required some refactoring of the C so app-layer-events.h did not also include "rust.h", which causes issues for bindgen, probably related to circular references. AppLayerEventType was chosen as the first step as its an argument type some app-layer functions that we may want to use bindgen to export Rust, and one of the requirements of bindgen might be that C functions should only use datatypes defined in C, and not Rust. Following such a rule also prevents circular dependencies between Rust and C code. Bindgen generates the bindings in a file named "bindings.rs" in the target/ direction, which "sys.rs" will statically "include" at compiling name, making these bindings available under package "crate::sys". "build.rs" had to be placed in the non-standard location of "src/build.rs" (its usually alongside Cargo.toml) to satisfy the out-of-tree build requirements of distcheck. Note that bindgen is also available as a command line tool which could be used instead of integrating this at compile time, however the tool requires a newer version of Rust than our MSRV, and may present additional issues with respect to autoconf and distcheck.
Follow the naming scheme for public exports.
This lets us remove decode.h from app-layer-events.h as pulling in app-layer-events.h shouldn't result in pulling in dpdk, and other includes not related to app-layer-events. decode.h also doesn't need those forward declarations anymore due to previous changes.
Instead of defining this function pointer in type in Rust, and having it in C signatures, create a type and export it to Rust. To facilitate this, and new header has been creates, "app-layer-types.h", this is to avoid the circular reference of C headers pulling in "rust.h" which are required to generate Rust bindings.
This exposes the C define ALPROTO values to Rust without having to perform some runtime initialization with init_ffi. As app-layer-protos.h was clean of a circular reference to rust.h we could use it directly, it just needed the addition of suricata-common.h.
jasonish
force-pushed
the
bindgen-cli/v1
branch
4 times, most recently
from
587acd0
to
3f1f490
Compare
Replace bindgen usage from build.rs with the bindgen-cli program much like we use cbindgen. As bindgen can only accept one header file, we construct a pseudo-header file of all the headers that need to be consumed. Makefile dependency checking is done to make sure the pseudo-header is only generated as needed to avoid rebuilding every time. Special handling is required for Windows to use the Windows path.
The bindgen found in Ubuntu 24.04 and older is not new enough for --allowlist-item.
jasonish
force-pushed
the
bindgen-cli/v1
branch
from
3f1f490
to
4e75afe
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Like #12062, but uses the bindgen-cli tool.
Currently requires it to exist, but the idea is that a distribution archive would provide the bindings, meaning users of the release would not require bindgen to be install (and the clang tooling it requires).
Draft as I'm figuring out some Windows issue, also an issue on the other PR.