Skip to content

chroot: quick fix for #2687 #2689

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
Merged
merged 5 commits into from
Oct 10, 2021
Merged

chroot: quick fix for #2687 #2689

merged 5 commits into from
Oct 10, 2021

Conversation

jhscheer
Copy link
Contributor

@sylvestre
Copy link
Contributor

Could you please add a test to make sure we don't regress?
thanks

@jhscheer jhscheer marked this pull request as draft September 19, 2021 10:51
@jhscheer
chroot: quick fix for uutils#2687
94fbe1e 
* fixes uutils#2687
* change error message to better mimic GNU
@jhscheer
Copy link
Contributor Author

Sorry I don't know how to implement a feasible test for this since root permission is required to trigger the relevant parts.

@jhscheer jhscheer marked this pull request as ready for review September 19, 2021 12:02
@sylvestre
Copy link
Contributor

oh, just that passing 0 argument doesn't panic :)
doesn't need more!

@jhscheer jhscheer mentioned this pull request Sep 19, 2021
Merged
@jhscheer
Copy link
Contributor Author

oh, just that passing 0 argument doesn't panic :)
doesn't need more!

I added a test. It depends on #2692 to be merged first.
(not sure if this is what you had in mind)

@jhscheer
chroot: move logic so it can be triggered by tests
8c0b7d1 
* move the command building logic before the `chroot` syscall so it
will be reachable by tests that don't have root permissions.
@jhscheer
Copy link
Contributor Author

I gave this some more thought and moved the the command building logic before the chroot syscall so it can be reached by the CICD tests which, as I've learned, cannot run tests with sudo.

sylvestre
sylvestre reviewed Sep 22, 2021
View reviewed changes
@jhscheer jhscheer requested a review from sylvestre October 2, 2021 10:13
@sylvestre sylvestre merged commit 54ae2fa into uutils:master Oct 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sylvestre sylvestre Awaiting requested review from sylvestre

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

Successfully merging this pull request may close these issues.

chroot: thread 'main' panicked
2 participants
@jhscheer @sylvestre