From f4f9841c918450649dbe43d60809607182adbc59 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Fri, 3 May 2024 15:37:51 -0700 Subject: [PATCH] cli and `local_server`: allow more complicated ports specifications The default is now `--port 8080-8090 --port 0` to try 11 ports, and then to tell the OS to use a random open port if all of those were busy. --- CHANGELOG.md | 4 ++ src/bin/diffedit3.rs | 95 ++++++++++++++++++++++++++++++++++---------- src/local_server.rs | 35 ++++++++-------- 3 files changed, 95 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2871273..71b36a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Breaking changes +* The `--port-range` option was removed. Instead, `--port` can now be repeated and accepts dash-separated ranges. For example, the default is now equivalent to `--port 8080-8090 --port 0`. + +* This also corresponds to a breaking change to the `local_server` library interface + ### New features ### Fixed bugs diff --git a/src/bin/diffedit3.rs b/src/bin/diffedit3.rs index 49ac1af..f4dbf65 100644 --- a/src/bin/diffedit3.rs +++ b/src/bin/diffedit3.rs @@ -1,5 +1,59 @@ +use std::str::FromStr; + use clap::Parser; use diffedit3::local_server::{run_server, MergeToolError}; +use thiserror::Error; + +#[derive(Debug, PartialEq, Eq, Clone)] +struct PortRange { + min_port: usize, + max_port: usize, +} + +impl PortRange { + fn iter(&self) -> impl Iterator { + self.min_port..=self.max_port + } +} + +#[derive(Debug, PartialEq, Eq, Clone, Error)] +enum ParsePortRangeError { + #[error( + "A port range must be one unsigned integer or two unsigned integers separated by a -, for \ + example 1234 or 1234-56789" + )] + SyntaxError, + #[error("The minimum port {0} cannot be greater than the maximum port {1}")] + InequalityError(usize, usize), +} + +impl FromStr for PortRange { + type Err = ParsePortRangeError; + + fn from_str(s: &str) -> Result { + let parse_usize = |s: &str| { + s.parse::() + .map_err(|_| ParsePortRangeError::SyntaxError) + }; + match s.split_once('-') { + None => { + let port = parse_usize(s)?; + Ok(Self { + min_port: port, + max_port: port, + }) + } + Some((first, second)) => { + let (min_port, max_port) = (parse_usize(first)?, parse_usize(second)?); + if min_port <= max_port { + Ok(Self { min_port, max_port }) + } else { + Err(ParsePortRangeError::InequalityError(min_port, max_port)) + } + } + } + } +} /// Compare three directories in a browser and allow editing one of them #[derive(Parser)] @@ -7,16 +61,21 @@ use diffedit3::local_server::{run_server, MergeToolError}; pub struct LocalServerCli { #[command(flatten)] lib_cli: diffedit3::fs::Cli, - /// Port to use for `http://127.0.0.1` - #[arg(long, short, conflicts_with = "port_range")] - port: Option, - // TODO: Change syntax from `--port-range 8080 8085` to `--port 8080-8085`? - /// Minimum and maximum port numbers to try for `http://127.0.0.1`. + /// Port or port range to use for `http://127.0.0.1` (can be repeated) + /// + /// Port 0 is a special value that instructs the OS to pick a random unused + /// port number. Ports 1-1023 are generally unavailable for use by + /// unpriveledged processes. /// - /// First, the minimum port is tried. If that is busy, the next port is - /// tried, and so on. - #[arg(long, num_args(2), default_values = ["8080", "8090"])] - port_range: Vec, + /// For example, the default is equivalent to `--port 8080-8090 --port 0`. + /// This means that port number 8080 is tried first. If that is busy, port + /// 8081 is tried, and so on. If all ports between 8080 and 8090 (inclusive) + /// are busy, port 0 is tried, meaning that the OS picks a random open port. + #[arg( + long, short, default_values = ["8080-8090", "0"], + value_name = "PORT_OR_PORT_RANGE", value_parser = PortRange::from_str + )] + port: Vec, /// Do not try to open the browser automatically /// /// See for a brief description of how the @@ -58,17 +117,13 @@ async fn main() -> Result<(), MergeToolError> { tracing_subscriber::fmt::init(); } - let (min_port, max_port) = match cli.port { - Some(port) => (port, port), - None => (cli.port_range[0], cli.port_range[1]), // Clap guarantees exactly two values - }; - if min_port > max_port { - exit_with_cli_error(format!( - "Error: the minimum port {min_port} cannot be greater than the maximum port \ - {max_port}." - )); - }; - if let Err(err) = run_server(input, min_port, max_port, !cli.no_browser).await { + if let Err(err) = run_server( + input, + Box::new(cli.port.into_iter().flat_map(|port| port.iter())), + !cli.no_browser, + ) + .await + { std::process::exit(match err { MergeToolError::IOError(err) => { eprintln!("{err}"); diff --git a/src/local_server.rs b/src/local_server.rs index 499b1f4..55aae1e 100644 --- a/src/local_server.rs +++ b/src/local_server.rs @@ -16,6 +16,7 @@ use thiserror::Error; use crate::DataInterface; type DataInterfacePointer = Arc>>; +pub type PortsToTry = Box>; #[derive(rust_embed::RustEmbed)] #[folder = "webapp/dist/"] @@ -131,8 +132,7 @@ fn acceptor_to_socket_address( pub async fn run_server( input: Box, - min_port: usize, - max_port: usize, + ports_to_try: PortsToTry, open_browser: bool, ) -> Result<(), MergeToolError> { let (terminate_channel, mut terminate_rx): (ExitCodeSender, _) = tokio::sync::mpsc::channel(10); @@ -147,21 +147,19 @@ pub async fn run_server( .nest("/", EmbeddedFilesEndpoint::::new()) .nest("/api", apis); - let mut port = min_port; - let mut error = None; - let acceptor = loop { - if port > max_port { - return Err(MergeToolError::FailedToOpenPort(error.unwrap())); + let acceptor = 'acceptor: { + let mut error = None; + for port in ports_to_try { + let listener = TcpListener::bind(format!("127.0.0.1:{}", port)); + match listener.into_acceptor().await { + Ok(a) => break 'acceptor a, + Err(err) => { + eprintln!("Couldn't bind to port {port}."); + error = Some(err) + } + }; } - let listener = TcpListener::bind(format!("127.0.0.1:{}", port)); - match listener.into_acceptor().await { - Ok(a) => break a, - Err(err) => { - eprintln!("Couldn't bind to port {port}."); - error = Some(err) - } - }; - port += 1; + return Err(MergeToolError::FailedToOpenPort(error.unwrap())); }; // Get the actual address we bound to. The primary reason to do this instead of @@ -212,13 +210,12 @@ pub async fn run_server( /// Initialize tokio correctly and call `run_server` pub fn run_server_sync( input: Box, - min_port: usize, - max_port: usize, + ports_to_try: PortsToTry, open_browser: bool, ) -> Result<(), MergeToolError> { tokio::runtime::Builder::new_multi_thread() .enable_all() .build() .unwrap() - .block_on(run_server(input, min_port, max_port, open_browser)) + .block_on(run_server(input, ports_to_try, open_browser)) }