From adc955f3db2883b15f9b0ce0f887f8287eef2f7a Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Mon, 15 May 2023 15:18:11 +0200 Subject: [PATCH 1/2] Propagate `.cargo/config.toml` `[env]` settings to the process environment Environment variables can be set and optionally override the process environment through `.cargo/config.toml`'s `[env]` section: https://doc.rust-lang.org/cargo/reference/config.html#env These config variables have specific precedence rules with regards to overriding the environment set in the process, and can optionally represent paths relative to the parent of the containing `.cargo/` folder. Besides exposing variables to all other processes called by `xbuild`, this also allows `xbuild` itself to be driven by variables set in `.cargo/config.toml`, such as `$ANDROID_HOME` needed for #116. https://github.com/rust-mobile/cargo-subcommand/pull/12 https://github.com/rust-mobile/cargo-subcommand/pull/16 --- xbuild/src/cargo/config.rs | 253 +++++++++++++++++++++++++++++++++++-- xbuild/src/cargo/mod.rs | 17 +-- xbuild/src/lib.rs | 7 +- xbuild/src/main.rs | 24 +++- 4 files changed, 278 insertions(+), 23 deletions(-) diff --git a/xbuild/src/cargo/config.rs b/xbuild/src/cargo/config.rs index 8ad539f9..ce05fd0b 100644 --- a/xbuild/src/cargo/config.rs +++ b/xbuild/src/cargo/config.rs @@ -1,10 +1,19 @@ -use anyhow::Result; +use anyhow::{Context, Result}; use serde::Deserialize; -use std::path::Path; +use std::{ + borrow::Cow, + collections::BTreeMap, + env::VarError, + ops::Deref, + path::{Path, PathBuf}, +}; -#[derive(Debug, Deserialize)] +#[derive(Clone, Debug, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "kebab-case")] pub struct Config { pub build: Option, + /// + pub env: Option>, } impl Config { @@ -12,22 +21,250 @@ impl Config { let contents = std::fs::read_to_string(path)?; Ok(toml::from_str(&contents)?) } +} + +#[derive(Debug)] +pub struct LocalizedConfig { + pub config: Config, + /// The directory containing `./.cargo/config.toml` + pub workspace: PathBuf, +} + +impl Deref for LocalizedConfig { + type Target = Config; + + fn deref(&self) -> &Self::Target { + &self.config + } +} + +impl LocalizedConfig { + pub fn new(workspace: PathBuf) -> Result { + Ok(Self { + config: Config::parse_from_toml(workspace.join(".cargo/config.toml"))?, + workspace, + }) + } /// Search for and open `.cargo/config.toml` in any parent of the workspace root path. + // TODO: Find, parse, and merge _all_ config files following the hierarchical structure: + // https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure pub fn find_cargo_config_for_workspace(workspace: impl AsRef) -> Result> { let workspace = workspace.as_ref(); let workspace = dunce::canonicalize(workspace)?; workspace .ancestors() - .map(|dir| dir.join(".cargo/config.toml")) - .find(|p| p.is_file()) - .map(Config::parse_from_toml) + .find(|dir| dir.join(".cargo/config.toml").is_file()) + .map(|p| p.to_path_buf()) + .map(Self::new) .transpose() } + + /// Propagate environment variables from this `.cargo/config.toml` to the process environment + /// using [`std::env::set_var()`]. + /// + /// Note that this is automatically performed when calling [`Subcommand::new()`][super::Subcommand::new()]. + pub fn set_env_vars(&self) -> Result<()> { + if let Some(env) = &self.config.env { + for (key, env_option) in env { + // Existing environment variables always have precedence unless + // the extended format is used to set `force = true`: + if !matches!(env_option, EnvOption::Value { force: true, .. }) + && std::env::var_os(key).is_some() + { + continue; + } + + std::env::set_var(key, env_option.resolve_value(&self.workspace)?.as_ref()) + } + } + + Ok(()) + } } -#[derive(Debug, Deserialize)] +#[derive(Clone, Debug, Deserialize, PartialEq, Eq)] pub struct Build { - #[serde(rename = "target-dir")] pub target_dir: Option, } + +/// Serializable environment variable in cargo config, configurable as per +/// , +#[derive(Clone, Debug, Deserialize, PartialEq, Eq)] +#[serde(untagged)] +pub enum EnvOption { + String(String), + Value { + value: String, + #[serde(default)] + force: bool, + #[serde(default)] + relative: bool, + }, +} + +impl EnvOption { + /// Retrieve the value and canonicalize it relative to `config_parent` when [`EnvOption::Value::relative`] is set. + /// + /// `config_parent` is the directory containing `.cargo/config.toml` where this was parsed from. + pub fn resolve_value(&self, config_parent: impl AsRef) -> Result> { + Ok(match self { + Self::Value { + value, + relative: true, + force: _, + } => { + let value = config_parent.as_ref().join(value); + let value = dunce::canonicalize(&value) + .with_context(|| format!("Failed to canonicalize `{}`", value.display()))?; + value + .into_os_string() + .into_string() + .map_err(VarError::NotUnicode)? + .into() + } + Self::String(value) | Self::Value { value, .. } => value.into(), + }) + } +} + +#[test] +fn test_env_parsing() { + let toml = r#" +[env] +# Set ENV_VAR_NAME=value for any process run by Cargo +ENV_VAR_NAME = "value" +# Set even if already present in environment +ENV_VAR_NAME_2 = { value = "value", force = true } +# Value is relative to .cargo directory containing `config.toml`, make absolute +ENV_VAR_NAME_3 = { value = "relative/path", relative = true }"#; + + let mut env = BTreeMap::new(); + env.insert( + "ENV_VAR_NAME".to_string(), + EnvOption::String("value".into()), + ); + env.insert( + "ENV_VAR_NAME_2".to_string(), + EnvOption::Value { + value: "value".into(), + force: true, + relative: false, + }, + ); + env.insert( + "ENV_VAR_NAME_3".to_string(), + EnvOption::Value { + value: "relative/path".into(), + force: false, + relative: true, + }, + ); + + assert_eq!( + toml::from_str::(toml), + Ok(Config { + build: None, + env: Some(env) + }) + ); +} + +#[test] +fn test_env_precedence_rules() { + let toml = r#" +[env] +CARGO_SUBCOMMAND_TEST_ENV_NOT_FORCED = "not forced" +CARGO_SUBCOMMAND_TEST_ENV_FORCED = { value = "forced", force = true }"#; + + let config = LocalizedConfig { + config: toml::from_str::(toml).unwrap(), + workspace: PathBuf::new(), + }; + + // Check if all values are propagated to the environment + config.set_env_vars().unwrap(); + + assert!(matches!( + std::env::var("CARGO_SUBCOMMAND_TEST_ENV_NOT_SET"), + Err(VarError::NotPresent) + )); + assert_eq!( + std::env::var("CARGO_SUBCOMMAND_TEST_ENV_NOT_FORCED").unwrap(), + "not forced" + ); + assert_eq!( + std::env::var("CARGO_SUBCOMMAND_TEST_ENV_FORCED").unwrap(), + "forced" + ); + + // Set some environment values + std::env::set_var( + "CARGO_SUBCOMMAND_TEST_ENV_NOT_FORCED", + "not forced process environment value", + ); + std::env::set_var( + "CARGO_SUBCOMMAND_TEST_ENV_FORCED", + "forced process environment value", + ); + + config.set_env_vars().unwrap(); + + assert_eq!( + std::env::var("CARGO_SUBCOMMAND_TEST_ENV_NOT_FORCED").unwrap(), + // Value remains what is set in the process environment, + // and is not overwritten by set_env_vars() + "not forced process environment value" + ); + assert_eq!( + std::env::var("CARGO_SUBCOMMAND_TEST_ENV_FORCED").unwrap(), + // Value is overwritten thanks to force=true, despite + // also being set in the process environment + "forced" + ); +} + +#[test] +fn test_env_canonicalization() { + use std::ffi::OsStr; + + let toml = r#" +[env] +CARGO_SUBCOMMAND_TEST_ENV_SRC_DIR = { value = "src", force = true, relative = true } +"#; + + let config = LocalizedConfig { + config: toml::from_str::(toml).unwrap(), + workspace: PathBuf::new(), + }; + + config.set_env_vars().unwrap(); + + let path = std::env::var("CARGO_SUBCOMMAND_TEST_ENV_SRC_DIR") + .expect("Canonicalization for a known-to-exist ./src folder should not fail"); + let path = PathBuf::from(path); + assert!(path.is_absolute()); + assert!(path.is_dir()); + assert_eq!(path.file_name(), Some(OsStr::new("src"))); + + let toml = r#" +[env] +CARGO_SUBCOMMAND_TEST_ENV_INEXISTENT_DIR = { value = "blahblahthisfolderdoesntexist", force = true, relative = true } +"#; + + let config = LocalizedConfig { + config: toml::from_str::(toml).unwrap(), + workspace: PathBuf::new(), + }; + + let e = config.set_env_vars().unwrap_err(); + + assert_eq!( + e.to_string(), + "Failed to canonicalize `blahblahthisfolderdoesntexist`" + ); + assert_eq!( + e.root_cause().to_string(), + "No such file or directory (os error 2)" + ); +} diff --git a/xbuild/src/cargo/mod.rs b/xbuild/src/cargo/mod.rs index d90c0158..b82a2896 100644 --- a/xbuild/src/cargo/mod.rs +++ b/xbuild/src/cargo/mod.rs @@ -4,13 +4,13 @@ use std::path::{Path, PathBuf}; use std::process::Command; mod artifact; -mod config; +pub mod config; pub mod manifest; mod utils; pub use artifact::{Artifact, CrateType}; -use self::config::Config; +use self::config::LocalizedConfig; use self::manifest::Manifest; use crate::{CompileTarget, Opt}; @@ -72,13 +72,10 @@ impl Cargo { let package_root = manifest_path.parent().unwrap(); - // TODO: Find, parse, and merge _all_ config files following the hierarchical structure: - // https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure - let config = Config::find_cargo_config_for_workspace(package_root)?; - // TODO: Import LocalizedConfig code from cargo-subcommand and propagate `[env]` - // if let Some(config) = &config { - // config.set_env_vars().unwrap(); - // } + let config = LocalizedConfig::find_cargo_config_for_workspace(package_root)?; + if let Some(config) = &config { + config.set_env_vars()?; + } let target_dir = target_dir .or_else(|| { @@ -101,7 +98,7 @@ impl Cargo { .unwrap_or_else(|| &manifest_path) .parent() .unwrap() - .join(utils::get_target_dir_name(config.as_ref()).unwrap()) + .join(utils::get_target_dir_name(config.as_deref()).unwrap()) }); Ok(Self { diff --git a/xbuild/src/lib.rs b/xbuild/src/lib.rs index 4fd57b4a..34e501a1 100644 --- a/xbuild/src/lib.rs +++ b/xbuild/src/lib.rs @@ -17,7 +17,7 @@ macro_rules! exe { }; } -mod cargo; +pub mod cargo; pub mod command; mod config; mod devices; @@ -387,7 +387,7 @@ pub struct BuildTargetArgs { /// Build artifacts for target device. To find the device /// identifier of a connected device run `x devices`. #[clap(long, conflicts_with = "store")] - device: Option, + device: Option, /// Build artifacts with format. Can be one of `aab`, /// `apk`, `appbundle`, `appdir`, `appimage`, `dmg`, /// `exe`, `ipa`, `msix`. @@ -424,6 +424,9 @@ impl BuildTargetArgs { Some(Device::host()) } else { self.device + .as_ref() + .map(|device| device.parse()) + .transpose()? }; let platform = if let Some(platform) = self.platform { platform diff --git a/xbuild/src/main.rs b/xbuild/src/main.rs index 69591f4e..9e1acfdf 100644 --- a/xbuild/src/main.rs +++ b/xbuild/src/main.rs @@ -2,7 +2,7 @@ use anyhow::Result; use app_store_connect::certs_api::CertificateType; use clap::{Parser, Subcommand}; use std::path::PathBuf; -use xbuild::{command, BuildArgs, BuildEnv}; +use xbuild::{cargo::config::LocalizedConfig, command, BuildArgs, BuildEnv}; #[derive(Parser)] #[clap(author, version, about, long_about = None)] @@ -76,12 +76,30 @@ enum Commands { }, } +/// Setup a partial build environment (e.g. read `[env]` from `.cargo/config.toml`) when there is +/// no crate/manifest selected. Pretend `$PWD` is the workspace. +/// +/// Only necessary for apps that don't call [`BuildEnv::new()`], +fn partial_build_env() -> Result<()> { + let config = LocalizedConfig::find_cargo_config_for_workspace(".")?; + if let Some(config) = &config { + config.set_env_vars()?; + } + Ok(()) +} + impl Commands { pub fn run(self) -> Result<()> { match self { Self::New { name } => command::new(&name)?, - Self::Doctor => command::doctor(), - Self::Devices => command::devices()?, + Self::Doctor => { + partial_build_env()?; + command::doctor() + } + Self::Devices => { + partial_build_env()?; + command::devices()? + } Self::Build { args } => { let env = BuildEnv::new(args)?; command::build(&env)?; From bc36f20faa291b8b9126c85bcc9f96c2ddfbe210 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Mon, 15 May 2023 15:20:52 +0200 Subject: [PATCH 2/2] cargo/config: Don't canonicalize joined `[env]` `relative` paths Cargo doesn't do this either, and canonicalization requires the path to exist which it does not have to. --- xbuild/src/cargo/config.rs | 57 ++++++++++++-------------------------- 1 file changed, 17 insertions(+), 40 deletions(-) diff --git a/xbuild/src/cargo/config.rs b/xbuild/src/cargo/config.rs index ce05fd0b..b57ca3ac 100644 --- a/xbuild/src/cargo/config.rs +++ b/xbuild/src/cargo/config.rs @@ -1,4 +1,4 @@ -use anyhow::{Context, Result}; +use anyhow::Result; use serde::Deserialize; use std::{ borrow::Cow, @@ -104,7 +104,7 @@ pub enum EnvOption { } impl EnvOption { - /// Retrieve the value and canonicalize it relative to `config_parent` when [`EnvOption::Value::relative`] is set. + /// Retrieve the value and join it to `config_parent` when [`EnvOption::Value::relative`] is set. /// /// `config_parent` is the directory containing `.cargo/config.toml` where this was parsed from. pub fn resolve_value(&self, config_parent: impl AsRef) -> Result> { @@ -113,16 +113,13 @@ impl EnvOption { value, relative: true, force: _, - } => { - let value = config_parent.as_ref().join(value); - let value = dunce::canonicalize(&value) - .with_context(|| format!("Failed to canonicalize `{}`", value.display()))?; - value - .into_os_string() - .into_string() - .map_err(VarError::NotUnicode)? - .into() - } + } => config_parent + .as_ref() + .join(value) + .into_os_string() + .into_string() + .map_err(VarError::NotUnicode)? + .into(), Self::String(value) | Self::Value { value, .. } => value.into(), }) } @@ -225,9 +222,7 @@ CARGO_SUBCOMMAND_TEST_ENV_FORCED = { value = "forced", force = true }"#; } #[test] -fn test_env_canonicalization() { - use std::ffi::OsStr; - +fn test_env_relative() { let toml = r#" [env] CARGO_SUBCOMMAND_TEST_ENV_SRC_DIR = { value = "src", force = true, relative = true } @@ -235,36 +230,18 @@ CARGO_SUBCOMMAND_TEST_ENV_SRC_DIR = { value = "src", force = true, relative = tr let config = LocalizedConfig { config: toml::from_str::(toml).unwrap(), - workspace: PathBuf::new(), + // Path does not have to exist + workspace: PathBuf::from("my/work/space"), }; config.set_env_vars().unwrap(); let path = std::env::var("CARGO_SUBCOMMAND_TEST_ENV_SRC_DIR") - .expect("Canonicalization for a known-to-exist ./src folder should not fail"); + .expect("Relative env var should always be set"); let path = PathBuf::from(path); - assert!(path.is_absolute()); - assert!(path.is_dir()); - assert_eq!(path.file_name(), Some(OsStr::new("src"))); - - let toml = r#" -[env] -CARGO_SUBCOMMAND_TEST_ENV_INEXISTENT_DIR = { value = "blahblahthisfolderdoesntexist", force = true, relative = true } -"#; - - let config = LocalizedConfig { - config: toml::from_str::(toml).unwrap(), - workspace: PathBuf::new(), - }; - - let e = config.set_env_vars().unwrap_err(); - - assert_eq!( - e.to_string(), - "Failed to canonicalize `blahblahthisfolderdoesntexist`" - ); - assert_eq!( - e.root_cause().to_string(), - "No such file or directory (os error 2)" + assert!( + path.is_relative(), + "Workspace was not absolute so this shouldn't either" ); + assert_eq!(path, Path::new("my/work/space/src")); }