Read /usr/lib/os-release as fallback for /etc/os-release
#351
Conversation
hyperupcall
changed the title
os-release in /usr/lib if missing one in /etc/usr/lib/os-release as fallback for /etc/os-release
|
Sounds good! I'll add the tests and other changes within the next few days |
hyperupcall
force-pushed
the
hyperupcall-use-usrlib-fallback
branch
2 times, most recently
from
838142e
to
c8547d0
Compare
hyperupcall
force-pushed
the
hyperupcall-use-usrlib-fallback
branch
7 times, most recently
from
2020525
to
4ffa230
Compare
|
Added the tests and fixed the nit! |
run_test.sh
Outdated
| EOF | ||
|
|
||
| echo "Creating /etc/os-release" | ||
| cat > /etc/os-release <<< "$osrelease_contents" |
There was a problem hiding this comment.
I don't fully follow why we need the osrelease_contents. Can't we just mv cp the file back?
The trap is already smart enough to handle that.
There was a problem hiding this comment.
You're right! After all these years, my misunderstanding of cp was: if it encountered a symlink to copy, it would copy the symlink, not the backing target the symlink resolves to. I have change it to use cp :)
There was a problem hiding this comment.
I have pushed changes that fixes this. It would be nice to either just copy _etc-os-release or _usrlib-os-release instead of creating a new, _os-release file, but if /etc/os-release and /usr/lib/os-release are both relative symlinks, then this will fail. Because at the point where we need to copy, say, _etc-os-release back to /etc/os-release, the symlink will no longer properly resolve because it is in a different directory.
| @@ -2320,7 +2325,7 @@ unset CC CXX CFLAGS CXXFLAGS LDFLAGS | |||
| # Set important variables | |||
| current_kernel=$(uname -r) | |||
| current_os=$(uname -s) | |||
| running_distribution=$(distro_version) | |||
| running_distribution=$(distro_version) || exit | |||
There was a problem hiding this comment.
This seems quite odd, why do we need it?
There was a problem hiding this comment.
distro_version has code that is responsible for exiting the program on error (die 4 ...). Beacuse this code is running in a subshell, the die does not exit the program, but only exits the subshell. This means that the command running_distribution=$(distro_version) only yields a non-zero exit code. Since errexit is not set, this means that the next commands are ran (not exiting).
This is not what we want because we wanted the die to actually exit the program, not just the subshell. So we add the || exit to ensure our program is actually exited. (we want to run the exit out of the subshell)
There was a problem hiding this comment.
It seems this wasn't done before, which means that if /etc/os-release wasn't previously present, then the error would show up on screen, and the rest of the script would run, perhaps doing mysterious and myschevious things to the user's system.
There was a problem hiding this comment.
Can i request splitting the change into separate commit (as part of this PR) and adding some/all of the above analysis in the commit message.
|
Apart from the comments (most notably the |
|
@hyperupcall did you have the chance to respin this PR and address the comments? |
|
Yes! I'll have this done within the next several hours. My apologies for the wait |
hyperupcall
force-pushed
the
hyperupcall-use-usrlib-fallback
branch
from
c880984
to
155a850
Compare
`distro_version` contains code that can run `exit`, which is supposed to exit the whole process. However, `distro_version` runs in a subshell. By default in Bash, this means that `exit` will only exit the subshell, and set `?` to that exit code. The rest of script will execute, despite the error. There are two ways to fix this: - Enable the POSIX shell option `errexit` for the entire script - Manually check for non-zero exit code and "cascade the error" The latter was chosen because it is a localized change that does not affect the behavior of the entire script.
hyperupcall
force-pushed
the
hyperupcall-use-usrlib-fallback
branch
from
155a850
to
fbc2548
Compare
Closes #345