build cargo_driver: update embuild to support non-git IDF_PATH

[denbeigh2000]

Updates the version of embuild, and the way it gets invoked, so that we can make use of the changes in esp-rs/embuild#95.

This will allow the end user to provide an IDF_PATH that is not a git checkout.

See this comment for testing methodology

Closes #241.
Not sure about #184, but it will at least remove some blockers to that end.

cc/ @ivmarkov

See also:

• embuild shouldn't require git or .git to exist in order to build the project embuild#81 (comment)
• Support building with non-git repositories embuild#95

[ivmarkov]

As in the other PR (embuild), I'm struggling to understand why this line is necessary.
Can you elaborate a bit on it? How come we need it now, but we did not need it before?

[denbeigh2000]

These refer to two paths with different purposes that eventually get returned on line 127:

• idf_path, the esp-idf source directory aka $IDF_PATH used by embuild::EspIdf, returned as idf
• idf_tools_install_dir, the tool installation directory (no source code), returned as tools_install_dir.

Before this PR, the only way to set a custom IDF checkout was to set the $IDF_PATH environment variable - this has been changed to use config.native.idf_path, which respects both documented ways to configure ESP-IDF when constructed.

Note

This change might also fix an existing bug where this config in Cargo.toml isn't respected? 🤔

[package.metadata.esp-idf-sys]
idf_path = "/path/to/my/idf-checkout"

I'm happy to add a comment and/or update variable names if there's something you find clearer.

[clouds56]

If I read this correctly, before these 2 PRs,

• esp-idf-sys would read idf_path from (a) env of IDF_PATH, (b) package.metadata.esp-idf-sys.idf_path in cargo.toml.
• while embuild would only read IDF_PATH from (a) env of IDF_PATH

Suppose we specify idf_path from (b)

[package.metadata.esp-idf-sys]
idf_path = "/path/to/my/idf-checkout"

embuild would never know idf_path is set to /path/to/my/idf-checkout, so we need to pass this information to embuild. And now there's 2 way to achieve this

1. passing as an argument to embuild::EspIdf::from_env, as @denbeigh2000 do in this PR.
2. set environment variable using std::env::set_var, not sure if there's other side effects.

@ivmarkov hope this helps.

[ivmarkov]

Hmmm, is this change OK? This way you'll also patch external, user-supplied repositories which might not be ideal (i.e. making changes to a user-supplied directory).

Why can't we just use the old logic with is_managed_espidf?

[denbeigh2000]

Hm you're correct - this is checking if the dir is a git repo, instead of if it's managed by EspIdf. Will fix this in next commit (tomorrow?)

[ivmarkov]

@denbeigh2000 Sorry for the delay. Actually not sure if the ball is in my garden now or yours? I think we have two outstanding items:

• Restore the is_managed_espidf logic for patching. I guess this is on you?
• This one which is I guess on me. What I struggle to understand is - given that idf_path and idf_tools_install_dir can be read from the crate meta-data, why are we passing these (actually, only the idf_path one) to a method in embuild named from_env? Shouldn't we either load it all from the environment, or don't load anything from the environment?

[denbeigh2000]

@denbeigh2000 Sorry for the delay.

No problem, my day job has also kept me busy.

Actually not sure if the ball is in my garden now or yours? I think we have two outstanding items:

• Restore the is_managed_espidf logic for patching. I guess this is on you?

Yup, this is on me, I'll fix this up.

---

• This one which is I guess on me. What I struggle to understand is - given that idf_path and idf_tools_install_dir can be read from the crate meta-data, why are we passing these (actually, only the idf_path one) to a method in embuild named from_env? Shouldn't we either load it all from the environment, or don't load anything from the environment?

This might be asking two questions, and I want to make sure I address both:

1. "What should a method named from_env accept?"
2. "What is the appropriate way to pass parameters to embuild? Cargo metadata config, environment variables, or explicit params to the constructor?"

---

What should from_env accept?

You're right, it doesn't seem appropriate for a method named from_env to accept parameters at all, it should infer from the surrounding env. I'll (at least) create a try_from method that just accepts the idf_path.

---

Should we set idf_path with cargo metadata?

Even though callers are able to set package.metadata.esp-idf-sys in their Cargo.toml, it doesn't make much sense for embuild to look here - these are under the esp-idf-sys configuration namespace, and are not an implementation detail of this crate.

Put differently, if another crate was using embuild but not esp-idf-sys _{(for some reason)}, it would feel very odd to put this config in Cargo.toml:

[package.metadata.esp-idf-sys]
idf_path = "/path/to/esp-idf"

embuild also doesn't read this currently. To support configuration this way in embuild without esp-idf-sys, we'd have to port the resolution logic from esp-idf-sys, which just feels out of scope.

Should we set idf_path with an env var?

embuild has respected IDF_PATH for some time, but this appears to be more of an implementation detail so esp-idf-sys behaves as documented. This crate doesn't advertise the IDF_PATH variable heavily.

Today, try_from_env just throws an error if IDF_PATH is unset, so it feels appropriate to me if it doesn't try to read from env and only accepts an explicit path passed in from the caller.

It makes sense to continue respecting IDF_PATH for backward-compatibility reasons, though.

[ivmarkov]

@denbeigh2000 I have the feeling I'm threading water here simply because the current code in esp-idf-sys that deals with
a user-specified IDF_PATH (from environment, or from cargo-metadata) is a bit messy, and hard to understand. For me, at least.

However, this is not fair to the work you've put in this and the embuild PR. Therefore:

• I'll merge your PR as-is or mostly as-is (with minimal changes, so that it is mergable with the latest master - Nix - support - rebase https://github.com/esp-rs/esp-idf-sys/pull/353 #356); the embuild PR is already merged
• I would like to ask you if you could test the esp-idf-sys master surface with *nix (or your env in general) afterwards, and let me know if you notice any issues, so that we can open additional PRs to address those.

Thanks again for your work.

Closing in favor of #356