feat: provide a usable include directory#111
Conversation
7cfdc90 to
9a198b3
Compare
21bd72d to
e21bf0f
Compare
There was a problem hiding this comment.
Pull request overview
This PR makes the generated/installed MbedTLS include directory self-contained by generating and installing the effective mbedtls_config.h (and hook _alt headers) during the build, so bindgen and downstream consumers don’t need extra -D flags to get the correct configuration.
Changes:
- Generate an MbedTLS config header (
mbedtls/mbedtls_config.h) in the build process and merge it into the installed include tree. - Rework the build/bindgen pipeline to use the built artifacts’ include directory (instead of passing config/hook defines via clang args).
- Add generated hook
_altheaders and corresponding*_WORK_AREA_SIZEconfig defines; addregexas a build dependency for config parsing.
Reviewed changes
Copilot reviewed 12 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| xtask/src/main.rs | Uses compile() artifacts to pass the built include dir into bindings generation. |
| xtask/Cargo.toml | Adds/reorders deps needed by the shared builder/config generation code. |
| mbedtls-rs-sys/build.rs | Updates build flow to use new MbedtlsArtifacts (include dir + libraries). |
| mbedtls-rs-sys/Cargo.toml | Adds build dependency on regex for config parsing/generation. |
| mbedtls-rs-sys/gen/builder.rs | Introduces config generation, staging include dir, and returns compilation artifacts including the include path. |
| mbedtls-rs-sys/gen/config.rs | New config parser/renderer for mbedtls_config.h driven by regex. |
| mbedtls-rs-sys/gen/include/include.h | Removes MBEDTLS_CONFIG_FILE indirection; relies on the installed config header. |
| mbedtls-rs-sys/gen/include/config.h | Removes the previously checked-in generated config header. |
| mbedtls-rs-sys/gen/hook/sha1_alt.h | Adds generated SHA1 ALT context header using a work-area size macro. |
| mbedtls-rs-sys/gen/hook/sha256_alt.h | Adds generated SHA256 ALT context header using a work-area size macro. |
| mbedtls-rs-sys/gen/hook/sha512_alt.h | Adds generated SHA512 ALT context header using a work-area size macro. |
| mbedtls-rs-sys/src/include/*.rs | Updates pre-generated bindings to reflect the new config/hook defines being baked into headers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mbedtls-rs-sys/gen/config.rs
Outdated
| // - `value`: The value of the config option, if it has one. | ||
| // | ||
| // See: <https://regex101.com/r/u5XNfA/1> | ||
| Regex::new( |
There was a problem hiding this comment.
I guess one way to generate our own config file is this way - I would call it - the regexp parsing way - i.e. parse the "default" mbedtls config.h into a bunch of key-value pairs, and then add/update/remove these.
And alternative - I think arguably simpler - way - would be to do it the "C way". I.e., generate a config.h file that:
#includes the standardmbedtls_config.hone as its first statement- For each removed config (not that we remove configs so far), put an
#undefstatement - For each added/updated config, put an
#undef+#definepair of statements
So in a way, why bother regexp-parsing the default mbedtls config in the first place, when we can "update" it using pure C preprocessor means?
There was a problem hiding this comment.
That's a great point. I got "inspired" (or rather carried away by) how MbedTLS itself provides a config editor :D
The only potential argument for parsing the config I could make is that it adds a level of validation. Because the set function panics if you try to set a config value that doesn't exist, we know that all the config options we're setting are understood by MbedTLS.
The "C way" approach cannot possibly check whether a config option is understood by MbedTLS if it's commented out by default.
It definitely sounds like the better approach considering long-term maintainability, so I'm happy to switch to it.
There was a problem hiding this comment.
Although note that we'll still have to actually replace the original config file, otherwise we're right back to having to provide compiler definitions everywhere.
So what I would end up doing is using MBEDTLS_USER_CONFIG_FILE while compiling mbedtls and afterwards concatenating it to the end of the original header.
There was a problem hiding this comment.
If you mean that the current, static config.h file should go - I agree - it needs to be replaced with a dynamically-generated one either way.
I'm just saying we can generate the config.h file by just having:
#include <...path-to-mbedtls_default_config_file.h>
// Here all defines due to hooks or because our build deviates from
// the default settings of the default config file follow - auto-generated
// Remove feature FOO, which is enabled by default
#ifdef HAVE_FOO
#undef HAVE_FOO
#endif
// Add/update feature BAR
#ifdef HAVE_BAR
#undef HAVE_BAR
#endif
#define HAVE_BARThere was a problem hiding this comment.
I understand your point on how we should generate the config file and I'm happy to go with that approach. I believe we're not quite on the same page yet when it comes to why we need to generate the config file and why I'm saying we need to replace the default config file (mbedtls/mbedtls_config.h).
The entire point of this PR is to make it such that another project (e.g. openthread) can safely compile against this library. This means the other project will link against our archive files and pass our include directory to the compiler.
Let's say we don't replace the mbedtls_config.h file and instead create a custom_mbedtls_config.h that includes #include "mbedtls_config.h" at the top. Now, we have to explicitly pass -D MBEDTLS_CONFIG_FILE=/path/to/custom_mbedtls_config.h to every compilation unit that even remotely touches mbedtls - including openthread.
If we forget to pass the define to the compiler, openthread compiles against the default config of mbedtls, not our custom one. This will almost certainly result in headaches.
Instead of forcing every consumer to be aware of this pitfall, my plan is to make it such that compiling against the default config (which we replace) is the correct approach. That way we completely avoid the need for any compiler flags.
I don't have a vendetta against the the current config.h file. I just want to make it safe and convenient for openthread to compile against this library.
Generating the config file is only part of that because we're dynamically changing the config based on hooks.
There was a problem hiding this comment.
I believe we're not quite on the same page yet when it comes to why we need to generate the config file and why I'm saying we need to replace the default config file (
mbedtls/mbedtls_config.h).
I think we are on the same page as to why, I'm just not sure we align on the definition of "how to replace it". See below.
Instead of forcing every consumer to be aware of this pitfall, my plan is to make it such that compiling against the default config (which we replace) is the correct approach. That way we completely avoid the need for any compiler flags.
Sure.
By "replacing" I guess you mean that all occurrences of #include "mbedtls/mbedtls_config.h" inside the MbedTLS source code should somehow resolve to our custom config file, which obviously needs to be named mbedtls_config.h then and obviously needs to reside in a directory named mbedtls.
My only concern with the current approach is that you might be copying too many .h files around?
Isn't it enough to just create an OUT_DIR/extra-includes/mbedtls/mbedtls_config.h file and then make sure that -I$OUT_DIR/extra-includes is the first include directory before all others in the list of include directories?
There was a problem hiding this comment.
Isn't it enough to just create an
OUT_DIR/extra-includes/mbedtls/mbedtls_config.hfile and then make sure that-I$OUT_DIR/extra-includesis the first include directory before all others in the list of include directories?
True, that works. I'm a bit uncomfortable with how brittle this approach is though considering that messing up the order of include directories silently breaks it. I think we also need to rely on something like #include_next to ensure our generated config file can include the original header without accidentally including itself.
My current preference would be to do it like this:
- Generate the config header like you suggested and write it to a directory in $OUT.
- Compile mbedtls with the
MBEDTLS_USER_CONFIG_FILEset to this generated file. The "user" config is automatically included after including the default config. See: https://github.com/Mbed-TLS/mbedtls/blob/1d088bb84e81af8e457cefe99a6060b8d7e66ad0/include/mbedtls/build_info.h#L50-L69 - Now take the config header and append it to the end of
$OUT/include/mbedtls/mbedtls_config.h(This emulates the effect of settingMBEDTLS_USER_CONFIG_FILE, but without relying on a define). - We can use your suggestion of the separate include directory to avoid copying the hook alt headers (like
sha256_alt.h). This feels "safe" to me because the hooks don't override any existing headers so the order of includes doesn't matter.
With this we no longer need to copy around anything. The only thing we end up doing is a simple concatenation of two files.
There was a problem hiding this comment.
Alright. Just do it as you feel being most appropriate.
I think you are currently much deeper into the details so I trust your judgement and don't want to further micro-manage this stuff as that would bring more harm than good, I feel.
Just let me know when ready.
BTW - shouldn't we - in parallel with this PR - also open a PR against openthread that kinda "consumes" the fruits of this one and makes OT capable of using an external MbedTLS instance? I'm sure you are actually having this guy already, just not as a PR? You know - to make sure that we are not missing anything important that OT would need so that it could link to/use an external MbedTLS, and that might require significant changes to the approach?
There was a problem hiding this comment.
I think you are currently much deeper into the details so I trust your judgement and don't want to further micro-manage this stuff as that would bring more harm than good, I feel.
To be clear, I really appreciated your feedback here. The whole config regex parsing aspect was definitely a "kill your darlings" kind of deal. I'm only pushing back on some aspects because I've lost a lot of hours debugging issues related to -sys crates so I prefer to make sure there's as little room for errors as possible.
BTW - shouldn't we - in parallel with this PR - also open a PR against openthread that kinda "consumes" the fruits of this one and makes OT capable of using an external MbedTLS instance? I'm sure you are actually having this guy already, just not as a PR? You know - to make sure that we are not missing anything important that OT would need so that it could link to/use an external MbedTLS, and that might require significant changes to the approach?
Agreed. I've been a bit busy with other work this week, but I have a version of openthread that at least compiles using this library. I still need to actually test it on a device though to make sure it doesn't just crash and burn immediately :)
As soon as I have some time to work on this again I'll open a draft PR on openthread so we can see what it looks like.
There was a problem hiding this comment.
@ivmarkov I've now switched to the approach I outlined in #111 (comment).
I've also gone ahead and created the draft PR in the openthread repo here: esp-rs/openthread#64
As of yet I unfortunately don't have a real workload to test this on, but at least the examples are working as expected.
e21bf0f to
1e47789
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
1e47789 to
4194b36
Compare
4194b36 to
bc7a819
Compare
|
@scootermon Seems this branch/PR has conflicts. Could you resolve these so I can merge? |
bc7a819 to
aa544a6
Compare
To facilitate the use of MbedTLS headers by other crates (specifically those linking against libraries that depend on MbedTLS), we now expose the `include` Cargo metadata. This commit also refactors the build process to ensure we don't have to rely on compile-time definitions for the headers to work correctly. See: <esp-rs#109>
aa544a6 to
374bce7
Compare
The include directory currently requires compiler flags to work correctly. Without MBEDTLS_CONFIG_FILE defined, headers use MbedTLS's default configuration, causing link failures or runtime crashes. Hooks also require custom flags.
We now dynamically generate the MbedTLS configuration header in the build script instead of passing options via -D flags. This makes the include directory usable without the need for any compiler flags.
Bindgen can now run against this include directory without custom flags.
See: #109
Note that this PR does not yet fully resolve the aforementioned issue. We still have to tackle the case when using the pre-compiled libraries, which currently don't expose the include directory.