From 3f7e105eddd9769e40eeca77dbfe913cf363a6ec Mon Sep 17 00:00:00 2001 From: just-an-engineer Date: Wed, 31 Jul 2024 11:59:33 -0400 Subject: [PATCH 1/3] Add ability to specify a port number from cli for start and stop server, instead of config or env vars --- README.md | 4 ++-- src/cmdline.rs | 28 ++++++++++++++++++++-------- src/commands.rs | 31 +++++++++++++++++++++++-------- src/test/tests.rs | 26 ++++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 8fa5a6e6c..639c1f803 100644 --- a/README.md +++ b/README.md @@ -114,9 +114,9 @@ sccache supports gcc, clang, MSVC, rustc, [NVCC](https://docs.nvidia.com/cuda/cu If you don't [specify otherwise](#storage-options), sccache will use a local disk cache. -sccache works using a client-server model, where the server runs locally on the same machine as the client. The client-server model allows the server to be more efficient by keeping some state in memory. The sccache command will spawn a server process if one is not already running, or you can run `sccache --start-server` to start the background server process without performing any compilation. +sccache works using a client-server model, where the server runs locally on the same machine as the client. The client-server model allows the server to be more efficient by keeping some state in memory. The sccache command will spawn a server process if one is not already running, or you can run `sccache --start-server[=port]` to start the background server process without performing any compilation. -You can run `sccache --stop-server` to terminate the server. It will also terminate after (by default) 10 minutes of inactivity. +You can run `sccache --stop-server[=port]` to terminate the server. It will also terminate after (by default) 10 minutes of inactivity. Running `sccache --show-stats` will print a summary of cache statistics. diff --git a/src/cmdline.rs b/src/cmdline.rs index 8a55023e7..925a53f5b 100644 --- a/src/cmdline.rs +++ b/src/cmdline.rs @@ -62,9 +62,9 @@ pub enum Command { /// Run background server. InternalStartServer, /// Start background server as a subprocess. - StartServer, + StartServer(Option), /// Stop background server. - StopServer, + StopServer(Option), /// Zero cache statistics and exit. ZeroStats, /// Show the status of the distributed client. @@ -136,13 +136,19 @@ fn get_clap_command() -> clap::Command { .action(ArgAction::SetTrue), flag_infer_long("start-server") .help("start background server") - .action(ArgAction::SetTrue), + .value_name("port") + .num_args(0..=1) + .default_missing_value("None") + .action(ArgAction::Append), flag_infer_long("debug-preprocessor-cache") .help("show all preprocessor cache entries") .action(ArgAction::SetTrue), flag_infer_long("stop-server") .help("stop background server") - .action(ArgAction::SetTrue), + .value_name("port") + .num_args(0..=1) + .default_missing_value("None") + .action(ArgAction::Append), flag_infer_long_and_short("zero-stats") .help("zero statistics counters") .action(ArgAction::SetTrue), @@ -268,12 +274,18 @@ pub fn try_parse() -> Result { .cloned() .expect("There is a default value"); Ok(Command::ShowStats(fmt, true)) - } else if matches.get_flag("start-server") { - Ok(Command::StartServer) + } else if matches.contains_id("start-server") { + let port = matches + .get_one::("start-server") + .and_then(|s| s.parse::().ok()); + Ok(Command::StartServer(port)) } else if matches.get_flag("debug-preprocessor-cache") { Ok(Command::DebugPreprocessorCacheEntries) - } else if matches.get_flag("stop-server") { - Ok(Command::StopServer) + } else if matches.contains_id("stop-server") { + let port = matches + .get_one::("stop-server") + .and_then(|s| s.parse::().ok()); + Ok(Command::StopServer(port)) } else if matches.get_flag("zero-stats") { Ok(Command::ZeroStats) } else if matches.get_flag("dist-auth") { diff --git a/src/commands.rs b/src/commands.rs index ab740582e..fd0fe1c55 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -82,7 +82,10 @@ async fn read_server_startup_status( /// Re-execute the current executable as a background server, and wait /// for it to start up. #[cfg(not(windows))] -fn run_server_process(startup_timeout: Option) -> Result { +fn run_server_process( + startup_timeout: Option, + _port: Option, +) -> Result { trace!("run_server_process"); let tempdir = tempfile::Builder::new().prefix("sccache").tempdir()?; let socket_path = tempdir.path().join("sock"); @@ -98,11 +101,14 @@ fn run_server_process(startup_timeout: Option) -> Result Result<()> { /// Re-execute the current executable as a background server. #[cfg(windows)] -fn run_server_process(startup_timeout: Option) -> Result { +fn run_server_process( + startup_timeout: Option, + _port: Option, +) -> Result { use futures::StreamExt; use std::mem; use std::os::windows::ffi::OsStrExt; @@ -190,6 +199,7 @@ fn run_server_process(startup_timeout: Option) -> Result) -> Result { if port != actualport { // bail as the next connect_with_retry will fail @@ -662,11 +676,11 @@ pub fn run_command(cmd: Command) -> Result { } server::start_server(config, get_port())?; } - Command::StartServer => { + Command::StartServer(_port) => { trace!("Command::StartServer"); println!("sccache: Starting the server..."); - let startup = - run_server_process(startup_timeout).context("failed to start server process")?; + let startup = run_server_process(startup_timeout, _port) + .context("failed to start server process")?; match startup { ServerStartup::Ok { port } => { if port != DEFAULT_PORT { @@ -678,10 +692,11 @@ pub fn run_command(cmd: Command) -> Result { ServerStartup::Err { reason } => bail!("Server startup failed: {}", reason), } } - Command::StopServer => { + Command::StopServer(port) => { trace!("Command::StopServer"); println!("Stopping sccache server..."); - let server = connect_to_server(get_port()).context("couldn't connect to server")?; + let server = connect_to_server(port.unwrap_or_else(get_port)) + .context("couldn't connect to server")?; let stats = request_shutdown(server)?; stats.print(false); } diff --git a/src/test/tests.rs b/src/test/tests.rs index d5c3ee0e9..fdd0424e7 100644 --- a/src/test/tests.rs +++ b/src/test/tests.rs @@ -320,3 +320,29 @@ fn test_server_port_in_use() { s ); } + +#[test] +#[serial] +// test fails intermittently on macos: +// https://github.com/mozilla/sccache/issues/234 +#[cfg(not(target_os = "macos"))] +fn test_server_port_from_cli() { + // Bind an arbitrary free port. + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let sccache = find_sccache_binary(); + let port = listener.local_addr().unwrap().port().to_string(); + let output = Command::new(sccache) + .arg("--start-server") + .arg(port) + .output() + .unwrap(); + assert!(!output.status.success()); + let s = String::from_utf8_lossy(&output.stderr); + const MSG: &str = "Server startup failed:"; + assert!( + s.contains(MSG), + "Output did not contain '{}':\n========\n{}\n========", + MSG, + s + ); +} From 3a2565298e9180d72bedcc89b2e7f92945beb738 Mon Sep 17 00:00:00 2001 From: just-an-engineer Date: Mon, 5 Aug 2024 11:55:53 -0400 Subject: [PATCH 2/3] Added in config support for port --- src/commands.rs | 14 ++++++++++---- src/config.rs | 23 +++++++++++++++++++++++ tests/harness/mod.rs | 1 + tests/oauth.rs | 1 + 4 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/commands.rs b/src/commands.rs index fd0fe1c55..189880ba6 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -51,10 +51,16 @@ const SERVER_STARTUP_TIMEOUT: Duration = Duration::from_millis(10000); /// Get the port on which the server should listen. fn get_port() -> u16 { - env::var("SCCACHE_SERVER_PORT") - .ok() - .and_then(|s| s.parse().ok()) - .unwrap_or(DEFAULT_PORT) + let fallback = || -> u16 { + env::var("SCCACHE_SERVER_PORT") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(DEFAULT_PORT) + }; + match &Config::load() { + Ok(config) => config.port.unwrap_or_else(fallback), + Err(_) => fallback(), + } } /// Check if ignoring all response errors diff --git a/src/config.rs b/src/config.rs index fbc94ce7e..1b26bd6e1 100644 --- a/src/config.rs +++ b/src/config.rs @@ -556,6 +556,7 @@ pub struct FileConfig { pub cache: CacheConfigs, pub dist: DistConfig, pub server_startup_timeout_ms: Option, + pub port: Option, } // If the file doesn't exist or we can't read it, log the issue and proceed. If the @@ -946,6 +947,7 @@ pub struct Config { pub fallback_cache: DiskCacheConfig, pub dist: DistConfig, pub server_startup_timeout: Option, + pub port: Option, } impl Config { @@ -967,6 +969,7 @@ impl Config { cache, dist, server_startup_timeout_ms, + port, } = file_conf; conf_caches.merge(cache); @@ -982,6 +985,7 @@ impl Config { fallback_cache, dist, server_startup_timeout, + port, } } } @@ -1281,6 +1285,7 @@ fn config_overrides() { }, dist: Default::default(), server_startup_timeout_ms: None, + port: None, }; assert_eq!( @@ -1303,6 +1308,7 @@ fn config_overrides() { }, dist: Default::default(), server_startup_timeout: None, + port: None, } ); } @@ -1578,6 +1584,23 @@ no_credentials = true rewrite_includes_only: false, }, server_startup_timeout_ms: Some(10000), + port: None, + } + ) +} + +#[test] +fn test_port_config() { + // just set up a config file with just port, then have it read it in, and ensure port is defined in the struct + const CONFIG_STR: &str = "port = 8080"; + let file_config: FileConfig = toml::from_str(CONFIG_STR).expect("Is valid toml."); + assert_eq!( + file_config, + FileConfig { + cache: Default::default(), + dist: Default::default(), + server_startup_timeout_ms: None, + port: Some(8080), } ) } diff --git a/tests/harness/mod.rs b/tests/harness/mod.rs index f5997934c..3ed1f1ae7 100644 --- a/tests/harness/mod.rs +++ b/tests/harness/mod.rs @@ -165,6 +165,7 @@ pub fn sccache_client_cfg( rewrite_includes_only: false, // TODO }, server_startup_timeout_ms: None, + port: None, } } diff --git a/tests/oauth.rs b/tests/oauth.rs index af8709c35..ba82a3afd 100755 --- a/tests/oauth.rs +++ b/tests/oauth.rs @@ -60,6 +60,7 @@ fn config_with_dist_auth( rewrite_includes_only: true, }, server_startup_timeout_ms: None, + port: None, } } From f4cf2eb234d9c7768162b53249bcfb068546f1cc Mon Sep 17 00:00:00 2001 From: just-an-engineer Date: Tue, 13 Aug 2024 11:09:45 -0400 Subject: [PATCH 3/3] Add a client cache to store most recently used port to use for future operations (such as a compile request) without having to specify the non-default port every time --- src/commands.rs | 17 +++++-- src/config.rs | 1 - src/test/tests.rs | 42 +++++++++++------- src/util.rs | 111 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 150 insertions(+), 21 deletions(-) diff --git a/src/commands.rs b/src/commands.rs index 189880ba6..33efb3727 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -21,7 +21,7 @@ use crate::jobserver::Client; use crate::mock_command::{CommandChild, CommandCreatorSync, ProcessCommandCreator, RunCommand}; use crate::protocol::{Compile, CompileFinished, CompileResponse, Request, Response}; use crate::server::{self, DistInfo, ServerInfo, ServerStartup, ServerStats}; -use crate::util::daemonize; +use crate::util::{self, daemonize}; use byteorder::{BigEndian, ByteOrder}; use fs::{File, OpenOptions}; use fs_err as fs; @@ -55,7 +55,13 @@ fn get_port() -> u16 { env::var("SCCACHE_SERVER_PORT") .ok() .and_then(|s| s.parse().ok()) - .unwrap_or(DEFAULT_PORT) + .unwrap_or( + util::get_from_client_cache("recent_port") + .ok() + .and_then(|port| port) + .and_then(|port| port.parse().ok()) + .unwrap_or(DEFAULT_PORT), + ) }; match &Config::load() { Ok(config) => config.port.unwrap_or_else(fallback), @@ -90,7 +96,7 @@ async fn read_server_startup_status( #[cfg(not(windows))] fn run_server_process( startup_timeout: Option, - _port: Option, + cli_port: Option, ) -> Result { trace!("run_server_process"); let tempdir = tempfile::Builder::new().prefix("sccache").tempdir()?; @@ -107,7 +113,10 @@ fn run_server_process( tokio::net::UnixListener::bind(&socket_path)? }; - let port = _port.unwrap_or_else(get_port); + let port = cli_port.unwrap_or_else(get_port); + if cli_port.is_some() { + util::write_to_client_cache("recent_port", port.to_string().as_str())?; + } let _child = process::Command::new(&exe_path) .current_dir(workdir) diff --git a/src/config.rs b/src/config.rs index 1b26bd6e1..af283d84a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1591,7 +1591,6 @@ no_credentials = true #[test] fn test_port_config() { - // just set up a config file with just port, then have it read it in, and ensure port is defined in the struct const CONFIG_STR: &str = "port = 8080"; let file_config: FileConfig = toml::from_str(CONFIG_STR).expect("Is valid toml."); assert_eq!( diff --git a/src/test/tests.rs b/src/test/tests.rs index fdd0424e7..aea833521 100644 --- a/src/test/tests.rs +++ b/src/test/tests.rs @@ -299,7 +299,6 @@ fn test_server_compile() { // https://github.com/mozilla/sccache/issues/234 #[cfg(not(target_os = "macos"))] fn test_server_port_in_use() { - // Bind an arbitrary free port. let listener = TcpListener::bind("127.0.0.1:0").unwrap(); let sccache = find_sccache_binary(); let output = Command::new(sccache) @@ -327,22 +326,33 @@ fn test_server_port_in_use() { // https://github.com/mozilla/sccache/issues/234 #[cfg(not(target_os = "macos"))] fn test_server_port_from_cli() { - // Bind an arbitrary free port. - let listener = TcpListener::bind("127.0.0.1:0").unwrap(); let sccache = find_sccache_binary(); - let port = listener.local_addr().unwrap().port().to_string(); - let output = Command::new(sccache) - .arg("--start-server") - .arg(port) + loop { + let port = 10_000 + rand::random::() % 30_000; + let output = Command::new(sccache.clone()) + .arg("--start-server") + .arg(port.to_string()) + .output() + .unwrap(); + let s = String::from_utf8_lossy(&output.stderr); + const PORT_IN_USE: &str = "Address in use"; + if s.contains(PORT_IN_USE) { + continue; + } + assert!(output.status.success()); + break; + } + // try to compile something to ensure our compile requests use the most recent port + let output = Command::new(sccache.clone()) + .arg("gcc") + .arg("-c") + .arg("test.c") + .arg("-o") + .arg("test.o") .output() .unwrap(); - assert!(!output.status.success()); - let s = String::from_utf8_lossy(&output.stderr); - const MSG: &str = "Server startup failed:"; - assert!( - s.contains(MSG), - "Output did not contain '{}':\n========\n{}\n========", - MSG, - s - ); + assert!(output.status.success()); + + let output = Command::new(sccache).arg("--stop-server").output().unwrap(); + assert!(output.status.success()); } diff --git a/src/util.rs b/src/util.rs index 4fc45af2e..1a2fffbe4 100644 --- a/src/util.rs +++ b/src/util.rs @@ -15,6 +15,7 @@ use crate::mock_command::{CommandChild, RunCommand}; use blake3::Hasher as blake3_Hasher; use byteorder::{BigEndian, ByteOrder}; +use directories::BaseDirs; use fs::File; use fs_err as fs; use object::{macho, read::archive::ArchiveFile, read::macho::FatArch}; @@ -939,6 +940,104 @@ pub fn new_reqwest_blocking_client() -> reqwest::blocking::Client { .expect("http client must build with success") } +pub fn write_to_client_cache(key: &str, value: &str) -> Result<()> { + if let Some(base_dirs) = BaseDirs::new() { + let cache_dir = base_dirs.cache_dir(); + let cache_dir = cache_dir.join("sccache"); + std::fs::create_dir_all(&cache_dir)?; + let cache_file = cache_dir.join("client_cache"); + let mut file = std::fs::OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(true) + .open(cache_file)?; + let mut contents = String::new(); + file.read_to_string(&mut contents)?; + + let mut new_contents = String::new(); + let mut found = false; + for line in contents.lines() { + if line.starts_with(key) { + found = true; + new_contents.push_str(&format!("{} = \"{}\"\n", key, value)); + } else { + new_contents.push_str(line); + new_contents.push('\n'); + } + } + if !found { + new_contents.push_str(&format!("{} = \"{}\"\n", key, value)); + } + file.write_all(new_contents.as_bytes())?; + Ok(()) + } else { + Err(anyhow!( + "no valid home directory path could be retrieved from OS" + )) + } +} + +pub fn get_from_client_cache(key: &str) -> Result> { + if let Some(base_dirs) = BaseDirs::new() { + let cache_dir = base_dirs.cache_dir(); + let cache_dir = cache_dir.join("sccache"); + let cache_file = cache_dir.join("client_cache"); + if !cache_file.exists() { + return Ok(None); + } + let mut file = std::fs::File::open(&cache_file)?; + let mut contents = String::new(); + file.read_to_string(&mut contents)?; + for line in contents.lines() { + let mut parts = line.splitn(2, '='); + if let Some(k) = parts.next() { + if k.trim() == key { + if let Some(v) = parts.next() { + return Ok(Some(v.trim().trim_matches('"').to_string())); + } + } + } + } + Ok(None) + } else { + Err(anyhow!( + "no valid home directory path could be retrieved from OS" + )) + } +} + +pub fn remove_key_from_client_cache(key: &str) -> Result<()> { + if let Some(base_dirs) = BaseDirs::new() { + let cache_dir = base_dirs.cache_dir(); + let cache_dir = cache_dir.join("sccache"); + let cache_file = cache_dir.join("client_cache"); + if !cache_file.exists() { + return Ok(()); + } + let mut file = std::fs::OpenOptions::new() + .read(true) + .write(true) + .truncate(true) + .open(cache_file)?; + let mut contents = String::new(); + file.read_to_string(&mut contents)?; + let mut new_contents = String::new(); + for line in contents.lines() { + if !line.starts_with(key) { + new_contents.push_str(line); + new_contents.push('\n'); + } + } + file.write_all(new_contents.as_bytes())?; + Ok(()) + } else { + Err(anyhow!( + "no valid home directory path could be retrieved from OS" + )) + } +} + #[cfg(test)] mod tests { use super::{OsStrExt, TimeMacroFinder}; @@ -1049,4 +1148,16 @@ mod tests { finder.find_time_macros(b"TIMESTAMP__ This is larger than the haystack"); assert!(finder.found_timestamp()); } + + #[test] + fn test_client_cache() { + let key = "test_key"; + let value = "test_value"; + super::write_to_client_cache(key, value).unwrap(); + let result = super::get_from_client_cache(key).unwrap(); + assert_eq!(result, Some(value.to_string())); + super::remove_key_from_client_cache(key).unwrap(); + let result = super::get_from_client_cache(key).unwrap(); + assert_eq!(result, None); + } }