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

Support using ESP-IDF from the IDF_PATH variable #41

Closed
ivmarkov opened this issue Dec 9, 2021 · 9 comments · Fixed by #42
Closed

Support using ESP-IDF from the IDF_PATH variable #41

ivmarkov opened this issue Dec 9, 2021 · 9 comments · Fixed by #42

Comments

@ivmarkov
Copy link
Collaborator

ivmarkov commented Dec 9, 2021

If the user has defined an IDF_PATH environment variable, the native build should use the ESP-IDF framework designated by that environment variable, rather than cloning and using a separate ESP-IDF GIT repo.

More details:

  • (Relevant for ESP-IDF versions 4.3.X which do require patching): This ESP-IDF instance should be considered "unmanaged" and should not be patched. It would be assumed, that the user has done that, possibly by providing an ESP-IDF fork which has all the Rust patches applied
  • Related to Support for starting build in activated ESP-IDF environment #36 : in addition to having IDF_PATH defined, the user may or may not have the tooling of that particular ESP-IDF instance install.sh-ed and export.sh-ed:
    • If the tooling is already installed and exported by the user (can be detected by e.g. checking if idf.py is on $PATH), the native builder should not install any additional tooling, setup Python virtual env and so on. It should assume, that the necessary tooling is already on the path (scope of Support for starting build in activated ESP-IDF environment #36 actually, which turns out to be a sub-task of this one)
    • If the tooling is not found on the $PATH, the installer should try to install it, based on the ESP-IDF instance pointed to by the IDF_PATH variable, and then proceed with setting up a virtual Python environment specifically for the build and so on.
@ivmarkov
Copy link
Collaborator Author

ivmarkov commented Dec 9, 2021

Working on that.

ivmarkov added a commit that referenced this issue Dec 9, 2021
ivmarkov added a commit that referenced this issue Dec 9, 2021
@rgcottrell
Copy link

Instead of checking for tooling on the path, I would suggest adding a new feature, let's say "system", to the existing "pio" and "native" features. If the "system" feature is species, the the installation should be skipped. If the tools are found on the path, then they can be used. If the tools are not present, however, I think it would be better fail immediately.

I know that I often forget to activate the IDF environment before running a build. It would be undesirable and unexpected in that case to try to install a new version of the SDK and tools.

@ivmarkov
Copy link
Collaborator Author

Instead of checking for tooling on the path, I would suggest adding a new feature, let's say "system", to the existing "pio" and "native" features. If the "system" feature is species, the the installation should be skipped. If the tools are found on the path, then they can be used. If the tools are not present, however, I think it would be better fail immediately.

I know that I often forget to activate the IDF environment before running a build. It would be undesirable and unexpected in that case to try to install a new version of the SDK and tools.

IDF_PATH is not about using and installing your tooling, but about using your esp-idf.

Introducing another feature is not such a great idea for a variety of reasons. However if you really really want to configure it so that external tooling is always expected and never installed automatically, you can set ESP_IDF_INSTALL_LOCATION to frompath and that would achieve the effect you want.

@rgcottrell
Copy link

IDF_PATH is not about using and installing your tooling, but about using your esp-idf.

Introducing another feature is not such a great idea for a variety of reasons. However if you really really want to configure it so that external tooling is always expected and never installed automatically, you can set ESP_IDF_INSTALL_LOCATION to frompath and that would achieve the effect you want.

What is the ESP_IDF_INSTALL_LOCATION parameter? I don't see it documented and a quick search of the crate didn't reveal any references to it.

I understand that adding new features if not necessary is undesirable. However, it feels like the crate is a bit overloaded and is taking on two separate tasks: 1) generating bindings for the SDK, 2) installing and configuring the IDF tools. These two goals seem to be orthogonal to each other.

Maybe if instead of adding a new feature, the pio and native options are made explicitly opt in. In the absence of specifying either feature then an activated toolchain would be expected to be present. This would minimize the surprise of having an entire SDK downloaded and installed unexpectedly.

My main, concern, however, was to add esp-idf-sys as a dependency on a CMake project and that seems to already be a supported use case with a little extra CMake configuration.

@rgcottrell
Copy link

Another use case for not installing the toolchain automatically is to allow projects to be built from within a Docker image that has all of the tools already installed as part of the base image. It would be undesirable and probably counterproductive to attempt to download and install another version of the toolchain. In this case it's probably the responsibility of the container to ensure that everything is properly installed and configured, but I wonder if there is anything else the crate could do to help prevent mistakes.

@ivmarkov
Copy link
Collaborator Author

IDF_PATH is not about using and installing your tooling, but about using your esp-idf.
Introducing another feature is not such a great idea for a variety of reasons. However if you really really want to configure it so that external tooling is always expected and never installed automatically, you can set ESP_IDF_INSTALL_LOCATION to frompath and that would achieve the effect you want.

What is the ESP_IDF_INSTALL_LOCATION parameter? I don't see it documented and a quick search of the crate didn't reveal any references to it.

Sorry, I really meant ESP_IDF_TOOLS_INSTALL_DIR, where frompath is the new thing introduced by this PR.

I understand that adding new features if not necessary is undesirable. However, it feels like the crate is a bit overloaded and is taking on two separate tasks: 1) generating bindings for the SDK, 2) installing and configuring the IDF tools. These two goals seem to be orthogonal to each other.

It is also 3) building the EDP-IDF and 4) Generating linker flags for the Rust linking process of the binary crate.

Now, I'm all for "single responsibility", "good architecture" and so on CS blah blah (please don't take offense!), but it is not that we have not tried to separate these tasks as much as reasonable. Please study the code (including embuild) and if you have concrete suggestions how to further split/modularize these otherwise closely inter-related tasks, I would gladly return feedback.

Maybe if instead of adding a new feature, the pio and native options are made explicitly opt in. In the absence of specifying either feature then an activated toolchain would be expected to be present. This would minimize the surprise of having an entire SDK downloaded and installed unexpectedly.

I hear you and I feel from purely CS point of view that might be the best option.

Yet, we also have to think for whom we are optimizing the user experience.

If I do what you are suggesting, then average Joe Embedded Hobbyist coming with only initial (if any) Rust experience and zero embedded programming knowledge (let alone ESP-IDF!) will have to type cargo build --features pio instead of just cargo build (that is, unless we can put features in .cargo/config.toml, but even then...). So the thing is - for whom are we optimizing the experience here?

My main, concern, however, was to add esp-idf-sys as a dependency on a CMake project and that seems to already be a supported use case with a little extra CMake configuration.

Indeed. Now - to resonate a bit with your feelings that this crate is doing too many things including "5) support cmake-first bindings-only build", we can try to unify a bit the env variables the crate expects when called from a cmake-first build with those used for cargo-first build. I can't promise 100% unification, but I can give it a shot once I'm back.

@ivmarkov
Copy link
Collaborator Author

Another use case for not installing the toolchain automatically is to allow projects to be built from within a Docker image that has all of the tools already installed as part of the base image. It would be undesirable and probably counterproductive to attempt to download and install another version of the toolchain. In this case it's probably the responsibility of the container to ensure that everything is properly installed and configured, but I wonder if there is anything else the crate could do to help prevent mistakes.

This use case would of course be supported by this PR. As for preventing mistakes, putting a ESP_IDF_TOOLS_INSTALL_DIR=frompath in a .cargo/config.toml file located in a (grand-)parent dir of the binary crate might help. Not sure how all directory mappings would work with a docker image though.

And then again, aren't we optimizing the user experience of a niche use case and worsening the user experience of the main use case, where the user would just download Rust and Clang (if it is not already installed by the distro) and then let the crate do its job by downloading and doing whatever necessary automatically?

@rgcottrell
Copy link

Sorry, I really meant ESP_IDF_TOOLS_INSTALL_DIR, where frompath is the new thing introduced by this PR.

Okay, that sounds great. We are currently exploring whether we should use a cargo-first approach, a CMake-first approach, or some other build architecture. It sounds like the frompath option will give us a lot of flexibility here.

It is also 3) building the EDP-IDF and 4) Generating linker flags for the Rust linking process of the binary crate.

I see. There's a lot of extra work that a cargo-first build needs to do that a CMake-first build doesn't. Hopefully having these steps won't introduce too much build time overhead if we don't need them, since keeping build times as short as possible would be very valuable.

If I do what you are suggesting, then average Joe Embedded Hobbyist coming with only initial (if any) Rust experience and zero embedded programming knowledge (let alone ESP-IDF!) will have to type cargo build --features pio instead of just cargo build (that is, unless we can put features in .cargo/config.toml, but even then...). So the thing is - for whom are we optimizing the experience here?

That's a good point, but I think you could still optimize the experience the first user experience by setting the default features (such as having pio set in the default features like it is now). Then more advanced users can disable the defaults and configure more advanced options as they get more experience with the crate.

Anyway, I think this is pretty low priority once there's an easy way of configuring the crate to use an already installed ESP-IDF environment.

Indeed. Now - to resonate a bit with your feelings that this crate is doing too many things including "5) support cmake-first bindings-only build", we can try to unify a bit the env variables the crate expects when called from a cmake-first build with those used for cargo-first build. I can't promise 100% unification, but I can give it a shot once I'm back.

Thanks. I think supporting the CMake-first use case will add a significant amount of value to this crate and anything that can help unify the two approaches will only help.

@rgcottrell
Copy link

This use case would of course be supported by this PR. As for preventing mistakes, putting a ESP_IDF_TOOLS_INSTALL_DIR=frompath in a .cargo/config.toml file located in a (grand-)parent dir of the binary crate might help. Not sure how all directory mappings would work with a docker image though.

That's a great idea. I think it's likely that we will want the actual directory to be configurable, but we could do something like setting ESP_IDF_TOOLS_INSTALL_DIR=frompath:RememberToSetTheEspIdToolsInstallDir in .cargo/config.toml for example and then make sure that the environment variable is overriden when building.

What would the behavior be if an invalid path is set? Hopefully it would error out and not fallback to trying to download and install the tools.

N3xed pushed a commit to N3xed/esp-idf-sys that referenced this issue Apr 11, 2022
N3xed pushed a commit that referenced this issue Apr 26, 2022
@N3xed N3xed linked a pull request Apr 26, 2022 that will close this issue
@N3xed N3xed closed this as completed in #42 Apr 26, 2022
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 a pull request may close this issue.

2 participants