Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2.24 will no longer exclude targets prefixed with - #21930

Closed
aviau opened this issue Feb 6, 2025 · 8 comments
Closed

2.24 will no longer exclude targets prefixed with - #21930

aviau opened this issue Feb 6, 2025 · 8 comments
Assignees
Labels

Comments

@aviau
Copy link
Contributor

aviau commented Feb 6, 2025

Describe the bug

In 2.22, I could run the following:

$ pants fmt :: -folder::

In 2.24, I get the following error:

$ pants fmt :: -folder::
$ Unknown flag -f in global context

According to my understanding of the docs, it should work:

To ignore something, prefix the argument with -

Pants version
2.24

OS
I can repo on linux and macos

Workaround

It seems that --filter-address-regex can be used for similar results:

  • pants fmt :: --filter-address-regex=-^folder
@aviau aviau added the bug label Feb 6, 2025
@benjyw benjyw self-assigned this Feb 6, 2025
@benjyw
Copy link
Contributor

benjyw commented Feb 6, 2025

Presumably a bug in the new options parser. I will take a look.

@benjyw
Copy link
Contributor

benjyw commented Feb 8, 2025

Ah, yes, this is because I didn't do this TODO, and this case fell between the chairs. Will do it now. Thanks for the bug report @aviau !

@benjyw
Copy link
Contributor

benjyw commented Feb 9, 2025

OK, I have a fix. Will write a regression test and send it for review.

@aviau
Copy link
Contributor Author

aviau commented Feb 9, 2025

Great!

@benjyw
Copy link
Contributor

benjyw commented Feb 11, 2025

Fixed in #21943

benjyw added a commit that referenced this issue Feb 12, 2025
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: Huon Wilson <[email protected]>
tdyas pushed a commit that referenced this issue Feb 12, 2025
…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]>
@benjyw
Copy link
Contributor

benjyw commented Feb 12, 2025

There is a bandaid fix for 2.24.x branch in #21951

@benjyw
Copy link
Contributor

benjyw commented Feb 12, 2025

Will be fixed in 2.24.2 and 2.25.0 and beyond. Thanks for the bug report!

@benjyw benjyw closed this as completed Feb 12, 2025
@aviau
Copy link
Contributor Author

aviau commented Feb 13, 2025

Thanks for the quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants