Skip to content

feature(install) - add group & owner support + a refactor #1641

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

Merged
merged 6 commits into from
Dec 12, 2020

Conversation

sylvestre
Copy link
Contributor

@sylvestre sylvestre commented Nov 17, 2020

For this, move the chown & chgrp functions to uucore
As we need this feature for install too

@sylvestre
Copy link
Contributor Author

@rivy @Arcterus @nbraud
show_info is using executable!() which uses module_path() to get the name of the program.

In this PR, I would like to use the actual binary name instead of uucore. Otherwise, it causes unexpected output and test failure like:

  left: `"uucore: changing group of \'/tmp\': Operation not permitted (os error 1)"`,
 right: `"chgrp: changing group of \'/tmp\': Operation not permitted (os error 1)"`', tests/common/util.rs:132:9

Any idea how to fix that? thanks

@rivy
Copy link
Member

rivy commented Nov 18, 2020

@rivy @Arcterus @nbraud
show_info is using executable!() which uses module_path() to get the name of the program.

In this PR, I would like to use the actual binary name instead of uucore. Otherwise, it causes unexpected output and test failure like:

  left: `"uucore: changing group of \'/tmp\': Operation not permitted (os error 1)"`,
 right: `"chgrp: changing group of \'/tmp\': Operation not permitted (os error 1)"`', tests/common/util.rs:132:9

Any idea how to fix that? thanks

Yeah, don't implement it as a straight copy of the code... avoid UI I/O within uucore and instead refactor to return Result<...>'s. Then, you can handle the results within the specific util itself including any UI.

@rivy
Copy link
Member

rivy commented Nov 18, 2020

@sylvestre , I know I read something about why you wanted to do this... but I can't find the email/post.
Would you reiterate your reasoning for the move?

@sylvestre
Copy link
Contributor Author

Would you reiterate your reasoning for the move?

@rivy because the "install" command can change the ownership & group of a file. As we are already doing this for chgrp & chown, I think it would be better to have it uucore and make them reusable by both programs.

@sylvestre
Copy link
Contributor Author

Yeah, don't implement it as a straight copy of the code... avoid UI I/O within uucore and instead refactor to return Result<...>'s. Then, you can handle the results within the specific util itself including any UI.

I was hoping for something easier but you are right, I will do that, thanks :)

@sylvestre sylvestre force-pushed the install-owner-group branch 3 times, most recently from 110b0c6 to f64018b Compare November 19, 2020 21:50
@sylvestre sylvestre changed the title WIP: Install - add group&owner support feature(install) - add group & owner support + a refactor Nov 19, 2020
@sylvestre sylvestre requested review from rivy and Arcterus and removed request for rivy November 19, 2020 22:08
@sylvestre sylvestre merged commit 94571ec into uutils:master Dec 12, 2020
@sylvestre sylvestre deleted the install-owner-group branch December 12, 2020 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants