Skip to content

fix(parse_size): Add cross-platform total_physical_memory implementat… #9653

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.

Sign up for GitHub

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

Jump to bottom
Open
naoNao89 wants to merge 13 commits into
base: main
Choose a base branch
from
Open

fix(parse_size): Add cross-platform total_physical_memory implementat… #9653

naoNao89 wants to merge 13 commits into from
+472 −28

Conversation

@naoNao89
Copy link
Contributor

@naoNao89 naoNao89 commented Dec 13, 2025

Implements total_physical_memory() for macOS, FreeBSD, OpenBSD, and NetBSD using platform-specific sysctl commands. Enables sort -S 50% to work on all BSD-based systems.

Fixes #7257

@naoNao89 naoNao89 force-pushed the feat/cross-platform-percent-buffer-size branch from 401a9fe to 8fae6fc Compare December 13, 2025 17:42
@github-actions
Copy link

github-actions bot commented Dec 13, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)

@naoNao89 naoNao89 force-pushed the feat/cross-platform-percent-buffer-size branch 4 times, most recently from 80fa255 to 34d1ef3 Compare December 13, 2025 18:43
@github-actions
Copy link

github-actions bot commented Dec 13, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)

@naoNao89 naoNao89 force-pushed the feat/cross-platform-percent-buffer-size branch from 34d1ef3 to f57d398 Compare December 13, 2025 19:58
@github-actions
Copy link

github-actions bot commented Dec 13, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)

@lcheylus
Copy link
Contributor

lcheylus commented Dec 16, 2025

LGTM for FreeBSD and OpenBSD to get "Total physical memory" via sysctl (libc or direct call).

sylvestre
sylvestre requested changes Dec 25, 2025
View reviewed changes
Copy link
Contributor

@sylvestre sylvestre left a comment

Choose a reason for hiding this comment

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

this patch has a lot of duplication, could you please have a look? thanks

@naoNao89 naoNao89 force-pushed the feat/cross-platform-percent-buffer-size branch from f57d398 to 6c0f36a Compare December 26, 2025 11:02
@github-actions
Copy link

github-actions bot commented Dec 26, 2025

GNU testsuite comparison:

Note: The gnu test tests/basenc/bounded-memory is now being skipped but was previously passing.

@naoNao89
fix(parse_size): Add cross-platform total_physical_memory implementat…
87f02ec 
…ions

- Implements total_physical_memory() for macOS (hw.memsize)
- Implements total_physical_memory() for FreeBSD (hw.physmem)
- Implements total_physical_memory() for OpenBSD (hw.physmem)
- Implements total_physical_memory() for NetBSD (hw.physmem64)
- Enables 'sort -S 50%' to work on all BSD-based platforms
- Updates tests to include percentage buffer sizes for BSD platforms

Fixes uutils#7257
@naoNao89 naoNao89 force-pushed the feat/cross-platform-percent-buffer-size branch from 6c0f36a to 67839ab Compare December 28, 2025 21:59
@github-actions
Copy link

github-actions bot commented Dec 28, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/tee (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/tty/tty-eof is no longer failing!

@naoNao89
fix(sort): use native sysctl for percentage buffer-size on BSD platforms
e104905 
Replace subprocess-based sysctl calls with native libc::sysctl() syscalls
for macOS, FreeBSD, OpenBSD, and NetBSD to fix test_buffer_sizes failure
in CI environments.

The previous implementation spawned sysctl subprocesses which could fail
in sandboxed GitHub Actions runners. The native approach is more reliable,
performant, and includes subprocess fallback for robustness.

Fixes percentage-based buffer size parsing (e.g., sort -S 10%)
@naoNao89 naoNao89 force-pushed the feat/cross-platform-percent-buffer-size branch from 67839ab to e104905 Compare December 28, 2025 23:56
@github-actions
Copy link

github-actions bot commented Dec 29, 2025

GNU testsuite comparison:

Note: The gnu test tests/id/smack was skipped on 'main' but is now failing.
Note: The gnu test tests/mkdir/smack-no-root was skipped on 'main' but is now failing.
Note: The gnu test tests/mkdir/smack-root was skipped on 'main' but is now failing.

sylvestre and others added 4 commits December 29, 2025 13:39
@sylvestre
Merge branch 'main' into feat/cross-platform-percent-buffer-size
0e0be39
@naoNao89
fix: resolve CI failures for clippy and macOS uptime test
b77c0cd 
- Fix clippy::borrow-as-ptr lint error in parse_size.rs by using raw pointer syntax
- Fix macOS uptime test regex to accept integer load averages without decimals

Fixes OpenBSD and macOS CI test failures in PR uutils#9653
@naoNao89
Merge branch 'main' into feat/cross-platform-percent-buffer-size
48a087d
@naoNao89
fix: prevent subprocess fd inheritance causing tty-eof test hang
67b1812 
The bsd_sysctl_memory() subprocess fallback was inheriting parent's stdin,
causing commands like sort to hang when receiving EOF. This broke the
GNU tty-eof test which expects immediate exit on Ctrl+D.

Fix: Configure subprocess stdin/stdout/stderr to prevent fd inheritance:
- stdin: Stdio::null() (no input needed)
- stdout: Stdio::piped() (capture output)
- stderr: Stdio::null() (suppress errors)

Fixes GNU test: tests/tty/tty-eof
@github-actions
Copy link

github-actions bot commented Dec 30, 2025

GNU testsuite comparison:

GNU test failed: tests/tty/tty-eof. tests/tty/tty-eof is passing on 'main'. Maybe you have to rebase?

sylvestre
sylvestre reviewed Dec 30, 2025
View reviewed changes
naoNao89 added a commit to naoNao89/coreutils that referenced this pull request Dec 31, 2025
@naoNao89
feat(parse_size): Add comprehensive builder validation to prevent fra…
9216589 
…gile configurations

Addresses PR uutils#9653 review feedback about 'fragile commands' by implementing
fail-fast validation in Parser builder pattern.

Changes:
- Add ParserBuilderError enum with 8 validation error variants
- Refactor builder methods to return Result<&mut Self, ParserBuilderError>
- Implement comprehensive unit validation (57 valid units including k/m/g/t)
- Add cross-validation between builder settings (default_unit vs allow_list)
- Detect conflicts (b_byte_count with 'b' unit, '%' with size units)
- Update all call sites (sort, du, df, od) to handle new error types
- Fix invalid '%' unit in sort's parse_byte_count allow_list

Benefits:
- Configuration errors detected immediately (not during parsing)
- Clear error messages listing invalid/conflicting settings
- Maintains backward compatibility through explicit error reporting

Files modified:
- src/uucore/src/lib/features/parser/parse_size.rs (core implementation)
- src/uu/sort/src/sort.rs (error handling + fix invalid '%')
- src/uu/du/src/du.rs (error handling)
- src/uu/df/src/df.rs (error handling)
- src/uu/od/src/od.rs (error handling)
naoNao89 added a commit to naoNao89/coreutils that referenced this pull request Dec 31, 2025
@naoNao89
feat(parse_size): Add comprehensive builder validation to prevent fra…
b854560 
…gile configurations

Addresses PR uutils#9653 review feedback about 'fragile commands' by implementing
fail-fast validation in Parser builder pattern.

Changes:
- Add ParserBuilderError enum with 8 validation error variants
- Refactor builder methods to return Result<&mut Self, ParserBuilderError>
- Implement comprehensive unit validation (57 valid units including k/m/g/t)
- Add cross-validation between builder settings (default_unit vs allow_list)
- Detect conflicts (b_byte_count with 'b' unit, '%' with size units)
- Update all call sites (sort, du, df, od) to handle new error types
- Fix invalid '%' unit in sort's parse_byte_count allow_list

Benefits:
- Configuration errors detected immediately (not during parsing)
- Clear error messages listing invalid/conflicting settings
- Maintains backward compatibility through explicit error reporting

Files modified:
- src/uucore/src/lib/features/parser/parse_size.rs (core implementation)
- src/uu/sort/src/sort.rs (error handling + fix invalid '%')
- src/uu/du/src/du.rs (error handling)
- src/uu/df/src/df.rs (error handling)
- src/uu/od/src/od.rs (error handling)
naoNao89 and others added 2 commits December 31, 2025 00:34
@naoNao89
feat(parse_size): Add comprehensive builder validation to prevent fra…
08cc7b6 
…gile configurations

Addresses PR uutils#9653 review feedback about 'fragile commands' by implementing
fail-fast validation in Parser builder pattern.

Changes:
- Add ParserBuilderError enum with 8 validation error variants
- Refactor builder methods to return Result<&mut Self, ParserBuilderError>
- Implement comprehensive unit validation (57 valid units including k/m/g/t)
- Add cross-validation between builder settings (default_unit vs allow_list)
- Detect conflicts (b_byte_count with 'b' unit, '%' with size units)
- Update all call sites (sort, du, df, od) to handle new error types
- Fix invalid '%' unit in sort's parse_byte_count allow_list

Benefits:
- Configuration errors detected immediately (not during parsing)
- Clear error messages listing invalid/conflicting settings
- Maintains backward compatibility through explicit error reporting

Files modified:
- src/uucore/src/lib/features/parser/parse_size.rs (core implementation)
- src/uu/sort/src/sort.rs (error handling + fix invalid '%')
- src/uu/du/src/du.rs (error handling)
- src/uu/df/src/df.rs (error handling)
- src/uu/od/src/od.rs (error handling)
@naoNao89
Merge branch 'main' into feat/cross-platform-percent-buffer-size
935b7c6
@github-actions
Copy link

github-actions bot commented Dec 31, 2025

GNU testsuite comparison:

Congrats! The gnu test tests/tty/tty-eof is no longer failing!

@github-actions
Copy link

github-actions bot commented Dec 31, 2025

GNU testsuite comparison:

Congrats! The gnu test tests/tty/tty-eof is no longer failing!

@naoNao89
fix(clippy): use efficient to_string by dereferencing &&str
72c5094 
OpenBSD has stricter clippy linting that requires dereferencing
&&str before calling to_string() to use the fast str specialization
instead of the slower blanket impl.
@github-actions
Copy link

github-actions bot commented Dec 31, 2025

GNU testsuite comparison:

GNU test failed: tests/tty/tty-eof. tests/tty/tty-eof is passing on 'main'. Maybe you have to rebase?

@naoNao89
fix: allow '%' suffix in buffer-size for percentage-based memory allo…
bb47cb3 
…cation

- Added '%' to sort's buffer-size allow_list for Unix-like systems
- Removed overly strict validation that prevented '%' from being
  mixed with other size units in the same allow_list
- The '%' suffix triggers special handling for percentage of physical
  memory, but other units remain valid in the same configuration
@github-actions
Copy link

github-actions bot commented Dec 31, 2025

GNU testsuite comparison:

Congrats! The gnu test tests/tty/tty-eof is no longer failing!

@naoNao89
fix: resolve libc crate import scope issue in parse_size.rs
1a77c70 
Move 'use nix::libc' to module level for BSD platforms to fix macOS
compilation error where libc::c_int was referenced in function signature
before the import was in scope. Also removed duplicate use statements
from individual functions.
sylvestre
sylvestre reviewed Dec 31, 2025
View reviewed changes
sylvestre
sylvestre reviewed Dec 31, 2025
View reviewed changes
@github-actions
Copy link

github-actions bot commented Dec 31, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sylvestre sylvestre sylvestre requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

sort: support percent arguments to -S option for non-Linux systems

3 participants

@naoNao89 @lcheylus @sylvestre