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

Expose toolchain and sysroot to downstream crates #211

Open
kyrias opened this issue Jun 15, 2023 · 17 comments
Open

Expose toolchain and sysroot to downstream crates #211

kyrias opened this issue Jun 15, 2023 · 17 comments

Comments

@kyrias
Copy link

kyrias commented Jun 15, 2023

To be able to build C code and use bindgen it's necessary to have access to an appropriate C toolchain and sysroot. So far it's been possible to get this by using the toolchain downloaded by espup, but the plan is for it to stop downloading a toolchain at all once lld is capable of taking care of all of the linking.

Since esp-idf-sys ends up downloading the appropriate C toolchain for building code for the relevant targets it would be convenient if it would expose the appropriate paths to the toolchain and sysroot for downstream crates to use in their build.rs scripts.

This isn't a complete solution since it means that any crates that builds C code would need to depend on this crate to get access to the paths, but at least it would be strictly better than the current situation.

C.f. esp-rs/rust-build#127.

@N3xed
Copy link
Collaborator

N3xed commented Jun 16, 2023

Most, if not all, of the metadata about the build environment of the esp-idf is already exported (here), this includes:

  • The cfgs (should be available as environment variable DEP_ESP_IDF_EMBUILD_CFG_ARGS)
  • The C include arguments (env var DEP_ESP_IDF_EMBUILD_C_INCLUDE_ARGS)
  • The complete PATH environment variable used to build the esp-idf (env var DEP_ESP_IDF_EMBUILD_ENV_PATH)
  • The path to the esp-idf (env var DEP_ESP_IDF_EMBUILD_ESP_IDF_PATH)
  • The linker arguments (env var DEP_ESP_IDF_EMBUILD_LINK_ARGS)

To create instances of CfgArgs, CInclArgs and LinkArgs you can call XXXArgs::try_from_env("ESP_IDF").

The esp-idf build script also saves this information to a JSON file in the out dir of the esp-idf-sys crate, though the path to that file is currently not exposed as build script metadata.

The sysroot is currently not explicitly exposed. The native build gets it from cmake and the pio build queries ld with --print-sysroot here.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Jul 9, 2023

@kyrias Do you need more than what is already exposed (and what @N3xed described above), or shall I close this?

@kyrias
Copy link
Author

kyrias commented Jul 10, 2023

Hey,

Sorry for the delay.

We do need to get the sysroot somehow since bindgen doesn't know which headers it should use otherwise. So it would be nice if it was either exposed directly, or if there was some documented supported method of getting bindgen set up properly for projects using esp-idf.

As an example, after installing the toolchain with espup install --targets esp32c3 and sourcing the generated env file, running cargo build in a project with the following in build.rs:

    let bindings = bindgen::Builder::default()
        .header_contents(
            "wrapper.h",
            r#"
                #include <stdint.h>
                uint8_t foo(void);
            "#,
        )
        .parse_callbacks(Box::new(bindgen::CargoCallbacks))
        .generate()
        .unwrap();

fails with

  /home/remmy/src/i/bindgen-test/wrapper.h:3:17: error: unknown type name 'uint8_t'
  thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: ClangDiagnostic("/home/remmy/src/i/bindgen-test/wrapper.h:3:17: error: unknown type name 'uint8_t'\n")', build.rs:16:10

@codyps
Copy link

codyps commented Jul 19, 2023

FWIW, I'm currently working around this by setting a bunch of env variables. Here's an exact copy for some version of the esp-rs toolchains:

export SYSROOT_xtensa_esp32s3_espidf="$(xtensa-esp32s3-elf-ld --print-sysroot)"
export BINDGEN_EXTRA_CLANG_ARGS_xtensa_esp32s3_espidf="--sysroot=$SYSROOT_xtensa_esp32s3_espidf -I$SYSROOT_xtensa_esp32s3_espidf/include -Wno-unused-command-line-argument"
export CFLAGS_xtensa_esp32s3_espidf="-mlongcalls"

Notes:

  • I've only set the items for esp32s3 here as the project I'm developing is for the esp32s3.
  • SYSROOT_xtensa_esp32s3_espidf isn't actually used by anything else other than in the setting of BINDGEN_EXTRA_CLANG_ARGS_xtensa_esp32s3_espidf
  • -mlongcalls is required for the build (because I'm also using the cc crate to build some C code without awareness of the espidf), but is technically distinct from the sysroot issue. -Wno-unused-command-line-argument is needed for recent toolchains because otherwise bindgen gets unhappy about -mlongcalls being unused (it should ignore it, but complains).

I place the above at the bottom of the export.sh file generated by espup so that bindgen called in build scripts works for the xtensa-esp32s3-espidf target.

@kyrias
Copy link
Author

kyrias commented Jul 20, 2023

That's using the C toolchain downloaded by espup which as per the issue description is scheduled for removal once it's not needed for linking anymore.

@ivmarkov
Copy link
Collaborator

All of the above should be unnecessary. Please use - in your build.rs script of the crate that depends on esp-idf-sys - this:

embuild::build::CInclArgs::try_from_env("ESP_IDF")

@kyrias
Copy link
Author

kyrias commented Jul 31, 2023

I guess that works, but bindgen only accepts either individual arguments or an iterator of them, and so we'd need to pull in a crate that can do proper shell word splitting to be able to safely be able to pass what that returns to bindgen. :/

@kyrias
Copy link
Author

kyrias commented Jul 31, 2023

Ah, no, it does not work properly. It ends up pulling in some of the necessary things, but it doesn't pull in the full sysroot and so including e.g. string.h fails.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Jul 31, 2023

I have difficulties believing that, as what you get via CInclArgs is what bindgen in esp-idf-sys itself is using to assemble the esp-idf-sys bindings.

@ivmarkov
Copy link
Collaborator

(... modulo headers for custom components, but these are a separate topic altogether.)

@ivmarkov
Copy link
Collaborator

I guess that works, but bindgen only accepts either individual arguments or an iterator of them, and so we'd need to pull in a crate that can do proper shell word splitting to be able to safely be able to pass what that returns to bindgen. :/

Look at the link from my previous comment as to how to split it and pass it to bindgen. Not perfect, and might break if the ESP IDF directories contain whitespace (which they shouldn't, as that's not officially supported by ESP IDF), but all in all - a three-liner.

@N3xed
Copy link
Collaborator

N3xed commented Jul 31, 2023

I have difficulties believing that, as what you get via CInclArgs is what bindgen in esp-idf-sys itself is using to assemble the esp-idf-sys bindings.

That's not exatly true, we pass the sysroot and include directory for the sysroot explicitly too, and some other things
see embuild::bindgen::Factory::create_builder.

So I'd recommend, using embuild::bindgen::Factory like we're using, e.g.:

let bindgen_fact = embuild::bindgen::Factory {
    clang_args,
    linker: Some(compiler),
    mcu: None,
    force_cpp: false, // set to true if C++ instead of C
    sysroot: None // builder() will try to get the sysroot using `linker`
};
let bindgen = bindgen_fact.builder().expect("creating bindgen builder failed");

You'd have to have some logic to get the correct compiler executable path out of DEP_ESP_IDF_EMBUILD_ENV_PATH.
As for the clang_args just using the linker args should suffice.
As for the clang_args that's a bit tricky. The defines (-D args) are currently not exposed, only the include args. The linker args do technically have both the defines and include args, but intermingled plus ldproxy args, and most of the linker arguments are passed as a file on Windows (see here and here).

What esp-idf-sys does:

  1. Create a embuild::bindgen::Factory from the cmake metadata
  2. Configuring the bindgen::Builder with e.g. -target, some blocklists, etc

@N3xed
Copy link
Collaborator

N3xed commented Jul 31, 2023

But then again, if you're trying to create bindings for C code that uses the esp-idf, it's better to use the extra components mechanism instead. If not, using some custom logic, it is indeed possible to get the

  • compiler, from DEP_ESP_IDF_EMBUILD_ENV_PATH env var
  • sysroot, by querrying it from the appropriate ld (like embuild does, as I've linked above)
  • includes (to the esp-idf and its components), with CInclArgs.

Hope this helps.

@kyrias
Copy link
Author

kyrias commented Jul 31, 2023

And this is why it would be nice if this was exposed in an explicitly supported and documented manner, because this ends up being rather complicated to do something that's simple if you're not using ESP-IDF.

Though it feels like embuild should be the crate that exposes a way to create an appropriately configured bindgen::Factory. But then it would be nice if it had an interface for cc as well, since the same details needed to configure one is needed for the other.


Edit: For the record, we're building C code that is not using ESP-IDF.

@ivmarkov
Copy link
Collaborator

I think what to expose and where the rough edges currently are would be easiest to pinpoint if someone tries to use what we already have, and then show - in actual code - how much the additional overhead is - which then needs to be folded in the form of utility functions in either embuild or esp-idf-sys. But unless you actually try using the code from above, I feel it gets a bit theoretical in like "you guys need to provide a more convenient API".

@kyrias
Copy link
Author

kyrias commented Jul 31, 2023

My point isn't that "you guys need to provide a more convenient API", but that "you guys should provide a documented way of doing this thing that I can't figure out how to do."

For starters, I don't know what would be the "logic to get the correct compiler executable path out of DEP_ESP_IDF_EMBUILD_ENV_PATH." Traversing all of the paths would be "easy enough," but to do that we first need to figure out the logic to decide on the correct compiler executable name somehow, and I don't know how you would do that properly.

@ivmarkov
Copy link
Collaborator

Well, to provide the documentation you are asking for, I have to essentially go back and review the code I (and @N3xed) have written more than a year ago.

And in the meantime I'm forgetting it - case in point - the sysroot thing, which I actually implemented in the past - at least for the PIO build.

So I still maintain that if you learn the code a bit and then try to use it, it would be more efficient in that we'll know what else to document and where exactly the pain points are.

Also, you have a concrete use case at hand which would be driving you. I don't have this use case. Instead, I have a ton of other projects I'm supporting in my free time (just like this one), so lifting the task a bit on your shoulders doesn't sound that wrong to me.

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

No branches or pull requests

4 participants