From b5afa5d05b83af196b98522cf7668f3fb95e379c Mon Sep 17 00:00:00 2001 From: Nathan Hammond Date: Mon, 21 Aug 2023 14:04:44 +0800 Subject: [PATCH 1/6] Adopt std::cell::OnceCell --- crates/turborepo-lib/src/commands/link.rs | 3 +- crates/turborepo-lib/src/commands/login.rs | 3 +- crates/turborepo-lib/src/commands/mod.rs | 88 +++++++++------------- crates/turborepo-lib/src/config/client.rs | 17 ++--- crates/turborepo-lib/src/lib.rs | 1 + 5 files changed, 47 insertions(+), 65 deletions(-) diff --git a/crates/turborepo-lib/src/commands/link.rs b/crates/turborepo-lib/src/commands/link.rs index a15c58ea3732f..0867ffbcf0800 100644 --- a/crates/turborepo-lib/src/commands/link.rs +++ b/crates/turborepo-lib/src/commands/link.rs @@ -460,11 +460,10 @@ fn add_space_id_to_turbo_json(base: &CommandBase, space_id: &str) -> Result<()> #[cfg(test)] mod test { - use std::fs; + use std::{cell::OnceCell, fs}; use anyhow::Result; use tempfile::{NamedTempFile, TempDir}; - use tokio::sync::OnceCell; use turbopath::AbsoluteSystemPathBuf; use turborepo_ui::UI; use vercel_api_mock::start_test_server; diff --git a/crates/turborepo-lib/src/commands/login.rs b/crates/turborepo-lib/src/commands/login.rs index 23125e70690f8..bf09dbfad1ef9 100644 --- a/crates/turborepo-lib/src/commands/login.rs +++ b/crates/turborepo-lib/src/commands/login.rs @@ -298,12 +298,11 @@ async fn run_sso_one_shot_server( #[cfg(test)] mod test { - use std::fs; + use std::{cell::OnceCell, fs}; use reqwest::Url; use serde::Deserialize; use tempfile::{tempdir, NamedTempFile}; - use tokio::sync::OnceCell; use turbopath::AbsoluteSystemPathBuf; use turborepo_ui::UI; use vercel_api_mock::start_test_server; diff --git a/crates/turborepo-lib/src/commands/mod.rs b/crates/turborepo-lib/src/commands/mod.rs index 5761ee2714aca..eef14bacfe440 100644 --- a/crates/turborepo-lib/src/commands/mod.rs +++ b/crates/turborepo-lib/src/commands/mod.rs @@ -1,8 +1,7 @@ -use std::borrow::Borrow; +use std::{borrow::Borrow, cell::OnceCell}; use anyhow::Result; use sha2::{Digest, Sha256}; -use tokio::sync::OnceCell; use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf}; use turborepo_api_client::APIClient; use turborepo_ui::UI; @@ -10,7 +9,7 @@ use turborepo_ui::UI; use crate::{ config::{ default_user_config_path, get_repo_config_path, ClientConfig, ClientConfigLoader, - RepoConfig, RepoConfigLoader, UserConfig, UserConfigLoader, + Error as ConfigError, RepoConfig, RepoConfigLoader, UserConfig, UserConfigLoader, }, Args, }; @@ -55,18 +54,26 @@ impl CommandBase { }) } - fn create_repo_config(&self) -> Result<()> { + fn repo_config_init(&self) -> Result { let repo_config_path = get_repo_config_path(self.repo_root.borrow()); - let repo_config = RepoConfigLoader::new(repo_config_path) + RepoConfigLoader::new(repo_config_path) .with_api(self.args.api.clone()) .with_login(self.args.login.clone()) .with_team_slug(self.args.team.clone()) - .load()?; + .load() + } - self.repo_config.set(repo_config)?; + pub fn repo_config(&self) -> Result<&RepoConfig, ConfigError> { + self.repo_config.get_or_try_init(|| self.repo_config_init()) + } - Ok(()) + pub fn repo_config_mut(&mut self) -> Result<&mut RepoConfig, ConfigError> { + // Approximates `get_mut_or_try_init` + self.repo_config + .get_or_try_init(|| self.repo_config_init())?; + + Ok(self.repo_config.get_mut().unwrap()) } // NOTE: This deletes the repo config file. It does *not* remove the @@ -81,62 +88,41 @@ impl CommandBase { Ok(()) } - fn create_user_config(&self) -> Result<()> { - let user_config = UserConfigLoader::new(default_user_config_path()?) + fn user_config_init(&self) -> Result { + UserConfigLoader::new(default_user_config_path()?) .with_token(self.args.token.clone()) - .load()?; - self.user_config.set(user_config)?; - - Ok(()) - } - - fn create_client_config(&self) -> Result<()> { - let client_config = ClientConfigLoader::new() - .with_remote_cache_timeout(self.args.remote_cache_timeout) - .load()?; - self.client_config.set(client_config)?; - - Ok(()) + .load() } - pub fn repo_config_mut(&mut self) -> Result<&mut RepoConfig> { - if self.repo_config.get().is_none() { - self.create_repo_config()?; - } - - Ok(self.repo_config.get_mut().unwrap()) - } - - pub fn repo_config(&self) -> Result<&RepoConfig> { - if self.repo_config.get().is_none() { - self.create_repo_config()?; - } - - Ok(self.repo_config.get().unwrap()) + pub fn user_config(&self) -> Result<&UserConfig, ConfigError> { + self.user_config.get_or_try_init(|| self.user_config_init()) } - pub fn user_config_mut(&mut self) -> Result<&mut UserConfig> { - if self.user_config.get().is_none() { - self.create_user_config()?; - } + pub fn user_config_mut(&mut self) -> Result<&mut UserConfig, ConfigError> { + // Approximates `get_mut_or_try_init` + self.user_config + .get_or_try_init(|| self.user_config_init())?; Ok(self.user_config.get_mut().unwrap()) } - pub fn user_config(&self) -> Result<&UserConfig> { - if self.user_config.get().is_none() { - self.create_user_config()?; - } + fn client_config_init(&self) -> Result { + ClientConfigLoader::new() + .with_remote_cache_timeout(self.args.remote_cache_timeout) + .load() + } - Ok(self.user_config.get().unwrap()) + pub fn client_config(&self) -> Result<&ClientConfig, ConfigError> { + self.client_config + .get_or_try_init(|| self.client_config_init()) } - pub fn client_config(&self) -> Result<&ClientConfig> { - if self.client_config.get().is_none() { - self.create_client_config()?; - } + pub fn client_config_mut(&mut self) -> Result<&mut ClientConfig, ConfigError> { + // Approximates `get_mut_or_try_init` + self.client_config + .get_or_try_init(|| self.client_config_init())?; - Ok(self.client_config.get().unwrap()) + Ok(self.client_config.get_mut().unwrap()) } pub fn args(&self) -> &Args { diff --git a/crates/turborepo-lib/src/config/client.rs b/crates/turborepo-lib/src/config/client.rs index ae77a21cff0b3..9555a1220429d 100644 --- a/crates/turborepo-lib/src/config/client.rs +++ b/crates/turborepo-lib/src/config/client.rs @@ -4,6 +4,8 @@ use anyhow::Result; use config::{Config, ConfigError, Environment}; use serde::{Deserialize, Serialize}; +use crate::config::Error; + const DEFAULT_TIMEOUT: u64 = 20; #[derive(Debug, Clone, PartialEq, Eq)] @@ -49,7 +51,7 @@ impl ClientConfigLoader { self } - pub fn load(self) -> Result { + pub fn load(self) -> Result { let Self { remote_cache_timeout, environment, @@ -60,17 +62,12 @@ impl ClientConfigLoader { .add_source(Environment::with_prefix("turbo").source(environment)) .set_override_option("remote_cache_timeout", remote_cache_timeout)? .build()? + // Deserialize is the only user-input-fallible step. + // Everything else is programmer error. + // This goes wrong when TURBO_REMOTE_CACHE_TIMEOUT can't be deserialized to u64 .try_deserialize(); - // This goes wrong when TURBO_REMOTE_CACHE_TIMEOUT can't be deserialized to u64 - match config_attempt { - Err(_) => Ok(ClientConfig { - config: ClientConfigValue { - remote_cache_timeout: DEFAULT_TIMEOUT, - }, - }), - Ok(config) => Ok(ClientConfig { config }), - } + config_attempt.map(|config| ClientConfig { config }) } } diff --git a/crates/turborepo-lib/src/lib.rs b/crates/turborepo-lib/src/lib.rs index 56ed1b0267b29..e71d14d53dca1 100644 --- a/crates/turborepo-lib/src/lib.rs +++ b/crates/turborepo-lib/src/lib.rs @@ -3,6 +3,7 @@ #![feature(error_generic_member_access)] #![feature(provide_any)] #![feature(option_get_or_insert_default)] +#![feature(once_cell_try)] #![deny(clippy::all)] // Clippy's needless mut lint is buggy: https://github.com/rust-lang/rust-clippy/issues/11299 #![allow(clippy::needless_pass_by_ref_mut)] From bc1b761aee0823d9891498ee9f559d783580b337 Mon Sep 17 00:00:00 2001 From: Nathan Hammond Date: Mon, 21 Aug 2023 14:48:05 +0800 Subject: [PATCH 2/6] Naming is hard. --- crates/turborepo-lib/src/config/client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/turborepo-lib/src/config/client.rs b/crates/turborepo-lib/src/config/client.rs index 9555a1220429d..fcf2365fdb5cd 100644 --- a/crates/turborepo-lib/src/config/client.rs +++ b/crates/turborepo-lib/src/config/client.rs @@ -57,7 +57,7 @@ impl ClientConfigLoader { environment, } = self; - let config_attempt: Result = Config::builder() + let config_attempt: Result = Config::builder() .set_default("remote_cache_timeout", DEFAULT_TIMEOUT)? .add_source(Environment::with_prefix("turbo").source(environment)) .set_override_option("remote_cache_timeout", remote_cache_timeout)? From 6389eaa623b17600acfc4f138aa1f7a8bbf5a02d Mon Sep 17 00:00:00 2001 From: Nathan Hammond Date: Mon, 21 Aug 2023 15:13:57 +0800 Subject: [PATCH 3/6] Rust Analyzer was broken. --- crates/turborepo-lib/src/config/client.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/turborepo-lib/src/config/client.rs b/crates/turborepo-lib/src/config/client.rs index fcf2365fdb5cd..7320b8eacedad 100644 --- a/crates/turborepo-lib/src/config/client.rs +++ b/crates/turborepo-lib/src/config/client.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; use anyhow::Result; -use config::{Config, ConfigError, Environment}; +use config::{Config, Environment}; use serde::{Deserialize, Serialize}; use crate::config::Error; @@ -57,7 +57,7 @@ impl ClientConfigLoader { environment, } = self; - let config_attempt: Result = Config::builder() + let config_attempt = Config::builder() .set_default("remote_cache_timeout", DEFAULT_TIMEOUT)? .add_source(Environment::with_prefix("turbo").source(environment)) .set_override_option("remote_cache_timeout", remote_cache_timeout)? @@ -67,7 +67,9 @@ impl ClientConfigLoader { // This goes wrong when TURBO_REMOTE_CACHE_TIMEOUT can't be deserialized to u64 .try_deserialize(); - config_attempt.map(|config| ClientConfig { config }) + config_attempt + .map_err(|err| Error::Config(err)) + .map(|config| ClientConfig { config }) } } From d59ef938569d36a0e2c9801755f4b3af4ac71965 Mon Sep 17 00:00:00 2001 From: Nathan Hammond Date: Mon, 21 Aug 2023 15:32:22 +0800 Subject: [PATCH 4/6] Seriously what is wrong with things today. --- crates/turborepo-lib/src/config/client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/turborepo-lib/src/config/client.rs b/crates/turborepo-lib/src/config/client.rs index 7320b8eacedad..113cdc864b74c 100644 --- a/crates/turborepo-lib/src/config/client.rs +++ b/crates/turborepo-lib/src/config/client.rs @@ -68,7 +68,7 @@ impl ClientConfigLoader { .try_deserialize(); config_attempt - .map_err(|err| Error::Config(err)) + .map_err(Error::Config) .map(|config| ClientConfig { config }) } } From dd48a2e31644f0530b69ffff9d3f97e8a545636a Mon Sep 17 00:00:00 2001 From: Nathan Hammond Date: Mon, 21 Aug 2023 16:48:04 +0800 Subject: [PATCH 5/6] Update test. --- crates/turborepo-lib/src/config/client.rs | 37 ++++++++++++++++++++--- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/crates/turborepo-lib/src/config/client.rs b/crates/turborepo-lib/src/config/client.rs index 113cdc864b74c..bae0f4e53b434 100644 --- a/crates/turborepo-lib/src/config/client.rs +++ b/crates/turborepo-lib/src/config/client.rs @@ -132,10 +132,11 @@ mod test { #[test] fn test_client_arg_env_variable() -> Result<()> { #[derive(Debug)] - struct TestCase { + struct TestCase<'a> { arg: Option, env: String, output: u64, + want_err: Option<&'a str>, } let tests = [ @@ -143,61 +144,79 @@ mod test { arg: Some(0), env: String::from("0"), output: 0, + want_err: None, }, TestCase { arg: Some(0), env: String::from("2"), output: 0, + want_err: None, }, TestCase { arg: Some(0), env: String::from("garbage"), output: 0, + want_err: None, }, TestCase { arg: Some(0), env: String::from(""), output: 0, + want_err: None, }, TestCase { arg: Some(1), env: String::from("0"), output: 1, + want_err: None, }, TestCase { arg: Some(1), env: String::from("2"), output: 1, + want_err: None, }, TestCase { arg: Some(1), env: String::from("garbage"), output: 1, + want_err: None, }, TestCase { arg: Some(1), env: String::from(""), output: 1, + want_err: None, }, TestCase { arg: None, env: String::from("0"), output: 0, + want_err: None, }, TestCase { arg: None, env: String::from("2"), output: 2, + want_err: None, }, TestCase { arg: None, env: String::from("garbage"), output: DEFAULT_TIMEOUT, + want_err: Some( + "invalid type: string \"garbage\", expected an integer for key \ + `remote_cache_timeout` in the environment", + ), }, TestCase { arg: None, env: String::from(""), output: DEFAULT_TIMEOUT, + want_err: Some( + "invalid type: string \"\", expected an integer for key \ + `remote_cache_timeout` in the environment", + ), }, ]; @@ -210,9 +229,13 @@ mod test { env.insert("TURBO_REMOTE_CACHE_TIMEOUT".into(), test.env.clone()); Some(env) }) - .load()?; + .load(); - assert_eq!(config.remote_cache_timeout(), test.output); + if test.want_err.is_none() { + assert_eq!(config.unwrap().remote_cache_timeout(), test.output); + } else { + assert_eq!(config.err().unwrap().to_string(), test.want_err.unwrap()); + } } // We can only hit the actual system for env vars in a single test @@ -221,9 +244,13 @@ mod test { set_var("TURBO_REMOTE_CACHE_TIMEOUT", test.env.clone()); let config = ClientConfigLoader::new() .with_remote_cache_timeout(test.arg) - .load()?; + .load(); - assert_eq!(config.remote_cache_timeout(), test.output); + if test.want_err.is_none() { + assert_eq!(config.unwrap().remote_cache_timeout(), test.output); + } else { + assert_eq!(config.err().unwrap().to_string(), test.want_err.unwrap()); + } } Ok(()) From 59a6188c442c364e1fe0e4ee7a886fa0f43726c0 Mon Sep 17 00:00:00 2001 From: Nathan Hammond Date: Tue, 22 Aug 2023 01:32:55 +0800 Subject: [PATCH 6/6] Review feedback. --- crates/turborepo-lib/src/commands/mod.rs | 12 +++--------- crates/turborepo-lib/src/config/client.rs | 4 ++-- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/crates/turborepo-lib/src/commands/mod.rs b/crates/turborepo-lib/src/commands/mod.rs index eef14bacfe440..782ae9b5117c9 100644 --- a/crates/turborepo-lib/src/commands/mod.rs +++ b/crates/turborepo-lib/src/commands/mod.rs @@ -70,9 +70,7 @@ impl CommandBase { pub fn repo_config_mut(&mut self) -> Result<&mut RepoConfig, ConfigError> { // Approximates `get_mut_or_try_init` - self.repo_config - .get_or_try_init(|| self.repo_config_init())?; - + self.repo_config()?; Ok(self.repo_config.get_mut().unwrap()) } @@ -100,9 +98,7 @@ impl CommandBase { pub fn user_config_mut(&mut self) -> Result<&mut UserConfig, ConfigError> { // Approximates `get_mut_or_try_init` - self.user_config - .get_or_try_init(|| self.user_config_init())?; - + self.user_config()?; Ok(self.user_config.get_mut().unwrap()) } @@ -119,9 +115,7 @@ impl CommandBase { pub fn client_config_mut(&mut self) -> Result<&mut ClientConfig, ConfigError> { // Approximates `get_mut_or_try_init` - self.client_config - .get_or_try_init(|| self.client_config_init())?; - + self.client_config()?; Ok(self.client_config.get_mut().unwrap()) } diff --git a/crates/turborepo-lib/src/config/client.rs b/crates/turborepo-lib/src/config/client.rs index bae0f4e53b434..48785d16f091f 100644 --- a/crates/turborepo-lib/src/config/client.rs +++ b/crates/turborepo-lib/src/config/client.rs @@ -132,11 +132,11 @@ mod test { #[test] fn test_client_arg_env_variable() -> Result<()> { #[derive(Debug)] - struct TestCase<'a> { + struct TestCase { arg: Option, env: String, output: u64, - want_err: Option<&'a str>, + want_err: Option<&'static str>, } let tests = [