Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DRAFT] Add basic support for IDF Component Registry API client #78

Closed

Conversation

daggerrz
Copy link

@daggerrz daggerrz commented Jul 22, 2023

Since 5.0, some components like mDNS are distributed separately from IDF. I've been exploring approaches to how this can be resolved and am looking for feedback on the general approach before I do more work here. An alternative solution would be to simply use the Python component manager bundled with the IDF build system, but I found that to be more challenging to integrate into the build, than writing one in Rust.

A proof of concept of the Cargo component manager client and download manager is included here in embuild and the integration into the Cargo driver is here: esp-rs/esp-idf-sys#220.

This works for building a project depending on mDNS if ESP_IDF_COMP_MDNS_ENABLED is set in esp-idf-sys and the following is added to the project's Cargo.toml:

[[package.metadata.esp-idf-sys.extra_components]]
component_refs = [ { name = "espressif/mdns", version = "1.2" } ]

This is my first dive into the innards of the build system, so any guidance will be greatly appreciated. I have a lot more questions if the approach is considered sound.

@daggerrz daggerrz changed the title Add basic support for ESP Component manager API client [DRAFT] Add basic support for ESP Component manager API client Jul 22, 2023
@daggerrz daggerrz changed the title [DRAFT] Add basic support for ESP Component manager API client [DRAFT] Add basic support for IDF Component Registry API client Jul 22, 2023
@daggerrz daggerrz marked this pull request as draft July 22, 2023 17:31
@daggerrz
Copy link
Author

@ivmarkov, perhaps you have thoughts on this?

tests/resources/espcomp/comp/component_result.json Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@ivmarkov
Copy link
Collaborator

In general, I think you need the feedback of @N3xed , as he is our expert in the ESP IDF build system.

With that said, and w.r.t. re-using the Python component manager vs re-implementing our own, the latter would make sense only if we expect the component manager not to change every now and then. I have no idea whether that's the case? If you could share your opinion?

I have a few nits w.r.t. Rust code organization that I'll add inline - if you could address.

Copy link
Collaborator

@ivmarkov ivmarkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned - a few nits really. Nothing major.

src/espcomp/mod.rs Outdated Show resolved Hide resolved
src/espcomp/mod.rs Outdated Show resolved Hide resolved
src/espcomp/mod.rs Outdated Show resolved Hide resolved
src/espcomp/mod.rs Outdated Show resolved Hide resolved
@daggerrz
Copy link
Author

As I mentioned - a few nits really. Nothing major.

Thanks, I've added some commits and will squash if this moves forward.

With that said, and w.r.t. re-using the Python component manager vs re-implementing our own, the latter would make sense only if we expect the component manager not to change every now and then. I have no idea whether that's the case? If you could share your opinion?

I do not think they will change it as the API seems pretty set. However, there are a few features built into the Python version that would require some more effort. E.g:

  • Lock-files with version resolution to ensure build reproducibility
  • Hash / checksums verification
  • Components can require specific versions of IDF and, transitively, other components.

Most of this should be pretty straightforward to implement (and I can take care of it). I personally find it to be easier to understand the build process when "all" of it is in the Cargo build, but I don't have a very strong opinion about it as long as it works.

@daggerrz daggerrz requested a review from ivmarkov July 22, 2023 18:56
Dag Liodden added 3 commits July 23, 2023 17:37
Add support for filesystem `idf_component.yml` metadata and ensure the installed version matches the current spec.
@daggerrz
Copy link
Author

Added checks for outdated managed dependencies using idf_component.yml metadata in the component tarballs to make it easier to change version dependencies. This now works well for my "mDNS use case" and a component lock-file for reproducible builds is the only remaining must-have feature I can think of right now.

Great if @N3xed could take a look!

@N3xed
Copy link
Collaborator

N3xed commented Jul 26, 2023

I'm sorry for my inactivity on this issue.

I'm all for implementing things in Rust, but I don't think it is worth it for the component manager, or any aspect that is already covered by the esp-idf for that matter. Though I completely understand your reason, as bridging the gap between the Rust and esp-idf cmake world is indeed tricky.

Most of this should be pretty straightforward to implement (and I can take care of it).

It may be straightforward to implement, but it would very much increase the complexity and amount of dependencies to embuild (which already has a good few) and indirectly to esp-idf-sys (as build dependencies). I think, having our own component manager doesn't come exactly cheap.

I personally find it to be easier to understand the build process when "all" of it is in the Cargo build, but I don't have a very strong opinion about it as long as it works.

Everything else of the C/C++ build uses the esp-idf build system though, and it's probably not a good idea to mix the two.

That's my opinion at least. I've been looking into integrating the esp-idf component manager, so that components, that contain an idf_component.yml, just work (see esp-rs/esp-idf-sys#193 for example). I've wanted to do that since a while ago but haven't had the time until now.

@daggerrz
Copy link
Author

Not a problem - I've found this experiment to be very useful way to learn the build system, so no time wasted! Let me know if there's anything I can do to help with the CMake integration.

@daggerrz daggerrz closed this Jul 26, 2023
@ivmarkov
Copy link
Collaborator

ivmarkov commented Jul 27, 2023

One way or the other, I believe we do need this easy syntax supported soon-ish, and (obviously) without any patches to esp-idf-sys, as the original post here suggests, as requests for generating bindings (and linking against) components that now live in the ESP IDF Component Registry keep popping up. In other words, this syntax, as suggested by @daggerrz:

[[package.metadata.esp-idf-sys.extra_components]]
component_refs = [ { name = "espressif/mdns", version = "1.2" } ]

Reading the esp-idf-sys post from above, it amounts to adding a few simple changes to the CMakeLists.txt template, and then probably some glue in the esp-idf-sys Rust code so that it can pull the components' names and versions and then stuff these in the ESP_IDF_COMPONENTS environment variable?

@N3xed - are you saying you have some time to work on this these days? Otherwise, why don't we ask @daggerrz to work on that (if he is willing to, of course)? I don't want this topic to fall into oblivion again, given how important it is. And I'm swamped with other work ATM.

@N3xed
Copy link
Collaborator

N3xed commented Jul 27, 2023

are you saying you have some time to work on this these days? Otherwise, why don't we ask @daggerrz to work on that (if he is willing to, of course)? I don't want this topic to fall into oblivion again, given how important it is. And I'm swamped with other work ATM.

Yes, I've been using esp-rs/esp-idf-sys#193 (comment) as a starting point, I'll make a PR today.

Reading the esp-idf-sys post from above, it amounts to adding a few simple changes to the CMakeLists.txt template, and then probably some glue in the esp-idf-sys Rust code so that it can pull the components' names and versions and then stuff these in the ESP_IDF_COMPONENTS environment variable?

The integration between Rust and CMake is a bit more tricky though, since we want the lock file in the Rust workspace, and a way to specify dependencies in the Rust side.
As with specifying components in the Cargo.toml, I'll do something similar as @daggerrz suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants