On 12.06.2011 02:37, Barry Jackson wrote: > On 11/06/11 18:03, Anssi Hannula wrote: >> >> - Best to add a comment in the %post script to remind that any new files >> need to have a %ghost entry created. >> (btw: an alternative idea is to create a post-script and the filelist >> at the same time in a single for loop in %build/%install, so that >> the lists can never get out of sync as they are created from a single >> list; however, your current solution is adequate as well) > > Note added - keep it simple. >
You added the note in the %files section, not in %post. The issue only happens if someone updates %post but not %files, so it doesn't make sense to put the reminder in %files :) >> - You don't check for failures. If the mktemp call fails, you extract >> the tarball into /root etc.etc.. You need to check for failures on >> the mktemp/mkdir/cd commands (mktemp failure can be detected by >> checking if $newtmp string is empty), or alternatively run "set -e" >> to make the shell exit on failures (this leaves the tmpdir polluted, >> but it doesn't matter as much as it is an uncommon failure case and >> /tmp is cleaned periodically anyway). > > Added some checks - with clean-up of previously created dirs etc on fail. > mkdir %{mytmp} > [[ -d %{mytmp} ]] || exit 1 > cd %{mytmp} cd is not guaranteed to work even if %mytmp exists. Add "|| exit 1". > cd ${tmpextdir} > tar jxf %{mytmp}/skype-%{version}.tar.bz2 Same here... Well, actually these two aren't that serious, since they can't be exploited by non-root anyway. I'm somewhat ok with them, as long as your mentor is as well and no one else objects. Note that your "if ! [[ -d %{tmpdir} ]]; then" check is not 100% necessary, as %tmpdir not existing is unlikely and causes no side effects. Other nits: - Use better variable names- %mytmp, $tmpextdir, %tmpdir are confusing. - The description starts with an empty line. Remove it. -- Anssi Hannula