[Rpm-maint] [PATCH] Allow deprecations to work accross colors (RhBug:713323)

[Panu Matilainen]

On 12/07/2011 05:17 PM, Ales Kozumplik wrote:
> This enables package maintainers to:
> - Force removal of a no longer supported multilib library.
>
> - Deprecate packages of different header color than the package's.

Yup... I suppose this is the best we can do, short of introducing some 
means for packages to control the behavior. But even then packagers 
might not actually control what bits are "multilib'ed" in a distro.

> Note:
>    even x86_64 packages can have header color 1 in which case we are
>    currently left with no means to deprecate them from another x86_64
>    package. (RhBug:751574)

Not in the scope of this patch, but this is actually something we 
probably should at least warn about at build-time. There are always 
going to be special cases (eg grub likely qualifies as such), but 
"normal" cases wrong colored files in packages are likely to be bugs. We 
already have such a check on noarch packages (in processBinaryFiles).

> ---
>   lib/depends.c |   34 ++++++++++++++++++++++------------
>   1 files changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/lib/depends.c b/lib/depends.c
> index 43247f9..207a56e 100644
> --- a/lib/depends.c
> +++ b/lib/depends.c
> @@ -129,8 +129,19 @@ static rpmdbMatchIterator rpmtsPrunedIterator(rpmts ts, rpmDbiTagVal tag,
>       return mi;
>   }
>
> -#define skipColor(_tscolor, _color, _ocolor) \
> -	((_tscolor)&&  (_color)&&  (_ocolor)&&  !((_color)&  (_ocolor)))
> +/**
> + * Decides whether to skip a package upgrade/obsoletion on TE color.
> + *
> + * @param tscolor	color of this transaction
> + * @param color 	color of this transaction element
> + * @param ocolor 	header color of the upgraded/obsoleted package
> + *
> + * @return		non-zero if the package should be skipped
> + */
> +static int skipColor(rpm_color_t tscolor, rpm_color_t color, rpm_color_t ocolor)
> +{
> +    return tscolor&&  color&&  ocolor&&  !(color&  ocolor);
> +}

Please split the skipColor() part to a separate commit as this isn't 
related to the obsoletes behavior (in this version of the patch at least)

>
>   /* Add erase elements for older packages of same color (if any). */
>   static void addUpgradeErasures(rpmts ts, tsMembers tsmem, rpm_color_t tscolor,
> @@ -167,17 +178,18 @@ static void addObsoleteErasures(rpmts ts, tsMembers tsmem, rpm_color_t tscolor,
>   	if ((Name = rpmdsN(obsoletes)) == NULL)
>   	    continue;	/* XXX can't happen */
>
> -	/* XXX avoid self-obsoleting packages. */
> -	if (rstreq(rpmteN(p), Name))
> -	    continue;
> -
>   	mi = rpmtsPrunedIterator(ts, RPMDBI_NAME, Name, 1);
>
>   	while((oh = rpmdbNextIterator(mi)) != NULL) {
> -	    /* Ignore colored packages not in our rainbow. */
> -	    if (skipColor(tscolor, hcolor,
> -			  headerGetNumber(oh, RPMTAG_HEADERCOLOR)))
> +	    char * ohNEVRA = headerGetAsString(oh, RPMTAG_NEVRA);
> +	    const char *oarch = headerGetString(oh, RPMTAG_ARCH);
> +
> +	    /* avoid self-obsoleting packages */
> +	    if (rstreq(rpmteN(p), Name)&&  rstreq(rpmteA(p), oarch)) {
> +		rpmlog(RPMLOG_DEBUG, "  Not obsoleting: %s\n", ohNEVRA);
> +		free(ohNEVRA);
>   		continue;
> +	    }
>
>   	    /*
>   	     * Rpm prior to 3.0.3 does not have versioned obsoletes.
> @@ -185,13 +197,11 @@ static void addObsoleteErasures(rpmts ts, tsMembers tsmem, rpm_color_t tscolor,
>   	     */
>   	    if (rpmdsEVR(obsoletes) == NULL
>                   || rpmdsAnyMatchesDep(oh, obsoletes, _rpmds_nopromote)) {
> -		char * ohNEVRA = headerGetAsString(oh, RPMTAG_NEVRA);
>   		rpmlog(RPMLOG_DEBUG, "  Obsoletes: %s\t\terases %s\n",
>   			rpmdsDNEVR(obsoletes)+2, ohNEVRA);
> -		free(ohNEVRA);
> -
>   		removePackage(ts, oh, p);
>   	    }
> +	    free(ohNEVRA);
>   	}
>   	rpmdbFreeIterator(mi);
>       }

I'd keep the ohNEVRA local to the scopes where used for the debug output 
(it'll only get called once for any header anyway), partly because I 
prefer strict symmetry on alloc/free, partly because now it'll grab the 
nevra for every package we go through, whether used or not (and just for 
a debug output that's in 99% cases hidden anyway. It's unlikely to cause 
any measurable performance hit in this case, but some other dependency 
checking related place was wasting very measurable percentage of runtime 
generating debugging output nobody would normally see. So grumbling 
mostly just on principle here :)

Other than that, ACK.

Not related to this patch, but while on the subject... the use of 
rpmdsAnyMatchesDep() seems dubious here, as that matches against 
provides whereas obsoletes are on package names. It shouldn't make a 
difference on functionality as we're iterating based on the package 
name, but using rpmdsNVRMatchesDep() would make it clearer. Plus it's 
somewhat cheaper as well since there's just one value to compare, 
whereas constructing provides dependency set and comparing is quite a 
bit more work.

	- Panu -