External components discussion

[MabezDev]

Getting esp-idf-sys to build external components is relatively simple, I have PoC locally building esp-rainmaker. The hard part is how we tell esp-idf-sys where these external components are and what includes we need. Essentially the inter-crate communication.

It seems there is no way (unless I'm missing something, and I hope I am because it would make this really easy) in cargo to set environment variables for dependent crates; therefore a separate crate that depends on esp-idf-sys is out of the question because we can't give esp-idf-sys the information it needs to build these external components.

Option 1 - env variables in the final application

We already set custom variables to dictate the build, added something like ESP_IDF_EXTRA_COMPONENT_PATHS could work. The downside to this is that its yet another thing a user has to remember to include, and if its the case we have a separate wrapper crate for some external components we will get weird missing item errors when we try and build that crate if we forget to add our extra component variable

Option 2 - esp-idf-sys becomes monolithic

Instead of being able to customize the build process, the way to add new components would be to add them at the source level in esp-idf-sys. esp-idf-sys will then take care of downloading the external component and adding it into its normal build process. To save build time, it might be a good idea to put these external components behind feature flags.

There may be better options I haven't considered, I won't lie I'm not an expert in esp-idf-sys's build system hence why I am opening this 😅.

[ivmarkov]

I think you are right in that the only option to pass info to downstream crates is by using environment variables, or the [env] section in .cargo/config.toml which would imply that this has to happen at the binary crate level.

In fact, they closed the original issue referenced above and another one which was spawned from it once the [env] section got implemented in .cargo/config.toml.

As for Option 1 vs 2 - I'm fine with both, and both should probably be implemented:

• Option 2 - for stuff like RainMaker and probably for drivers like the esp32-camera ones
• Option 1 would still be useful as a backup for ad-hoc stuff that the user wants to plug into the build.

Perhaps you can start with Option 2, and then I or @N3xed can look into Option 1?

[N3xed]

therefore a separate crate that depends on esp-idf-sys is out of the question because we can't give esp-idf-sys the information it needs to build these external components.

Yup, that's right.

I think there are two types of components (as I see them):

• esp-idf components
• External libraries that may or may not use esp-idf APIs and are not written as esp-idf components or are trivial to convert to normal cmake libraries

And two use cases:

• Build the component in the build script of a separate crate
• Tell esp-idf-sys to build it

I don't think there is a general way to compile the extra components separately (i.e. in the build script of a separate crate). If such a component uses esp-idf's kconfig configuration system then there is really no nice way to do this. Additionally, I think we'd have to hack on the esp-idf build system quite a bit to emulate its component registration and building to make this work.
So having a general way to achieve this is not worth it.

But then the only thing a separate crate for such a component can do is generate the bindings, which I guess would only be worth it for really big components if at all.

External libraries, that don't use esp-idf's component mechanism or are trivial to convert to normal cmake library targets, are much easier to build in a build script of a separate crate. This is actually what I'm referring to with my response here, which in hindsight may not be that obvious. But again for simple libraries, I think that approach is pretty straightforward and also uses a separate crate.

With that said:

Option 1 - env variables in the final application
We already set custom variables to dictate the build, added something like ESP_IDF_EXTRA_COMPONENT_PATHS could work.

I think this is our only option for esp-idf extra components.
We could also use the [metadata] section in the Cargo.toml for this, but since we use environment variables for everything else we should use them there too.

The downside to this is that its yet another thing a user has to remember to include, and if its the case we have a separate wrapper crate for some external components we will get weird missing item errors when we try and build that crate if we forget to add our extra component variable

But this can very easily be prevented, by checking in the build script of such a wrapper crate that this component exists. (For example by checking that environment variable.) We could even export all the extra (or even all) component names/paths that were built in esp-idf-sys for more consistency.

Option 2 - esp-idf-sys becomes monolithic

We could do this too, but I think this approach should only be used for very select components that most users need. So that we don't end up with 20 features for different components.

and then I or @N3xed can look into Option 1?

Sure, let me come up with something for Option 1, if you think my reasoning here is sound.

[MabezDev]

Thanks for the input folks, I agree that we should probably implement both and for rainmaker I will submit a PR to esp-idf-sys soon.

Maybe one day we'll get the ability to pass env variables through Cargo.toml, something like

esp-idf-sys = { version = "x.y.z", env = [{name = "EXTRA_COMPONENTS", value = "/path/to/extra/components"}]}

[N3xed]

Okay, an update for this issue (and sorry for the delay), especially regarding building extra components configured by the user in esp-idf-sys (Option 1):

Getting esp-idf-sys to build such a component is pretty trivial. There are two approaches to specifying extra components:

• Specify each component separately in a list (wouldn't be pretty with env variables => cargo metadata).
• Specify only one directory where all components are contained (simple and nice with env variables).

The second approach is also the one @MabezDev suggested, and really the only one we need right now, so I'll go with that.

The bigger problem is with bindings generation. Here the question is whether esp-idf-sys should also generate the bindings for the extra components.
I think it should be able to do that, but in such a way that we allow the user to make their own crate for the bindings if they want to.

How do we specify the headers used for bindings generation though? I see three approaches:

1. Use all headers (i.e. all *.h files in the component dir).
2. The user has to list the headers that they want bindings for.
3. The components directory (directory that contains the components) must contain a specially named header that esp-idf-sys uses for bindings generation (e.g. bindgen.h).

(1) is bad because we would need to somehow turn bindings generation off for some packages if we want users to have custom bindings crates for their components (to reduce compilation overhead). And the user probably doesn't want bindings for all headers in the directory.
(2) has again the problem that specifying a list of headers per component in 1 (or more) environment variables is ugly (especially with many such components). Better would be to use cargo metadata for this, e.g.:

[package.metadata.esp-idf-sys.extra-components.foo]
headers = ["header1.h"]

(3) is the simplest approach and easiest to implement. If the file does not exist and we have extra components we could print a warning? This is the approach I think I'll implement.

If there are no issues, I'll start implementing this.

[MabezDev]

I think the only problem with option 3 is that we have to make invasive changes upstream, i.e for Rust support we need to add a bindings.h with the includes inside which the components will never actually use themselves. Unless you are thinking that we might vendor external component sources inside esp-idf-sys. Seeing as we are going down the monolithic approach, can't we also add the custom bindings.h for a specific component into esp-idf-sys? We could also add a feature flag to turn off binding generation for a custom components.

Take rainmaker as an example, if we add this external component into esp-idf-sys I expect the following changes.

[features]
rainmaker = []
rainmaker_with_custom_bindings = ["rainmaker"]

When rainmaker is enabled, the rainmaker sources are downloaded and registered with idf. If rainmaker_with_custom_bindings is set, bindgen will not run however if it is not set then default bindings will be generated.

[N3xed]

I think the only problem with option 3 is that we have to make invasive changes upstream, i.e for Rust support we need to add a bindings.h with the includes inside which the components will never actually use themselves.

Yes, I thought of that too, but I think if we only have one bindings.h in the components folder, so not in the component folder itself but in its parent folder, then this should work. So one header for the bindings for all extra components.

For example:
test/ - binary crate dir
test/components/ - extra C components directory
test/components/foo/CMakeLists.txt, test/components/foo/test.h - foo component (note: foo folder could be a submodule)
test/components/bindings.h - bindings header the user wrote:

#include "test.h"

test/.cargo/config.toml:

[env]
ESP_IDF_EXTRA_COMPONENTS = "./components" 
# relative to workspace dir, (i.e. `test/`)
# all subfolders of `./components` that are components are automatically added

Seeing as we are going down the monolithic approach, can't we also add the custom bindings.h for a specific component into esp-idf-sys? We could also add a feature flag to turn off binding generation for a custom components.

Since rainmaker is really multiple components and supplement to the esp-idf I think it makes sense to add a custom feature to esp-idf-sys that downloads the rainmaker repo (maybe also next to the esp-idf somewhere, e.g. a libs folder), adds it to the build and generates the bindings. So essentially, that rainmaker is built into esp-idf-sys and enabled with the feature that you suggested.

[ivmarkov]

[package.metadata.esp-idf-sys.extra-components.foo]
headers = ["header1.h"]

Excuse my ignorance, but I do not understand how package metadata in a crate foo that depends on esp-idf-sys can be propagated down to esp-idf-sys?

Please explain.

[N3xed]

Excuse my ignorance, but I do not understand how package metadata in a crate foo that depends on esp-idf-sys can be propagated down to esp-idf-sys?

Nope, sorry for the confusion. This metadata section would then be in the Cargo.toml of the top-level crate (i.e. the binary crate), which is not that much better than using env variables, but allows for specifying arbitrary values (lists, tables, strings, numbers, etc.).

[ivmarkov]

Excuse my ignorance, but I do not understand how package metadata in a crate foo that depends on esp-idf-sys can be propagated down to esp-idf-sys?

Nope, sorry for the confusion. This metadata section would then be in the Cargo.toml of the top-level crate (i.e. the binary crate), which is not that much better than using env variables, but allows for specifying arbitrary values (lists, tables, strings, numbers, etc.).

I don't think even that would work. The top-level crate still falls in the category of "crate foo that depends on esp-idf-sys trying to push configs / talk to esp-idf-sys" which is not supported. (Only the other way around is supported, i.e. esp-idf-sys -> foo - via the library feature / hack.)

IMO the only way forward (as per my original assessment) is to use either plain environment variables, or the [env] section in ./config/cargo.toml which is the same. This approach has nothing to do with the unsupported "crate foo depending on esp-idf-sys and communicating with esp-idf-sys".

It is just a way to set a bunch of environment variables before calling cargo build, and that's why it works.

Also: the fact that we put .cargo/config.toml in the workspace / binary crate is just a mere convenience, but it has nothing to do with a crate-to-crate communication. Perhaps I mislead you when saying - in my original post "this has to happen at the binary crate level". What I really should've written is "via environment variables, which might be set via the [env] section of .cargo/config.toml which the user might choose to put inside the binary crate directory, and keep under source control".

[ivmarkov]

Perhaps getting obvious here, but the only way for "crate foo that depends on esp-idf-sys" to affect the compilation config of esp-idf-sys is via features. Unfortunately, these are just boolean flags, while we need more than that. But more than that is not available, perhaps for good reasons (complexity of providing to esp-idf-sys a coherent configuration when crate A wants something from esp-idf-sys, and crate B wants the opposite).

[N3xed]

It would work though, because the location of Cargo.toml of the top-level crate is deterministic, meaning we know its location in every build script of every dependency, that's how we can communicate with these crates even though in view of the build the dependency of the top-level crate is only one-directional.

Also, and I've just figured this out, just calling cargo metadata on the top level crate would also work, and would get us all metadata (as far as I know) for all packages in the workspace (I don't know if this means all dependencies as well).

[ivmarkov]

Also, and I've just figured this out, just calling cargo metadata on the top level crate would also work, and would get us all metadata (as far as I know) for all packages in the workspace (I don't know if this means all dependencies as well).

Calling cargo metadata from a build.rs script where cargo is already running, might, or might not work, or might crash and/or give unpredictable results. Should be carefully checked. And then you still need some sort of resolution logic, where if contradicting requirements are coming from two different crates, one takes priority, or the build fails.

It would work though, because the location of Cargo.toml of the top-level crate is deterministic, meaning we know its location in every build script of every dependency, that's how we can communicate with these crates even though in view of the build the dependency of the top-level crate is only one-directional.

I agree that if we constrain the user to only specify the meta-data at the top-level binary crate (or workspace), the approach might work, as we already have a hack to figure out the workspace directory, so we can load and parse the Cargo.toml file ourselves. Also, there will be just one place where these are specified, so no need for special resolution logic there.

What I don't like very much is that with this approach we would be introducing asymmetry:

• Half of the esp-idf-sys configuration will be with environment variables
• The other half (building custom components) will be done with Cargo.toml meta-data

And in the end, you still don't have the perfect setup, which would be a separate-crate-per-custom-component for which you need cargo metadata from build.rs to work.

If we take this route (and btw I also much more prefer cargo metadata in build.rs to actually work), then ideally we should have everything which currently works via env vars to also be possible to specify via cargo metadata in the top-level Cargo.toml and the other way around. Where env vars take priority (override) the cargo metadata stuff.

So why don't we:

• Check if cargo metadata works from build.rs as that opens a whole lot of new possibilities
• If not, just do the env vars approach initially, as we have done so far
• Implement a feature parity with the env vars later, in the form of metadata in the top-level binary crate

?

[N3xed]

• Check if cargo metadata works from build.rs as that opens a whole lot of new possibilities

It seems like this indeed works, though with the same caveat that we have to know the Cargo.toml location of the top-level crate (i.e. the workspace directory). And I was partially wrong in saying that its location is deterministic from the target directory point-of-view because the location of the target directory can be changed.
But with our current strategy we already assume that the target directory is directly inside the workspace dir. So everytime the embuild::cargo::workspace-dir function is called this assumption is made (currently), as there is no env variable that cargo sets for the build scripts (see issue rust-lang/cargo#3946). An easy fix for this is to set CARGO_WORKSPACE_DIR = { value = "", relative = true } ourselves in the config.toml of the workspace (last comment of the aforementioned issue).
I think this warrants a note in the README (and maybe a comment on the embuild::cargo::workspace_dir function).

With that said, I agree, we can look into migrating to the metadata section.

• If not, just do the env vars approach initially, as we have done so far
• Implement a feature parity with the env vars later, in the form of metadata in the top-level binary crate

I think I'll make a PR using the metadata section and see how that turns out (specifically for the extra/external components). This will add two more (explicit) dependencies, cargo_metadata and serde_json, though. Going this way would solve the problem with specifying a list of components directories and bindgen header files.

[ivmarkov]

OK so to make sure I understand, we are implementing this bullet:
* Check if cargo metadata works from build.rs as that opens a whole lot of new possibilities

What this means, is that we CAN have separate crates representing ESP-IDF components, right? As in e.g. esp-idf-rainmaker-sys:

• Representing the raw bindings for Rainmaker
• Depending on esp-idf-sys
• Communicating with esp-idf-sys via its metadata written inside its own Cargo.toml (and NOT the Cargo.toml of the main (binary) crate - we need the location of the binary crate (or the workspace of the user) just to call cargo metadata on it and thus get the metadata of esp-idf-rainmaker-sys and other potential *-sys extensions of esp-idf-sys)
• embuild basically mediating in the communication between esp-idf-rainmaker-sys and esp-idf-sys by being invoked by esp-idf-sys and analyzing the metadata of esp-idf-rainmaker-sys using the above approach during its build

Right?