The geoip-1.6.0-r2.ebuild is dramatically wrong! First of all, doesn't create /usr/share/GeoIP/, so action in pkg_postinst() (attempt to download data files to that dir) just do nothing (siltently), cuz mentioned directory is not exists. It has been fixed in my ebuild (will be attached) w/ keepdir call. The second flaw laid in pkg_postrm() action -- even if mentioned dir will be created before and filled w/ data files, it will be *unconditionally* removed here! It doesn't recognize if a package was reinstalled/upgraded or just completely removed (only in latter case it must (really?) remove data files!) And finally -- provided ${FILESDIR}/geoipupdate-r3.sh do not respect ${ROOT}. Being running from pkg_postinst() action it may violate 9.1.11 of current PMS-5! Reproducible: Always Steps to Reproduce: 1. (re)install dev-libs/geoip 2. check presence of /usr/share/GeoIP/ Actual Results: the directory do not exist Expected Results: directory w/ few *.dat files downloaded
Created attachment 378834 [details, diff] fixes for geoip-1.6.0-r2.ebuild
Comment on attachment 378834 [details, diff] fixes for geoip-1.6.0-r2.ebuild >@@ -39,12 +39,16 @@ > prune_libtool_files > > newsbin "${FILESDIR}"/geoipupdate-r3.sh geoipupdate.sh >+ keepdir "${ROOT}/usr/share/GeoIP" No need for ROOT there - keepdir should take care of that. >-pkg_postrm() { >- rm -r "${ROOT}/usr/share/GeoIP/" >+pkg_prerm() { >+ # ATTENTION Check if this is uninstall action (i.e. not just a reinstall/upgrade) >+ if [ -z "${REPLACED_BY_VERSION}" ]; then >+ rm -r "${ROOT}"/usr/share/GeoIP/*.dat >+ fi > } Surely there must be an easier way.
(In reply to Alex Turbov from comment #0) > The geoip-1.6.0-r2.ebuild is dramatically wrong! Dramatically! Could you keep this technical please? > First of all, doesn't create /usr/share/GeoIP/ Fixed. > it will be *unconditionally* removed here! Fixed. > And finally -- provided ${FILESDIR}/geoipupdate-r3.sh do not respect > ${ROOT}. Being running from pkg_postinst() action it may violate 9.1.11 of > current PMS-5! You didn't show a patch for that one. What is the solution?
(In reply to Jeroen Roovers from comment #2) > >-pkg_postrm() { > >- rm -r "${ROOT}/usr/share/GeoIP/" > >+pkg_prerm() { > >+ # ATTENTION Check if this is uninstall action (i.e. not just a reinstall/upgrade) > >+ if [ -z "${REPLACED_BY_VERSION}" ]; then > >+ rm -r "${ROOT}"/usr/share/GeoIP/*.dat > >+ fi > > } > > Surely there must be an easier way. what's "hard" in this way? REPLACED_BY_VERSION will be empty in case of "pure" uninstall… how one more `if` makes it "hard"?
Comment on attachment 378834 [details, diff] fixes for geoip-1.6.0-r2.ebuild >+ if [ -z "${REPLACED_BY_VERSION}" ]; then >+ rm -r "${ROOT}"/usr/share/GeoIP/*.dat >+ fi For one thing, I don't see what the --recursive is for. For another, it could be a oneliner [[ .. ]] && foo
(In reply to Jeroen Roovers from comment #5) > Comment on attachment 378834 [details, diff] [details, diff] > fixes for geoip-1.6.0-r2.ebuild > > >+ if [ -z "${REPLACED_BY_VERSION}" ]; then > >+ rm -r "${ROOT}"/usr/share/GeoIP/*.dat > >+ fi > I've thought you want to propose some different approach… > For one thing, I don't see what the --recursive is for. yep. I've just forgot to remove it after change the action from pkg_postrm() to pkg_prerm()… > For another, it could be a oneliner [[ .. ]] && foo as you wish, go on do it the way you like… as for me `if` is a little "readable" than oneliner…
That still doesn't solve the issue of the script not using ROOT. It never actually did that before.
(In reply to Jeroen Roovers from comment #3) > > > And finally -- provided ${FILESDIR}/geoipupdate-r3.sh do not respect > > ${ROOT}. Being running from pkg_postinst() action it may violate 9.1.11 of > > current PMS-5! > > You didn't show a patch for that one. What is the solution? possible solution could be to pass a desired path as an argument and if omitted, set GEOIPDIR to default /usr/share/GeoIP. so being running from the ebuild it will respect $ROOT.
The script in fact says "403 Forbidden / Rate limited exceeded, please try again in 24 hours." so I guess testing from this IP address is a problem for now.
I might also address the issue of the Gentoo written script using wget, while net-misc/geoipupdate uses curl. The script should at least support both, if not $FETCHCOMMAND or something else entirely. I've seen examples of using only bash internals to do downloads... :)
I have changed the way the geoipupdate.sh script is installed in -r3.
(In reply to Jeroen Roovers from comment #11) > I have changed the way the geoipupdate.sh script is installed in -r3. as for me, it doesn't looks like a good idea to leave some files after a package gets removed... if I'm going to remove smth I want to remove all related files (and yes, I'm always review after such uninstalls what files/dirs wasn't removed from my system... and that's annoys me all the time). having such packages reminds me a Windows way... -- after half of the year, your system turns into dust-heap... btw, maybe it makes some sense to talk (in ML?) about some option for package manager to "completely remove all files related" (i.e. even config protected! -- it is too often happened for me, if I decide to remove some package forever, I don't want to leave it's configuration files, but enforced to review uninstall messages and remove them manually...)
(In reply to Alex Turbov from comment #12) > (In reply to Jeroen Roovers from comment #11) > > I have changed the way the geoipupdate.sh script is installed in -r3. > > as for me, it doesn't looks like a good idea to leave some files after a > package gets removed... if I'm going to remove smth I want to remove all > related files That wasn't disputed. > btw, maybe it makes some sense to talk (in ML?) about some option for > package manager to "completely remove all files related" (i.e. even config > protected! Generally, yes. But it's beyond the scope of this bug report. There are many problems with the package manager assuming that files are no longer needed because they are placed in directories owned by a package, and making the wrong assumption would certainly cause unwanted data loss. Index: geoip-1.6.0-r3.ebuild =================================================================== RCS file: /var/cvsroot/gentoo-x86/dev-libs/geoip/geoip-1.6.0-r3.ebuild,v retrieving revision 1.1 retrieving revision 1.2 diff -u -B -r1.1 -r1.2 --- geoip-1.6.0-r3.ebuild 14 Jun 2014 01:27:30 -0000 1.1 +++ geoip-1.6.0-r3.ebuild 14 Jun 2014 21:27:10 -0000 1.2 @@ -1,6 +1,6 @@ # Copyright 1999-2014 Gentoo Foundation # Distributed under the terms of the GNU General Public License v2 -# $Header: /var/cvsroot/gentoo-x86/dev-libs/geoip/geoip-1.6.0-r3.ebuild,v 1.1 2014/06/14 01:27:30 jer Exp $ +# $Header: /var/cvsroot/gentoo-x86/dev-libs/geoip/geoip-1.6.0-r3.ebuild,v 1.2 2014/06/14 21:27:10 jer Exp $ EAPI=5 inherit autotools eutils @@ -47,3 +47,7 @@ pkg_postinst() { "${ROOT}"/usr/sbin/geoipupdate.sh --force } + +pkg_prerm() { + [[ -z "${REPLACED_BY_VERSION}" ]] && rm -f "${ROOT}"/usr/share/GeoIP/*.dat +}
