coreutils
coreutils copied to clipboard
fix: #8034 chown parse trailing colon as implicit group-is-user
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.
please also add a test in https://github.com/uutils/coreutils/blob/main/tests/by-util/test_chown.rs thanks
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)
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
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)
GNU testsuite comparison:
GNU test failed: tests/chown/separator. tests/chown/separator is passing on 'main'. Maybe you have to rebase?
This broke the tests/chown/separator GNU test
This broke the
tests/chown/separatorGNU 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
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
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.
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.
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)