Move CLI flag extraction into ArgSplitter #21943

benjyw · 2025-02-11T01:40:14Z

Previously, for legacy reasons, we had a separate pass for
processing flags, while the ArgSplitter dealt with finding
goals and specs. This turned out to be not just a code
redundancy issue, but it caused a bug: #21930 .

This PR cleans up the redundancy and smooths out
some ragged legacy edges in the Rust options parser:

Employs uniform terminology: Now all CLI args
(specs, goals, flags) are referred to as args,
while args that set option values are referred to
as flags. Previously we used flags and args
somewhat interchangeably to refer to the latter,
but also args to refer to the former. This clears
up the ambiguity.
Notably, the Arg struct is now called Flag, and
lives in flags.rs.
Gets rid of the previous Args struct and its logic,
reusing its name to now be just a simple holder of
arg strings prior to parsing.
Moves the flag-gathering logic, previously in the old Args,
into ArgSplitter.
Modify tests appropriately. Including removing testing
of arbitrary short (-f) flags, which happened to work
before, even though we only officially support a known
small set of such flags (-v, -h, -l).
Regression test for 2.24 will no longer exclude targets prefixed with - #21930.
Make passthru args an Option, so we can distinguish between
pants ... and pants ... -- (i.e., no passthru separator vs
a passthru separator with no args after it).

This creates a new empty release notes file for 2.26, since we're cutting the `2.25.x` branch (pantsbuild#21911) and starting the 2.26.x series.

benjyw · 2025-02-11T01:56:50Z

src/python/pants/option/errors.py

@@ -116,7 +116,7 @@ def __init__(self, flags: Tuple[str, ...], arg_scope: str):
        self.flags = flags
        self.arg_scope = arg_scope
        scope = f"scope {self.arg_scope}" if self.arg_scope else "global scope"
-        msg = f"Unknown flags {', '.join(self.flags)} on {scope}"
+        msg = f"Unknown {pluralize(len(self.flags), 'flag')} {', '.join(self.flags)} on {scope}"


Noticed this while debugging, and it's not worth a separate PR.

benjyw · 2025-02-11T01:57:47Z

src/rust/engine/options/src/arg_splitter.rs

@@ -26,7 +56,8 @@ pub struct PantsCommand {
    pub goals: Vec<String>,                        // Requested known goals.
    pub unknown_goals: Vec<String>,                // Any requested but unknown goals.
    pub specs: Vec<String>, // What to run against, e.g. targets or files/dirs.
-    pub passthru: Vec<String>, // Any remaining args specified after a -- separator.
+    pub(crate) flags: Vec<Flag>, // Option values set via flag.
+    pub passthru: Option<Vec<String>>, // Any remaining args specified after a -- separator.


I will do a future pass on visibility, some of these pub may be able to be pub(crate). But I wanted to get this bugfix in first.

benjyw · 2025-02-11T01:58:52Z

src/rust/engine/options/src/arg_splitter.rs

@@ -90,7 +123,7 @@ impl ArgSplitter {
        //  changes to plugins (as they can introduce new goals) or changes on the filesystem.
        //  We might want to deprecate this behavior and consistently assume that these are goals,
        //  since the user can always add a `./` prefix to override.
-        while let Some(arg) = unconsumed_args.pop() {
+        while let Some(arg) = unconsumed_args.next() {


The reversing-and-popping method of iterating was needlessly confusing.

benjyw · 2025-02-11T01:59:30Z

src/rust/engine/options/src/arg_splitter.rs

+                }
+                passthru = Some(remainder);
+            } else if arg.starts_with("--") {
+                let mut components = arg.splitn(2, '=');


This is moved here from the old Args impl.

benjyw · 2025-02-11T02:03:38Z

src/rust/engine/options/src/flags_tests.rs

 }

 #[test]
 fn test_bool() {
    let args = mk_args([
-        "-c=swallow",
+        "--unladen-capacity=swallow",


The old Args implementation happened to support arbitrary short flags, and we tested that, but the OptionsParser itself didn't support those, so this is not taking away any capability of Pants itself, just an overeagerness of the previous implementation.

benjyw · 2025-02-11T02:04:06Z

src/rust/engine/options/src/flags_tests.rs

@@ -372,41 +379,6 @@ fn test_nonexistent_optional_fromfile() {
    assert!(args.get_string(&option_id!("foo")).unwrap().is_none());
 }

-#[test]
-fn test_passthrough_args() {


We now get these from ArgSplitter (and it is tested there).

benjyw · 2025-02-11T02:04:44Z

src/rust/engine/options/src/lib.rs

-        // Now get the PantsCommand based on the expanded aliases.
-        // For historical reasons, getting the SplitArgs (for goals and specs) is done
-        // in ArgSplitter, in a separate pass from getting the Args (for CLI flags).
-        // TODO: Combine the two passes?


benjyw · 2025-02-11T02:05:45Z

src/rust/engine/options/src/lib.rs

@@ -563,22 +570,18 @@ impl OptionParser {

        let alias_map = cli_alias::create_alias_map(known_scopes_to_flags, &alias_strings)?;

-        // Step #4: Read any spec_files from alias-expanded args, env, config and rcfiles.
-
-        // Add the args reader back in, after expanding aliases.


We no longer need to hang on to the args reader. We just insert one, remove it, and reinsert a new one again.

benjyw · 2025-02-11T02:08:51Z

src/rust/engine/options/src/scope.rs

-    static ref SCOPE_NAME_RE: Regex = Regex::new(r"^(?:[a-z0-9_])+(?:-(?:[a-z0-9_])+)*$").unwrap();
-}
-
-pub(crate) fn is_valid_scope_name(name: &str) -> bool {


We now parse flags in the ArgSplitter, which has a list of known scopes.

huonw

Makes sense.

I'm a bit surprised we didn't catch the negative specs bug earlier with an integration test or similar? What do you think about adding some sort of higher level "target selection" integration test?

src/rust/engine/options/src/arg_splitter.rs

WorkerPants · 2025-02-12T00:14:21Z

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

❌ 2.24.x

I was unable to cherry-pick this PR to 2.24.x, likely due to merge-conflicts.

Steps to Cherry-Pick locally

To resolve:

(Ensure your git working directory is clean)

Run the following script to reproduce the merge-conflicts:

git fetch https://github.com/pantsbuild/pants main \
  && git fetch https://github.com/pantsbuild/pants 2.24.x \
  && git checkout -b cherry-pick-21943-to-2.24.x FETCH_HEAD \
  && git cherry-pick dd87b85b1101414409cba0060feb505392b13797

Fix the merge conflicts and commit the changes
Run build-support/cherry_pick/make_pr.sh "21943" "2.24.x"

Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary.

✔️ 2.25.x

Successfully opened #21950.

When you're done manually cherry-picking, please remove the needs-cherrypick label on this PR.

Thanks again for your contributions!

🤖 Beep Boop here's my run link

…1950) Previously, for legacy reasons, we had a separate pass for processing flags, while the ArgSplitter dealt with finding goals and specs. This turned out to be not just a code redundancy issue, but it caused a bug: #21930 . This PR cleans up the redundancy and smooths out some ragged legacy edges in the Rust options parser: - Employs uniform terminology: Now all CLI args (specs, goals, flags) are referred to as `args`, while args that set option values are referred to as `flags`. Previously we used `flags` and `args` somewhat interchangeably to refer to the latter, but also `args` to refer to the former. This clears up the ambiguity. - Notably, the `Arg` struct is now called `Flag`, and lives in `flags.rs`. - Gets rid of the previous `Args` struct and its logic, reusing its name to now be just a simple holder of arg strings prior to parsing. - Moves the flag-gathering logic, previously in the old `Args`, into `ArgSplitter`. - Modify tests appropriately. Including removing testing of arbitrary short (`-f`) flags, which happened to work before, even though we only officially support a known small set of such flags (`-v`, `-h`, `-l`). - Regression test for #21930. - Make passthru args an Option, so we can distinguish between `pants ...` and `pants ... --` (i.e., no passthru separator vs a passthru separator with no args after it). Co-authored-by: Benjy Weinberger <[email protected]> Co-authored-by: Huon Wilson <[email protected]>

We fix directly in 2.24.x, as we can't cherrypick the more comprehensive #21943 due to its reliance on a whole stack of changes that we don't want in 2.24.x

huonw and others added 3 commits February 6, 2025 15:36

Initialise 2.26.x release notes file (pantsbuild#21925)

d79ed9f

This creates a new empty release notes file for 2.26, since we're cutting the `2.25.x` branch (pantsbuild#21911) and starting the 2.26.x series.

WIP

09a0569

Merge branch 'main' into fix_unknown_flag_handling

f07eefc

benjyw added the release-notes:not-required PR doesn't require mention in release notes label Feb 11, 2025

notes

6bfac2e

benjyw force-pushed the fix_unknown_flag_handling branch from e4d7389 to 6bfac2e Compare February 11, 2025 01:55

benjyw mentioned this pull request Feb 11, 2025

2.24 will no longer exclude targets prefixed with - #21930

Closed

benjyw requested review from cburroughs and huonw February 11, 2025 01:56

benjyw commented Feb 11, 2025

View reviewed changes

Remove unused function and its tests

e0a1ddd

benjyw commented Feb 11, 2025

View reviewed changes

benjyw added needs-cherrypick and removed release-notes:not-required PR doesn't require mention in release notes labels Feb 11, 2025

benjyw added this to the 2.24.x milestone Feb 11, 2025

huonw approved these changes Feb 11, 2025

View reviewed changes

src/rust/engine/options/src/arg_splitter.rs Outdated Show resolved Hide resolved

benjyw added 2 commits February 11, 2025 12:31

Fix tests

1bb6efe

Fix argv

5be9244

benjyw merged commit dd87b85 into pantsbuild:main Feb 12, 2025
24 checks passed

benjyw deleted the fix_unknown_flag_handling branch February 12, 2025 00:13

WorkerPants added the auto-cherry-picking-failed Auto Cherry-Picking Failed label Feb 12, 2025

benjyw mentioned this pull request Feb 12, 2025

Temporary fix for spurious errors on negative specs. #21951

Merged

benjyw removed needs-cherrypick auto-cherry-picking-failed Auto Cherry-Picking Failed labels Feb 12, 2025

benjyw modified the milestones: 2.24.x, 2.25.x Feb 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move CLI flag extraction into ArgSplitter #21943

Move CLI flag extraction into ArgSplitter #21943

benjyw commented Feb 11, 2025

benjyw Feb 11, 2025

benjyw Feb 11, 2025

benjyw Feb 11, 2025

benjyw Feb 11, 2025 •

edited

Loading

benjyw Feb 11, 2025

benjyw Feb 11, 2025

benjyw Feb 11, 2025

benjyw Feb 11, 2025

benjyw Feb 11, 2025

huonw left a comment

WorkerPants commented Feb 12, 2025

Move CLI flag extraction into ArgSplitter #21943

Move CLI flag extraction into ArgSplitter #21943

Conversation

benjyw commented Feb 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw left a comment

Choose a reason for hiding this comment

WorkerPants commented Feb 12, 2025

❌ 2.24.x

✔️ 2.25.x

benjyw Feb 11, 2025 •

edited

Loading