install-rkt.sh: Fix issue #3123 #3127
Conversation
|
Can one of the admins verify this patch? |
|
ok to test |
|
@andir do you mind reformatting the commit message to comply to https://github.com/coreos/rkt/blob/master/CONTRIBUTING.md#format-of-the-commit-message? |
andir
force-pushed
the
fix-checkinsta
branch
2 times, most recently
from
f779b89 to
b83f252
Compare
Checkinstall (or rather installwatch) hooks into the mkdir process of install and prefixes the given paths while creating them. Since mkdir does a chdir($dir) after the process of creating the named directories it detects, that the creation must have failed and bails. The install-pak script continues with the next file in the list which is placed in the fake directory as expected and doesn't fail since the directory exists in the prefixed dir. Thus the first file in the loop is missing from the prefix dir. This is fixed with creating each directory that is required before the normal install routine continues. Fixes rkt#3123
|
Commit message should be fine now. Let me know if there is anything else that needs to be done. |
|
@andir thanks for the contribution, merged! |
| /usr/lib/tmpfiles.d/\\ | ||
| /usr/lib/systemd/system/ | ||
| do | ||
| mkdir -p \$dir 2>/dev/null || : |
There was a problem hiding this comment.
Why there is || : added at the end?
If one of this directories will not be created - checkinstall will still have a problem and if they already existed -p will not fail.
There was a problem hiding this comment.
|| : ensures those commands - which will fail due to described bug - exit with the code 0. One can argue if that is (still) required. I did run the script with set -e in order to see where the install-pak fails. The script did bail out on any failed command (exit_code != 0) - which is a pretty sane approach IMHO.
mkdir creates the dir in the prefix that checkinstall uses transparently. But seems to check the un-prefixed directory for existence. install does invoke mkdir.
Check for details in #3123 and the bug reports that are linked there.
There was a problem hiding this comment.
#3123 is where I come from.
I know what does || : but
- this diff does not include
-ein shebang mkdir -pwill fail only if there was fs problem (lack of inodes, fs mounted in ro)
so|| :seems to be remainder of testing process, which should be removed before pushing commit to GH.
It's only a nit. I only wanted to ensure if there is another reason why this construct is used here.
There was a problem hiding this comment.
You rigth with both points. mkdir -p does not abort the script since there is no -e. I still prefer to handle known/expected failures when writing scripts since the next guy debugging the code will not stumble upon the known behavior while trying to figure out something else.
I'd be in favor of adding -e to the shebang. I can provide the PR that cleans this up.
When install-pak (called from install-rkt.sh) fails at some point abort packaging. This is based on a discussion in rkt#3127 (diff) where it was agreed upon that aborting on unexpected failures would be desirable.
When install-pak (called from install-rkt.sh) fails at some point abort packaging. This is based on a discussion in rkt#3127 (diff) where it was agreed upon that aborting on unexpected failures would be desirable.
When install-pak (called from install-rkt.sh) fails at some point abort packaging. This is based on a discussion in rkt#3127 (diff) where it was agreed upon that aborting on unexpected failures would be desirable.
No description provided.