Nix - support - rebase #353 (#356)

* build cargo_driver: update embuild to support non-git `IDF_PATH`

* Leave a TODO that we need to cleanup the code that deals with an activated ESP IDF environment

---------

Co-authored-by: Denbeigh Stevens <[email protected]>

Author: ivmarkov

build/common.rs

@@ -257,8 +257,7 @@ impl InstallDir {
     /// Get the install directory from the [`ESP_IDF_TOOLS_INSTALL_DIR_VAR`] env variable.
     ///
     /// If this env variable is unset or empty uses `default_install_dir` instead.
-    /// On success returns `(install_dir as InstallDir, is_default as bool)`.
-    pub fn try_from(location: Option<&str>) -> Result<InstallDir> {
+    pub fn try_from(location: Option<&str>) -> Result<Self> {
         let (location, path) = match &location {
             None => (crate::config::DEFAULT_TOOLS_INSTALL_DIR, None),
             Some(val) => {

build/native/cargo_driver.rs

@@ -10,7 +10,10 @@ 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, DEFAULT_ESP_IDF_REPOSITORY};
+use embuild::espidf::{
+    EspIdfOrigin, EspIdfRemote, FromEnvError, NotActivatedError, 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};
@@ -49,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}')", 
+                    "Specified MCU '{chip}' is not amongst the MCUs ([{}]) supported by the build target ('{target}')",
                     supported_chips.iter().map(|chip| format!("{chip}")).collect::<Vec<_>>().join(", ")
                 );
             }
@@ -82,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 |repo: &git::Repository,
+    let make_tools = move |tree: &SourceTree,
                            version: &Result<espidf::EspIdfVersion>|
           -> Result<Vec<espidf::Tools>> {
         eprintln!(
             "Using esp-idf {} at '{}'",
             espidf::EspIdfVersion::format(version),
-            repo.worktree().display()
+            tree.path().display(),
         );
 
         let mut tools = vec![];
@@ -127,38 +130,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 (install_dir, is_default_install_dir) = config.esp_idf_tools_install_dir()?;
+        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 = install_dir.is_from_env();
+        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.worktree().display(),
-                    espidf::IDF_PATH_VAR
-                );
+                        "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
-                    ));
+                            "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
-                    ));
+                            "Ignoring configuration setting `{ESP_IDF_VERSION_VAR}` ({custom_version}): \
+                             custom esp-idf repository detected via ${}",
+                            espidf::IDF_PATH_VAR
+                        ));
                     }
                 }
                 EspIdfOrigin::Managed(remote) => {
@@ -167,13 +169,21 @@ pub fn build() -> Result<EspIdfBuildOutput> {
             };
 
             let idf = espidf::Installer::new(esp_idf_origin)
-                .install_dir(install_dir.path().map(Into::into))
+                .install_dir(idf_tools_install_dir.path().map(Into::into))
                 .with_tools(make_tools)
                 .install()
                 .context("Could not install esp-idf")?;
-            Ok((idf, install_dir.clone()))
+            Ok((idf, idf_tools_install_dir.clone()))
         };
 
+        // TODO: This is a bit of a mess.
+        // We should probably refactor this and the lines below to make it more readable.
+        //
+        // For one, it is still unclear to me when this path should be used and when not.
+        // It seems an option is to also completely retire specifying the IDF PATH in the cargo-metadata config,
+        // see: https://github.com/esp-rs/esp-idf-sys/pull/353#issuecomment-2543179482
+        let idf_path = config.native.idf_path.as_deref();
+
         // 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
@@ -185,29 +195,29 @@ pub fn build() -> Result<EspIdfBuildOutput> {
                 eprintln!(
                     "Using activated esp-idf {} environment at '{}'",
                     espidf::EspIdfVersion::format(&idf.version),
-                    idf.repository.worktree().display()
+                    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.repository))?
+                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::NotActivated(NotActivatedError { 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} == {install_dir}")
+                    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_repo, .. }), _) => {
-                install(EspIdfOrigin::Custom(esp_idf_repo))?
+            (Err(FromEnvError::NotActivated(NotActivatedError { esp_idf_dir, .. })), _) => {
+                install(EspIdfOrigin::Custom(esp_idf_dir))?
             },
             (Err(FromEnvError::NoRepo(_)), _) => {
-                let origin = match &config.native.idf_path {
-                    Some(idf_path) => EspIdfOrigin::Custom(git::Repository::open(idf_path)?),
+                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()
@@ -284,9 +294,13 @@ pub fn build() -> Result<EspIdfBuildOutput> {
     let patch_set = match idf.version.as_ref().map(|v| (v.major, v.minor, v.patch)) {
         // master branch
         _ if {
-            let default_branch = idf.repository.get_default_branch()?;
-            let curr_branch = idf.repository.get_branch_name()?;
-            default_branch == curr_branch && default_branch.is_some()
+            if let SourceTree::Git(repository) = &idf.esp_idf_dir {
+                let default_branch = repository.get_default_branch()?;
+                let curr_branch = repository.get_branch_name()?;
+                default_branch == curr_branch && default_branch.is_some()
+            } else {
+                false
+            }
         } =>
         {
             cargo::print_warning(
@@ -313,10 +327,11 @@ pub fn build() -> Result<EspIdfBuildOutput> {
         }
     };
 
-    // Apply patches, only if the patches were not previously applied and if the esp-idf repo is managed.
+    // Apply patches, only if the patches were not previously applied and if the esp-idf dir is a managed GIT repo.
     if idf.is_managed_espidf && !patch_set.is_empty() {
-        idf.repository
-            .apply_once(patch_set.iter().map(|p| manifest_dir.join(p)))?;
+        if let SourceTree::Git(repository) = &idf.esp_idf_dir {
+            repository.apply_once(patch_set.iter().map(|p| manifest_dir.join(p)))?;
+        }
     }
 
     // "PATH" is anyway passed to the CMake generator, but if we don't set it here, we get the following warnings from CMake:
@@ -427,7 +442,7 @@ pub fn build() -> Result<EspIdfBuildOutput> {
     )?;
 
     let cmake_toolchain_file = path_buf![
-        &idf.repository.worktree(),
+        &idf.esp_idf_dir.path(),
         "tools",
         "cmake",
         chip.cmake_toolchain_file()
@@ -443,7 +458,7 @@ pub fn build() -> Result<EspIdfBuildOutput> {
             cmake::cmake(),
             "-P",
             extractor_script.as_ref().as_os_str();
-            env=("IDF_PATH", &idf.repository.worktree().as_os_str()))
+            env=("IDF_PATH", idf.esp_idf_dir.path()))
         .stdout()?;
 
         let mut vars = cmake::process_script_variables_extractor_output(output)?;
@@ -484,7 +499,7 @@ pub fn build() -> Result<EspIdfBuildOutput> {
         .cxxflag(cxx_flags)
         .env("IDF_COMPONENT_MANAGER", idf_comp_manager)
         .env("EXTRA_COMPONENT_DIRS", extra_component_dirs)
-        .env("IDF_PATH", idf.repository.worktree())
+        .env("IDF_PATH", idf.esp_idf_dir.path())
         .env("PATH", &idf.exported_path)
         .env("SDKCONFIG_DEFAULTS", defaults_files)
         .env("IDF_TARGET", &chip_name)
@@ -540,7 +555,7 @@ pub fn build() -> Result<EspIdfBuildOutput> {
         .context("Could not determine the compiler from cmake")?;
 
     let build_info = espidf::EspIdfBuildInfo {
-        esp_idf_dir: idf.repository.worktree().to_owned(),
+        esp_idf_dir: idf.esp_idf_dir.path().to_owned(),
         exported_path_var: idf.exported_path.try_to_str()?.to_owned(),
         venv_python: idf.venv_python,
         build_dir: cmake_build_dir.clone(),