coreutils icon indicating copy to clipboard operation
coreutils copied to clipboard

fix: #8034 chown parse trailing colon as implicit group-is-user

Open MarcusGrass opened this issue 5 months ago • 11 comments

fixes #8034

https://www.gnu.org/software/coreutils/manual/html_node/chown-invocation.html#chown-invocation

owner‘:’

If a colon but no group name follows owner, that user is made the owner of the files and the group of the files is changed to owner’s login group.

That case wasn't covered by parse_spec.

It doesn't explicitly say in the above doc, but doing it with ids like chown 0: <dir> produces:

chown: invalid spec: '0:'

So that was also implemented here.

MarcusGrass avatar Jun 11 '25 18:06 MarcusGrass

please also add a test in https://github.com/uutils/coreutils/blob/main/tests/by-util/test_chown.rs thanks

sylvestre avatar Jun 11 '25 19:06 sylvestre

GNU testsuite comparison:

GNU test failed: tests/chown/separator. tests/chown/separator is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

github-actions[bot] avatar Jun 11 '25 20:06 github-actions[bot]

please also add a test in https://github.com/uutils/coreutils/blob/main/tests/by-util/test_chown.rs thanks

There was already a test for this, however since the change that test started blowing up, since in CI user != group in some cases, and the user isn't allowed to chown its created file to its group. Added an exception in that specific case for CI

MarcusGrass avatar Jun 12 '25 05:06 MarcusGrass

GNU testsuite comparison:

GNU test failed: tests/chown/separator. tests/chown/separator is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

github-actions[bot] avatar Jun 12 '25 06:06 github-actions[bot]

GNU testsuite comparison:

GNU test failed: tests/chown/separator. tests/chown/separator is passing on 'main'. Maybe you have to rebase?

github-actions[bot] avatar Jun 14 '25 14:06 github-actions[bot]

This broke the tests/chown/separator GNU test

RenjiSann avatar Jun 14 '25 19:06 RenjiSann

This broke the tests/chown/separator GNU test

Yeah, I'm trying to figure out how because it's not failing running them locally. I'm new to contributing here so it's a bit of slow going getting into trying to navigate the different CI runners and their subtle environmental differences to a regular unix-machine, as well as digging into the GNU tests and understanding why those may fail in CI but not locally. I did manage to locate the test-logs though:


FAIL: tests/chown/separator
===========================

chown: changing ownership of '.': Operation not permitted (os error 1)
FAIL tests/chown/separator.sh (exit status: 1)

Trying to decipher how that happened

MarcusGrass avatar Jun 15 '25 09:06 MarcusGrass

edit tests/chown/separator to add set -v on the top and add debug messages for each line like "echo $fail" it will help to pin point where the error is coming from

sylvestre avatar Jun 15 '25 09:06 sylvestre

edit tests/chown/separator to add set -v on the top and add debug messages for each line like "echo $fail" it will help to pin point where the error is coming from

I can't reproduce it locally sadly so modifying my script doesn't help to pinpoint this. However, since the error is caused by an ownership change permission, I'm guessing here that previously (since the test uses a trailing ':' it passed because the old behaviour ignored that. Now that it's implemented to use it, it's trying to change ownership of . to the CI user's group, which it's not allowed to do (even though that is indeed the correct behaviour). It makes a lot of sense, since that was seen in previous test failures (and again, it runs fine locally on a linux machine).

I think that either the CI has to be set up with a more standard user which has a group and permissions to chown its created files to that group, or the test has to be skipped for CI. I think the former would likely be good in either case, since a lot of exceptions in the testing code had to be made to sidestep the permission issues.

MarcusGrass avatar Jun 15 '25 10:06 MarcusGrass

Some findings:

In CI, GNU chown-tests don't pass anymore because of how the GHA runner is set up, it's not allowed to switch permissions of . in the test to the runner's group, since the runner doesn't have a user-group.

Working around this is tricky. we could switch the job to run in a Ubuntu-container instead of on the runner. This has some pros: mainly more control and understanding of the environment. But some major cons: All GHA must run as root, installing KVM is tricky, more complicated pipeline since the container has to be prepped with dependencies. The 'all actions must run as root' thing is the most troublesome, since it'd make rust-cache and rust-toolchain actions inaccessible. The latter can be replaced with installing the toolchain for the running user in a script, but the former is more difficult. I dumped the infra messing-around in this branch.

I'm at a bit of a loss on what to do with this.

MarcusGrass avatar Jun 15 '25 18:06 MarcusGrass

GNU testsuite comparison:

GNU test failed: tests/chown/separator. tests/chown/separator is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/misc/tee (fails in this run but passes in the 'main' branch)

github-actions[bot] avatar Jun 15 '25 19:06 github-actions[bot]