build cargo_driver: update embuild to support non-git IDF_PATH
#353
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,9 @@ use config::{ESP_IDF_REPOSITORY_VAR, ESP_IDF_VERSION_VAR}; | |
| use embuild::cargo::IntoWarning; | ||
| use embuild::cmake::file_api::codemodel::Language; | ||
| use embuild::cmake::file_api::ObjKind; | ||
| use embuild::espidf::{ | ||
| EspIdfOrigin, EspIdfRemote, FromEnvError, SourceTree, DEFAULT_ESP_IDF_REPOSITORY, | ||
| }; | ||
| use embuild::fs::copy_file_if_different; | ||
| use embuild::utils::{OsStrExt, PathExt}; | ||
| use embuild::{bindgen, build, cargo, cmake, espidf, git, kconfig, path_buf}; | ||
|
|
@@ -50,7 +52,7 @@ pub fn build() -> Result<EspIdfBuildOutput> { | |
| if let Ok(chip) = Chip::from_str(mcu) { | ||
| if !supported_chips.iter().any(|sc| *sc == chip) { | ||
| bail!( | ||
| "Specified MCU '{chip}' is not amongst the MCUs ([{}]) supported by the build target ('{target}')", | ||
| supported_chips.iter().map(|chip| format!("{chip}")).collect::<Vec<_>>().join(", ") | ||
| ); | ||
| } | ||
|
|
@@ -83,13 +85,13 @@ pub fn build() -> Result<EspIdfBuildOutput> { | |
| let cmake_generator = config.native.esp_idf_cmake_generator(); | ||
|
|
||
| // A closure to specify which tools `idf-tools.py` should install. | ||
| let make_tools = move |tree: &SourceTree, | ||
| version: &Result<espidf::EspIdfVersion>| | ||
| -> Result<Vec<espidf::Tools>> { | ||
| eprintln!( | ||
| "Using esp-idf {} at '{}'", | ||
| espidf::EspIdfVersion::format(version), | ||
| tree.path().display(), | ||
| ); | ||
|
|
||
| let mut tools = vec![]; | ||
|
|
@@ -121,38 +123,37 @@ pub fn build() -> Result<EspIdfBuildOutput> { | |
| // we're using msys/cygwin. | ||
| // But this variable is also present when using git-bash. | ||
| env::remove_var("MSYSTEM"); | ||
| // Install the esp-idf and its tools. | ||
| let (idf, tools_install_dir) = { | ||
| // Get the install dir location from the build config, or use | ||
| // [`crate::config::DEFAULT_TOOLS_INSTALL_DIR`] if unset. | ||
| let (idf_tools_install_dir, is_default_install_dir) = config.esp_idf_tools_install_dir()?; | ||
| // EspIdf must come from the environment if `esp_idf_tools_install_dir` == `fromenv`. | ||
| let require_from_env = idf_tools_install_dir.is_from_env(); | ||
| let maybe_from_env = require_from_env || is_default_install_dir; | ||
|
|
||
| // Closure to install the esp-idf using `embuild::espidf::Installer`. | ||
| let install = |esp_idf_origin: EspIdfOrigin| -> Result<(espidf::EspIdf, InstallDir)> { | ||
| match &esp_idf_origin { | ||
| EspIdfOrigin::Custom(repo) => { | ||
| eprintln!( | ||
| "Using custom user-supplied esp-idf repository at '{}' (detected from env variable `{}`)", | ||
| repo.path().display(), | ||
| espidf::IDF_PATH_VAR | ||
| ); | ||
| if let Some(custom_url) = &config.native.esp_idf_repository { | ||
| cargo::print_warning(format_args!( | ||
| "Ignoring configuration setting `{ESP_IDF_REPOSITORY_VAR}=\"{custom_url}\"`: \ | ||
| custom esp-idf repository detected via ${}", | ||
| espidf::IDF_PATH_VAR | ||
| )); | ||
| } | ||
| if let Some(custom_version) = &config.native.esp_idf_version { | ||
| cargo::print_warning(format_args!( | ||
| "Ignoring configuration setting `{ESP_IDF_VERSION_VAR}` ({custom_version}): \ | ||
| custom esp-idf repository detected via ${}", | ||
| espidf::IDF_PATH_VAR | ||
| )); | ||
| } | ||
| } | ||
| EspIdfOrigin::Managed(remote) => { | ||
|
|
@@ -161,47 +162,49 @@ pub fn build() -> Result<EspIdfBuildOutput> { | |
| }; | ||
|
|
||
| let idf = espidf::Installer::new(esp_idf_origin) | ||
| .install_dir(idf_tools_install_dir.path().map(Into::into)) | ||
| .with_tools(make_tools) | ||
| .install() | ||
| .context("Could not install esp-idf")?; | ||
| Ok((idf, idf_tools_install_dir.clone())) | ||
| }; | ||
|
|
||
| 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. There was a problem hiding this comment. These refer to two paths with different purposes that eventually get returned on line 127:
Before this PR, the only way to set a custom IDF checkout was to set the Note This change might also fix an existing bug where this config in [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. There was a problem hiding this comment. If I read this correctly, before these 2 PRs,
Suppose we specify idf_path from (b) [package.metadata.esp-idf-sys]
idf_path = "/path/to/my/idf-checkout"
@ivmarkov hope this helps. |
||
|
|
||
| // 1. Try to use the activated esp-idf environment if `esp_idf_tools_install_dir` | ||
| // is `fromenv` or unset. | ||
| // 2. Use a custom esp-idf repository specified by `$IDF_PATH`/`idf_path` if | ||
| // available and install the tools using `embuild::espidf::Installer` in | ||
| // `install_dir`. | ||
| // 3. Install the esp-idf and its tools in `install_dir`. | ||
| match (espidf::EspIdf::try_from_env(idf_path), maybe_from_env) { | ||
| (Ok(idf), true) => { | ||
| eprintln!( | ||
| "Using activated esp-idf {} environment at '{}'", | ||
| espidf::EspIdfVersion::format(&idf.version), | ||
| idf.esp_idf_dir.path().display() | ||
| ); | ||
|
|
||
| (idf, InstallDir::FromEnv) | ||
| }, | ||
| (Ok(idf), false) => { | ||
| cargo::print_warning(format_args!( | ||
| "Ignoring activated esp-idf environment: {ESP_IDF_TOOLS_INSTALL_DIR_VAR} != {}", InstallDir::FromEnv | ||
| )); | ||
| install(EspIdfOrigin::Custom(idf.esp_idf_dir))? | ||
| }, | ||
| (Err(FromEnvError::NotActivated { source: err, .. }), true) | | ||
| (Err(FromEnvError::NoRepo(err)), true) if require_from_env => { | ||
| return Err(err.context( | ||
| format!("activated esp-idf environment not found but required by {ESP_IDF_TOOLS_INSTALL_DIR_VAR} == {idf_tools_install_dir}") | ||
| )) | ||
| } | ||
| (Err(FromEnvError::NotActivated { esp_idf_dir, .. }), _) => { | ||
| install(EspIdfOrigin::Custom(esp_idf_dir))? | ||
| }, | ||
| (Err(FromEnvError::NoRepo(_)), _) => { | ||
| let origin = match idf_path { | ||
| Some(idf_path) => EspIdfOrigin::Custom(SourceTree::Plain(idf_path.to_path_buf())), | ||
| None => EspIdfOrigin::Managed(EspIdfRemote { | ||
| git_ref: config.native.esp_idf_version(), | ||
| repo_url: config.native.esp_idf_repository.clone() | ||
|
|
@@ -274,13 +277,12 @@ pub fn build() -> Result<EspIdfBuildOutput> { | |
| None | ||
| }; | ||
|
|
||
| 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 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 |
||
| let patch_set = match idf.version.as_ref().map(|v| (v.major, v.minor, v.patch)) { | ||
| // master branch | ||
| _ if { | ||
| let default_branch = repository.get_default_branch()?; | ||
| let curr_branch = repository.get_branch_name()?; | ||
| default_branch == curr_branch && default_branch.is_some() | ||
| } => | ||
| { | ||
|
|
@@ -305,8 +307,7 @@ pub fn build() -> Result<EspIdfBuildOutput> { | |
| } | ||
| }; | ||
| if !patch_set.is_empty() { | ||
| repository.apply_once(patch_set.iter().map(|p| manifest_dir.join(p)))?; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -412,7 +413,7 @@ pub fn build() -> Result<EspIdfBuildOutput> { | |
| )?; | ||
|
|
||
| let cmake_toolchain_file = path_buf
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.