Skip to content

bugfix: make spack pkg grep respect windows CLI limits#50520

Merged
tgamblin merged 9 commits intodevelopfrom
bugfix-spack-pkg-grep-windows
Jun 2, 2025
Merged

bugfix: make spack pkg grep respect windows CLI limits#50520
tgamblin merged 9 commits intodevelopfrom
bugfix-spack-pkg-grep-windows

Conversation

@tgamblin
Copy link
Copy Markdown
Member

@tgamblin tgamblin commented May 17, 2025

spack pkg grep can construct command lines that are too long for Windows, i.e. command lines that are longer than 32768 characters.

This makes spack pkg grep respect the Windows limit by default, and gets the unix limit from sysconfig.

  • Add a new spack.cmd.group_arguments function to create CLI-safe arg groups
  • Default to max 500 elements or 32768 chars, whichever comes first
  • If sysconfig is available, get SC_ARG_MAX and use that for max chars
  • Add test for group_arguments

@spackbot-app spackbot-app bot added commands core PR affects Spack core functionality workflow labels May 17, 2025
@tgamblin tgamblin force-pushed the bugfix-spack-pkg-grep-windows branch from 3d7766e to aeaa2bb Compare May 19, 2025 00:32
@spackbot-app spackbot-app bot added the tests General test capability(ies) label May 19, 2025
@tgamblin tgamblin force-pushed the bugfix-spack-pkg-grep-windows branch 3 times, most recently from 42c1396 to 8048101 Compare May 19, 2025 23:12
`spack pkg grep` can construct command lines that are too long for Windows,
i.e. command lines that are longer than 32768 characters.

This makes `spack pkg grep` respect the Windows limit by default, and
gets the unix limit from `sysconfig`.

- [x] Add a new `spack.cmd.group_arguments` function to create CLI-safe arg groups
- [x] Default to max 500 elements or 32768 chars, whichever comes first
- [x] If sysconfig is available, get `SC_ARG_MAX` and use that for max chars
- [x] Clean up output handling in `test_pkg_grep` test
- [x] Add test for `group_arguments`

Signed-off-by: Todd Gamblin <[email protected]>
@tgamblin tgamblin force-pushed the bugfix-spack-pkg-grep-windows branch from 8048101 to be57175 Compare May 19, 2025 23:18
tgamblin added 2 commits May 19, 2025 16:22
Spack's `Executable` class isn't properly returning a called process's
return code when it fails with a `CalledProcessError`.  Record it before
raising a `ProcessError` so that client code can query it later.

Signed-off-by: Todd Gamblin <[email protected]>
@tgamblin tgamblin requested a review from johnwparent May 20, 2025 08:04
@tgamblin tgamblin marked this pull request as ready for review May 20, 2025 08:04
@johnwparent
Copy link
Copy Markdown
Contributor

johnwparent commented Jun 2, 2025

@tgamblin Would you like me to push up some of the executable script tests for the windows side?

Copy link
Copy Markdown
Contributor

@johnwparent johnwparent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, nothing to hold up a merge.

@tgamblin tgamblin merged commit 89f220a into develop Jun 2, 2025
36 checks passed
@tgamblin tgamblin deleted the bugfix-spack-pkg-grep-windows branch June 2, 2025 23:28
@tgamblin
Copy link
Copy Markdown
Member Author

tgamblin commented Jun 2, 2025

@johnwparent thanks! can yo duo that as a follow-on?

@johnwparent
Copy link
Copy Markdown
Contributor

@johnwparent thanks! can yo duo that as a follow-on?

Absolutely, LGTM

kshea21 pushed a commit to kshea21/spack that referenced this pull request Jun 18, 2025
`spack pkg grep` can construct command lines that are too long for Windows,
i.e. command lines that are longer than 32768 characters.

This makes `spack pkg grep` respect the Windows limit by default, and gets
the unix limit from `sysconfig`.

- [x] Add a new `spack.cmd.group_arguments` function to create CLI-safe arg groups
- [x] Default to max 500 elements or 32768 chars, whichever comes first
- [x] If sysconfig is available, get `SC_ARG_MAX` and use that for max chars
- [x] Make `Executable` always return an error if it's not successful
- [x] Add test for `group_arguments`

---------

Signed-off-by: Todd Gamblin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands core PR affects Spack core functionality tests General test capability(ies) utilities workflow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants