tests: fail when we cannot mv original os-release#373
tests: fail when we cannot mv original os-release#373evelikov merged 3 commits intodkms-project:masterfrom
Conversation
hyperupcall
left a comment
hyperupcall
left a comment
There was a problem hiding this comment.
Thanks for the fix - I just had one comment :)
| trap osrelease_cleanup EXIT | ||
|
|
||
| mv_osrelease() { | ||
| if [ -f "$1" ]; then |
There was a problem hiding this comment.
| if [ -f "$1" ]; then | |
| if [ -f "$1" ] || [ -L "$1" ]; then |
Should we keep previous behavior of moving a broken symbolic link?
There was a problem hiding this comment.
Good point - forgot that many distros have /etc/os-release as symlink.
The man page says "if it exists" where dkms code uses "-r". I'm inclined to change this + dkms code to "-e" which will stick to the man page + from brief test, it will skip if a broken symlink.
Maybe even throw in a test - both present but cannot be read, just for the fun of it.
What do you think?
There was a problem hiding this comment.
I think that sounds good - I like the idea of using -e. From my testing as well, it seems there is no difference compared to -r when dealing with broken symbolic links:
$ ls
$ ln -s a b
'b' -> 'a'
$ cat b
cat: b: No such file or directory
$ cat a
cat: a: No such file or directory
$ [ -e b ] && echo 'yes'
$ [ -r b ] && echo 'yes'
$ [ -f b ] && echo 'yes'so the change should be compatible
The man page says "if it exists", so let's do that. In practical sense there should be no difference in behaviour since all Linux distros that I've seen use 0644 permissions for the file. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
If we cannot move the os-release file(s) then the test should error out - fix that. While here adjust the trap location, so it does not trigger on cp failure or when there's no file to copy in the first place. v2: - check with -e, as per `man os-release` Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
... otherwise we may alter the permissions and/or dereference the symlink. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
|
Added a prep commit updating dkms to use -e. Also added a follow-up commit to use |
|
The Gentoo CI is failing, which is pre-existing and unrelated to this PR. Merging |
3 participants
If we cannot move the os-release file(s) then the test should error out - fix that.
While here adjust the trap location, so it does not trigger on cp failure or when there's no file to copy in the first place.
Minor fixup of #351
@hyperupcall can you give this a quick look? I should have drank more coffee before reviewing 😅