build cargo_driver: update embuild to support non-git IDF_PATH
#353
Conversation
| @@ -35,7 +35,7 @@ build-time = "0.1" # For esp_app_desc!() | |||
| const_format = "0.2" # For esp_app_desc!() | |||
|
|
|||
| [build-dependencies] | |||
| embuild = { version = "0.32", features = ["glob", "kconfig", "cmake", "espidf"] } | |||
| embuild = { git = "https://github.com/denbeigh2000/embuild", rev = "be687ce7dc78fd4cba6f0defd513e0ebb32aac73", version = "0.32", features = ["glob", "kconfig", "cmake", "espidf"] } | |||
There was a problem hiding this comment.
will update and un-draft following the merge of esp-rs/embuild#95
| }; | ||
|
|
||
| let idf_path = config.native.idf_path.as_deref(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
These refer to two paths with different purposes that eventually get returned on line 127:
idf_path, theesp-idfsource directory aka$IDF_PATHused byembuild::EspIdf, returned asidfidf_tools_install_dir, the tool installation directory (no source code), returned astools_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.
| @@ -274,13 +277,12 @@ pub fn build() -> Result<EspIdfBuildOutput> { | |||
| None | |||
| }; | |||
|
|
|||
| // Apply patches, only if the patches were not previously applied and if the esp-idf repo is managed. | |||
| if idf.is_managed_espidf { | |||
| if let SourceTree::Git(repository) = &idf.esp_idf_dir { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?)
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_PATHthat 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:
embuildshouldn't requiregitor.gitto exist in order to build the project embuild#81 (comment)