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

Build extra components specified in manifest metadata #114

Merged
merged 13 commits into from
Aug 1, 2022

Conversation

N3xed
Copy link
Collaborator

@N3xed N3xed commented Jul 27, 2022

The first bulk of this PR is refactoring the build configuration. This is done so that the configuration that before could only be specified with environment variables can now also be set in the package.metadata.esp-idf-sys table of the root crate's manifest.
Since we'll be using cargo metadata for specifying extra components anyway, it only makes sense that all configuration can be put into the Cargo.toml and not only configuration regarding extra esp-idf components.

The refactored build configuration uses serde, its derive macros, and a new dependency envy to deserialize environment variables and metadata values from a manifest. This has the advantage that all configuration options are now specified in a single struct and not littered throughout the build script. Additionally extending the configuration is much easier.

The second part is extending this configuration system to allow specifying extra, external esp-idf components (though solely in manifest metadata).
This is possible by adding an object to the package.metadata.esp-idf-sys.extra_components array of the manifest. For example:

[[package.metadata.esp-idf-sys.extra_components]]
component_dirs = ["esp-rainmaker/components/esp-insights/components", "esp-rainmaker/components"]
bindings_header = "src/bindings.h"

Fixes #32, fixes #72

N3xed added 7 commits July 25, 2022 19:16
Because #[path = "..."] approach sets different file paths for the same module,
this causes issues with the normal module structure.
Add dependency `envy` to deserialize struct from env variables.
Refactor `common::InstallDir` using the strum crate.
Add `build_driver::TOOLS_DIR`.
Cargo fmt `build/pio.rs`.
Split build configuration into separate modules and structs `BuildConfig`, `NativeConfig`.
This change allows the build configuration to be specified in the `package.metadata.esp-idf-sys`
table of the root crate's manifest (Cargo.toml). If no root crate exists in the workspace the crate
with the name equal to the `ESP_IDF_SYS_ROOT_CRATE` environment variable is used.

All configuration specified via environment variables will override the corresponding options
of the cargo metadata. If `cargo metadata` fails for some reason, this will only generate a warning
in the build output and not cause the build to fail.

Add build dependency `cargo_metadata`.
Makes it possible to specify extra components in the `package.metadata.esp-idf-sys.extra_components`
array in the root crate's manifest as well as all direct dependencies.
This change also instructs cmake to build all extra components, but without generating any bindings.
Because cmake already knows all the compiled components, it is just a matter of forwarding
that information to the build script. This removes the need to recurse into every subfolder of
the esp-idf's components folder.
There are two ways the bindings are generated.

The first is when `bindings_module` is not set. In that case the header
of the extra component will just be added to the normal esp-idf bindings
generation. All additional bindings will also be in the root of `esp-idf-sys`.

The second: when `bindings_module` is set. For every unique `bindings_module`
bindgen will be invoked to generate the bindings, which will then be appended as a new
module to the `bindings.rs` file. Note though that since this is a new bindgen invocation
duplicate symbols of the normal esp-idf bindings may be generated.
@N3xed N3xed marked this pull request as ready for review July 29, 2022 22:18
@N3xed
Copy link
Collaborator Author

N3xed commented Jul 29, 2022

Okay, this should be pretty close to the finish line now. The only thing missing is more documentation (i.e. README, maybe book) and an embuild release (that's why CI is failing).

Generally, this PR does two things.

First, refactor the configuration to use serde and the envy crate, and allow all build configurations to be specified in the Cargo.toml. This is possible by setting the specific option (lowercased) in the package.metadata.esp-idf-sys object of the root crate's Cargo.toml. The build script gets the metadata by running cargo metadata --frozen --offline in the workspace directory.
If cargo metadata does not give a root crate, it can be specified with the ESP_IDF_SYS_ROOT_CRATE environment variable.

Second, it allows to specify extra esp-idf components (even multiple) in the package.metadata.esp-idf-sys.extra_components array of the root crate's and all direct dependencies' Cargo.toml. Such an extra component config can be specified like this:

[[package.metadata.esp-idf-sys.extra_components]]
# A single path or a list of paths to a component directory, or directory containing components.
#
# Each path can be absolute or relative. Relative paths will be relative to the
# folder containing the defining `Cargo.toml`.
component_dirs = ["dir1", "dirs2"] # or
component_dirs = "one_dir"

# The path to the C header to generate the bindings with. If this option is absent,
# **no** bindings will be generated.
#
# The path can be absolute or relative. A relative path will be relative to the
# folder containing the defining `Cargo.toml`.
#
# Optional
bindings_header = "bindings.h"

# If this option is present, the component bindings will be generated separately from
# the `esp-idf` bindings and put into their own module inside the `esp-idf-sys` crate.
# Otherwise, if absent, the component bindings will be added to the existing
# `esp-idf` bindings (which are available in the crate root).
#
# To put the bindings into its own module, a separate bindgen instance will generate
# the bindings. Note that this will result in duplicate `esp-idf` bindings if the
# same `esp-idf` headers that were already processed for the `esp-idf` bindings are
# included by the component(s).
#
# Optional
bindings_module = "name"

Note, the two [ ([[ and ]]). The above would be equivalent to:

[package.metadata.esp-idf-sys]
extra_components = [
    { component_dirs = [ "dir1", "dirs2" ], bindings_header = "bindings.h", bindings_module = "name" }
]

Reviews are welcome.

@N3xed N3xed requested review from ivmarkov and MabezDev July 30, 2022 12:12
@MabezDev
Copy link
Member

Thanks for tackling this @N3xed! Attempting to try it out now, running into a few build errors though:

error[E0432]: unresolved import `embuild::bindgen::BindgenExt`
  --> /home/mabez/.cargo/git/checkouts/esp-idf-sys-d4c2e811b42d99d6/db3254a/build/build.rs:11:5
   |
11 | use embuild::bindgen::BindgenExt;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `BindgenExt` in `bindgen`

error[E0425]: cannot find function `default_bindings_file` in module `bindgen_utils`
   --> /home/mabez/.cargo/git/checkouts/esp-idf-sys-d4c2e811b42d99d6/db3254a/build/build.rs:126:40
    |
126 |     let bindings_file = bindgen_utils::default_bindings_file()?;
    |                                        ^^^^^^^^^^^^^^^^^^^^^ not found in `bindgen_utils`

error[E0425]: cannot find function `cargo_fmt_file` in module `bindgen_utils`
   --> /home/mabez/.cargo/git/checkouts/esp-idf-sys-d4c2e811b42d99d6/db3254a/build/build.rs:178:20
    |
178 |     bindgen_utils::cargo_fmt_file(&bindings_file);
    |                    ^^^^^^^^^^^^^^ not found in `bindgen_utils`

error[E0425]: cannot find value `ENV_PATH_VAR` in module `embuild::build`
   --> /home/mabez/.cargo/git/checkouts/esp-idf-sys-d4c2e811b42d99d6/db3254a/build/build.rs:197:45
    |
197 |         cargo::set_metadata(embuild::build::ENV_PATH_VAR, env_path);
    |                                             ^^^^^^^^^^^^ not found in `embuild::build`

error[E0425]: cannot find value `ESP_IDF_PATH_VAR` in module `embuild::build`
   --> /home/mabez/.cargo/git/checkouts/esp-idf-sys-d4c2e811b42d99d6/db3254a/build/build.rs:202:25
    |
202 |         embuild::build::ESP_IDF_PATH_VAR,
    |                         ^^^^^^^^^^^^^^^^ not found in `embuild::build`

error[E0599]: no method named `headers` found for struct `bindgen::Builder` in the current scope
   --> /home/mabez/.cargo/git/checkouts/esp-idf-sys-d4c2e811b42d99d6/db3254a/build/build.rs:147:10
    |
147 |         .headers(headers)?
    |          ^^^^^^^ help: there is an associated function with a similar name: `header`

error[E0599]: no method named `headers` found for struct `bindgen::Builder` in the current scope
   --> /home/mabez/.cargo/git/checkouts/esp-idf-sys-d4c2e811b42d99d6/db3254a/build/build.rs:161:18
    |
161 |                 .headers(headers.into_iter().inspect(|h| cargo::track_file(h)))?
    |                  ^^^^^^^ help: there is an associated function with a similar name: `header`

Some errors have detailed explanations: E0425, E0432, E0599.
For more information about an error, try `rustc --explain E0425`.
error: could not compile `esp-idf-sys` due to 7 previous errors
warning: build failed, waiting for other jobs to finish...

@N3xed
Copy link
Collaborator Author

N3xed commented Jul 30, 2022

@MabezDev
Yup, you need embuild master right now to build (I should've made that clearer). That's also why CI is failing.

I didn't want to make a new embuild release, since I might be adding more stuff for this feature.

@MabezDev
Copy link
Member

MabezDev commented Jul 30, 2022

Thanks for the hint about embuild. So I've been playing with this and I can't seem to get it working, sorry for the hand holding I'll need here in advance, but can you see where I'm going wrong here: https://github.com/MabezDev/esp-rainmaker ?

@MabezDev
Copy link
Member

Ugh ignore me, I missed an end quote on an include in binding.h 🤦. Seems to be working very well now! Will review a bit more thoroughly :)

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

This is working brilliantly, thanks for working on this! LGTM!

@czAdamV
Copy link

czAdamV commented Jul 30, 2022

I got this warning when I played around with this branch:

warning: `cargo metadata` exited with an error: error: failed to download `atomic-polyfill v0.1.8`
warning: 
warning: Caused by:
warning:   attempting to make an HTTP request, but --frozen was specified

I tried to add atomic-polyfill to the list of dependencies and got the same warning about bare-metal and then some more crates after that when I added that one. The warnings stopped when I deleted the --offline and --frozen flags from the cargo metadata call, and weirdly enough, they didn't appear again even when I put the flags back. I don't know much about the cargo build process, and I have no idea how to debug this issue (or even reproduce it again), but I figured it might be helpful to at least report it here 🙂

@N3xed
Copy link
Collaborator Author

N3xed commented Jul 31, 2022

I got this warning when I played around with this branch:

warning: `cargo metadata` exited with an error: error: failed to download `atomic-polyfill v0.1.8`
warning: 
warning: Caused by:
warning:   attempting to make an HTTP request, but --frozen was specified

I've also seen that in the CI output, and it seems to me this is because of rust-lang/cargo#10801 (related rust-lang/cargo#10352). My assumption was that when running cargo metadata in a build script to get the metadata of the same build, it should never require changes to the Cargo.lock or downloading crate metadata (since at the start of the build this should've already been done).

So, I'll remove these flags for now.

N3xed added 6 commits July 31, 2022 19:36
cargo metadata resolves all dependencies even ones that are disabled (known as weak dependencies)
which cargo build doesn't do (see issue rust-lang/cargo#10801). These unresolved dependencies require
network access to load their package metadata.
As the `--frozen` flag itself blocks network access (in addition to blocking changes to the Cargo.lock)
too, we need to remove both the `--frozen` and `--offline` flags. Doing this allows cargo the change the
`Cargo.lock` though, which is undesirable (as we're in the process of building the same dependencies that
rely on the `Cargo.lock` in the first place).
@N3xed N3xed force-pushed the feature/extra-components branch from 8891de8 to 27eb477 Compare August 1, 2022 14:58
@N3xed N3xed merged commit ae22edb into master Aug 1, 2022
@N3xed N3xed deleted the feature/extra-components branch August 1, 2022 20:52
@arashsm79
Copy link

arashsm79 commented Aug 6, 2022

Thanks for this!
After this was merged, I tried to use littlefs for my project and it worked flawlessly!

Here's the rundown for anyone who's interested:
I cloned the littlefs port for esp into a directory in my project (here it's called components).
Then added the following to my Cargo.toml

[[package.metadata.esp-idf-sys.extra_components]]
component_dirs = ["components"]
bindings_header = "src/bindings.h"

Contents of src/bindings.h:

#include "esp_littlefs.h"

Here's the partition table used when flashing:

# Name,   Type, SubType, Offset,  Size, Flags
# Note: if you have increased the bootloader size, make sure to update the offsets to avoid overlap
nvs,      data, nvs,     ,        0x6000,
phy_init, data, phy,     ,        0x1000,
factory,  app,  factory, ,        2M,
littlefs, data, spiffs,  ,        0x1d0000,

After building the project and generating the bindings, I simply mounted the partition and registered the littlefs filesystem.

    let mut fs_conf = esp_idf_sys::esp_vfs_littlefs_conf_t {
        base_path: cstr!("/littlefs").as_ptr(),
        partition_label: cstr!("littlefs").as_ptr(),
        ..Default::default()
    };
    fs_conf.set_format_if_mount_failed(true as u8);
    fs_conf.set_dont_mount(false as u8);

    unsafe {esp!(esp_idf_sys::esp_vfs_littlefs_register(&fs_conf))?};
    let (mut fs_total_bytes, mut fs_used_bytes) = (0, 0);
    unsafe {esp!(esp_idf_sys::esp_littlefs_info(fs_conf.partition_label, &mut fs_total_bytes, &mut fs_used_bytes))?};
    info!("LittleFs Info: total bytes = {}, used bytes = {}.", fs_total_bytes, fs_used_bytes);

Finally, I could use the standard library file operations to read from and write to the filesystem.

    {
        let mut file = fs::OpenOptions::new().write(true).create(true).open("/littlefs/log")?;
        file.write_all(b"Hello from the Rustland!\n")?;
    }
    {
        let mut file = fs::OpenOptions::new().read(true).write(true).open("/littlefs/log")?; 
        let mut contents = String::new();
        file.read_to_string(&mut contents)?;
        println!("{}", contents);
    }

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.

External components discussion External component
4 participants