Skip to content

Commit

Permalink
Simplify OptionParser fields. (pantsbuild#21689)
Browse files Browse the repository at this point in the history
Previously the OptionParser held explicit references to the 
`passthrough_args` and `args_tracker` of its ArgsReader.

However this denormalization can lead to inconsistency
if we were to want to replace the ArgsReader during 
options bootstrapping.

Now that we have the `as_any()` mechanism we can downcast
the ArgsReader and get those fields out of it as needed, so 
now there is only one reference into the args reader and its
internals.

This PR also adds a field to the `Args` struct to store the
original strings the `Args` was parsed from.

Both of these are preliminaries for an upcoming change
that ports the CLI Alias mechanism to Rust.
  • Loading branch information
benjyw authored Nov 25, 2024
1 parent 9d98df4 commit f8d5e37
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 27 deletions.
20 changes: 14 additions & 6 deletions src/rust/engine/options/src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ impl Arg {

#[derive(Debug)]
pub struct Args {
// The arg strings this struct was instantiated with.
#[allow(dead_code)]
arg_strs: Vec<String>,

// The structured args parsed from the arg strings.
args: Vec<Arg>,
passthrough_args: Option<Vec<String>>,
}
Expand All @@ -97,14 +102,16 @@ impl Args {
// Create an Args instance with the provided args, which must *not* include the
// argv[0] process name.
pub fn new<I: IntoIterator<Item = String>>(arg_strs: I) -> Self {
let arg_strs = arg_strs.into_iter().collect::<Vec<_>>();
let mut args: Vec<Arg> = vec![];
let mut passthrough_args: Option<Vec<String>> = None;
let mut scope = Scope::Global;
let mut args_iter = arg_strs.into_iter();

let mut args_iter = arg_strs.iter();
while let Some(arg_str) = args_iter.next() {
if arg_str == "--" {
// We've hit the passthrough args delimiter (`--`).
passthrough_args = Some(args_iter.collect::<Vec<String>>());
passthrough_args = Some(args_iter.cloned().collect::<Vec<String>>());
break;
} else if arg_str.starts_with("--") {
let mut components = arg_str.splitn(2, '=');
Expand All @@ -129,15 +136,16 @@ impl Args {
Some(value.to_string())
},
});
} else if is_valid_scope_name(&arg_str) {
scope = Scope::Scope(arg_str)
} else if is_valid_scope_name(arg_str) {
scope = Scope::Scope(arg_str.to_string())
} else {
// The arg is a spec, so revert to global context for any trailing flags.
scope = Scope::Global;
}
}

Self {
arg_strs,
args,
passthrough_args,
}
Expand Down Expand Up @@ -200,8 +208,8 @@ impl ArgsReader {
}
}

pub fn get_passthrough_args(&self) -> Option<&Vec<String>> {
self.args.passthrough_args.as_ref()
pub fn get_passthrough_args(&self) -> Option<Vec<String>> {
self.args.passthrough_args.clone()
}

pub fn get_tracker(&self) -> Arc<ArgsTracker> {
Expand Down
4 changes: 2 additions & 2 deletions src/rust/engine/options/src/args_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ fn test_passthrough_args() {
assert_string("debug", option_id!(-'l', "level"));

assert_eq!(
Some(&vec![
Some(vec![
"--passthrough0".to_string(),
"passthrough1".to_string(),
"-p".to_string(),
Expand All @@ -404,7 +404,7 @@ fn test_passthrough_args() {
fn test_empty_passthrough_args() {
let args = mk_args(["-ldebug", "--foo=bar", "--"]);

assert_eq!(Some(&vec![]), args.get_passthrough_args());
assert_eq!(Some(vec![]), args.get_passthrough_args());
}

#[test]
Expand Down
32 changes: 17 additions & 15 deletions src/rust/engine/options/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ pub use self::config::ConfigSource;
use self::config::{Config, ConfigReader};
pub use self::env::Env;
use self::env::EnvReader;
use crate::args::ArgsTracker;
use crate::fromfile::FromfileExpander;
use crate::parse::Parseable;
pub use build_root::BuildRoot;
Expand Down Expand Up @@ -310,8 +309,6 @@ pub struct DictOptionValue<'a> {
pub struct OptionParser {
sources: BTreeMap<Source, Arc<dyn OptionsSource>>,
include_derivation: bool,
passthrough_args: Option<Vec<String>>,
args_tracker: Arc<ArgsTracker>,
}

impl OptionParser {
Expand All @@ -338,8 +335,6 @@ impl OptionParser {
);

let args_reader = ArgsReader::new(args, fromfile_expander.clone());
let args_tracker = args_reader.get_tracker();
let passthrough_args = args_reader.get_passthrough_args().cloned();

let mut sources: BTreeMap<Source, Arc<dyn OptionsSource>> = BTreeMap::new();
sources.insert(
Expand All @@ -350,8 +345,6 @@ impl OptionParser {
let mut parser = OptionParser {
sources: sources.clone(),
include_derivation: false,
passthrough_args: None,
args_tracker: args_tracker.clone(),
};

fn path_join(prefix: &str, suffix: &str) -> String {
Expand Down Expand Up @@ -424,8 +417,6 @@ impl OptionParser {
parser = OptionParser {
sources: sources.clone(),
include_derivation: false,
passthrough_args: None,
args_tracker: args_tracker.clone(),
};

if allow_pantsrc
Expand Down Expand Up @@ -458,11 +449,10 @@ impl OptionParser {
}
}
}

Ok(OptionParser {
sources,
include_derivation,
passthrough_args,
args_tracker,
})
}

Expand Down Expand Up @@ -702,12 +692,24 @@ impl OptionParser {
})
}

pub fn get_passthrough_args(&self) -> Option<&Vec<String>> {
self.passthrough_args.as_ref()
pub fn get_passthrough_args(&self) -> Result<Option<Vec<String>>, String> {
Ok(self.get_args_reader()?.get_passthrough_args())
}

pub fn get_unconsumed_flags(&self) -> Result<HashMap<Scope, Vec<String>>, String> {
Ok(self.get_args_reader()?.get_tracker().get_unconsumed_flags())
}

pub fn get_unconsumed_flags(&self) -> HashMap<Scope, Vec<String>> {
self.args_tracker.get_unconsumed_flags()
fn get_args_reader(&self) -> Result<&ArgsReader, String> {
if let Some(args_reader) = self
.sources
.get(&Source::Flag)
.and_then(|source| source.as_any().downcast_ref::<ArgsReader>())
{
Ok(args_reader)
} else {
Err("This OptionParser does not have command-line args as a source".to_string())
}
}

// Given a map from section name to valid keys for that section,
Expand Down
11 changes: 7 additions & 4 deletions src/rust/engine/src/externs/options.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright 2024 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

use pyo3::exceptions::PyException;
use pyo3::types::{PyBool, PyDict, PyFloat, PyInt, PyList, PyString, PyTuple};
use pyo3::{prelude::*, BoundObject};

Expand Down Expand Up @@ -499,21 +500,23 @@ impl PyOptionParser {
}

fn get_passthrough_args(&self) -> PyResult<Option<Vec<String>>> {
Ok(self.0.get_passthrough_args().cloned())
self.0.get_passthrough_args().map_err(PyException::new_err)
}

fn get_unconsumed_flags(&self) -> HashMap<String, Vec<String>> {
fn get_unconsumed_flags(&self) -> PyResult<HashMap<String, Vec<String>>> {
// The python side expects an empty string to represent the GLOBAL scope.
self.0
Ok(self
.0
.get_unconsumed_flags()
.map_err(PyException::new_err)?
.into_iter()
.map(|(k, v)| {
(
(if k.name() == "GLOBAL" { "" } else { k.name() }).to_string(),
v,
)
})
.collect()
.collect())
}

fn validate_config(
Expand Down

0 comments on commit f8d5e37

Please sign in to comment.