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.

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

sort: Allow percentages as values for buffer-size #4059

Closed
wants to merge 9 commits into from
Closed

sort: Allow percentages as values for buffer-size #4059

wants to merge 9 commits into from

Conversation

sallto
Copy link

@sallto sallto commented Oct 18, 2022

Fixes issue #3500 by adding support for percentags of total memory for the buffer-size flag. The sysinfo crate is used to determine the total memory of a system.

I used the sysinfo crate instead of hacking together unsafe code to work with libc. This makes the changed part crossplatform-compatible and realistically safer since sysinfo is a well maintained and actively developed library.

I used the error handling from the original code. The original program will default to using all of the memory of a system (not just available), while this implementation will panic with an error.

lmk if there are weird things I did as this is my first "production" rust code. Thanks!

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm2 is no longer failing!
GNU test failed: tests/misc/tee. tests/misc/tee is passing on 'main'. Maybe you have to rebase?

sylvestre
sylvestre reviewed Oct 18, 2022
View reviewed changes
src/uu/sort/src/sort.rs Outdated Show resolved Hide resolved
@sylvestre
Copy link
Contributor

Thanks
could you please add an integration test here https://github.com/uutils/coreutils/blob/main/tests/by-util/test_sort.rs too?

@sallto
Copy link
Author

sallto commented Oct 19, 2022

Added the tests!
FYI: The nix crate understandably breaks the macos build. With the sysinfo crate this worked. I don't know if this is a priority

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/misc/tee is no longer failing!

@sylvestre
Copy link
Contributor

yeah, it is a priority to keep all jobs green, sorry :)

sallto added 4 commits October 23, 2022 13:12
@sallto
sort: Allow percentages as values for buffer-size
a97f32e 
Fixes issue uutils#3500 by adding support for percentags of total memory for the buffer-size flag. The sysinfo crate is used to determine the total memory of a system.

Usage of sysinfo instead of custom code makes this part very multi-platform compatible (See sysinfo documentation for supported OSes). As well as avoiding introducing unsafe code from libc's sysinfo.
@sallto
sort: Added integration tests for buffer-size flag
2926982
@sallto
sort: Added crossplattform support.
bb50506 
Added crossplattform support by implementing get_total_memory seperately for linux,macos and windows using nix, libc and windows-sys.
@sallto
fix: sort: Tests failing for percentages in buffer-size flag on 32bit…
9830657 
… systems.

Tests with percentages of total memory failed on 32bit system since the testsystems had more than 2^32 Byte of memory.
@sallto
Copy link
Author

sallto commented Oct 26, 2022

Ok I rewrote that part with platform-specific code. I had to disable some tests on 32-bit since the CI/CD Pipeline has more than 2^32 bit of total ram.

@sallto sallto requested a review from sylvestre October 26, 2022 10:44
@github-actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/chown/separator. tests/chown/separator is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/misc/md5sum-parallel. tests/misc/md5sum-parallel is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/misc/sha1sum-vec. tests/misc/sha1sum-vec is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/misc/sha224sum. tests/misc/sha224sum is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/misc/sha256sum. tests/misc/sha256sum is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/misc/sha384sum. tests/misc/sha384sum is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/misc/sha512sum. tests/misc/sha512sum is passing on 'main'. Maybe you have to rebase?

@sallto
Copy link
Author

sallto commented Nov 7, 2022

What are the next steps now? I restarted the CI pipeline on the branch which made the macos build pass. So only the android test job fails (which it also does on them main branch). See https://github.com/sallto/coreutils/actions/runs/3383769224

sylvestre
sylvestre reviewed Nov 7, 2022
View reviewed changes
src/uu/sort/Cargo.toml Outdated Show resolved Hide resolved
sylvestre
sylvestre requested changes Mar 27, 2023
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.

needs to be rebased

@sylvestre
Copy link
Contributor

@sallto ping?

@sylvestre
Copy link
Contributor

no action for a while, closing.

@sylvestre sylvestre closed this Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sylvestre sylvestre sylvestre requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sallto @sylvestre