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.

Sign up for GitHub

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

build cargo_driver: update embuild to support non-git IDF_PATH #353

Closed
wants to merge 1 commit into from
Closed

build cargo_driver: update embuild to support non-git IDF_PATH #353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"] }
ivmarkov marked this conversation as resolved.
Show resolved Hide resolved
anyhow = "1"
regex = "1.5"
bindgen = "0.69"
Expand Down
3 changes: 1 addition & 2 deletions build/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
85 changes: 43 additions & 42 deletions build/native/cargo_driver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, DEFAULT_ESP_IDF_REPOSITORY};
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};
Expand Down Expand Up @@ -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}')",
"Specified MCU '{chip}' is not amongst the MCUs ([{}]) supported by the build target ('{target}')",
supported_chips.iter().map(|chip| format!("{chip}")).collect::<Vec<_>>().join(", ")
);
}
Expand Down Expand Up @@ -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 |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![];
Expand Down Expand Up @@ -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 (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) => {
Expand All @@ -161,47 +162,49 @@ 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()))
};

let idf_path = config.native.idf_path.as_deref();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@denbeigh2000 denbeigh2000 Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 $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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// 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(), maybe_from_env) {
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.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::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 { 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()
Expand Down Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

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()?;
let default_branch = repository.get_default_branch()?;
let curr_branch = repository.get_branch_name()?;
default_branch == curr_branch && default_branch.is_some()
} =>
{
Expand All @@ -305,8 +307,7 @@ pub fn build() -> Result<EspIdfBuildOutput> {
}
};
if !patch_set.is_empty() {
idf.repository
.apply_once(patch_set.iter().map(|p| manifest_dir.join(p)))?;
repository.apply_once(patch_set.iter().map(|p| manifest_dir.join(p)))?;
}
}

Expand Down Expand Up @@ -412,7 +413,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()
Expand All @@ -428,7 +429,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)?;
Expand Down Expand Up @@ -469,7 +470,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)
Expand Down Expand Up @@ -520,7 +521,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(),
Expand Down