[Rpm-maint] [PATCH] Add debugsource recommends to debuginfo packages.

[Panu Matilainen]

On 09/18/2017 06:56 PM, Mark Wielaard wrote:
> Debuginfo packages are useful without debugsource files. But it is often
> useful to also have the debugsourc files. So make debuginfo packages that
> don't contain sources recommend the debugsource package (or the main
> debuginfo one if the sources are not in a separate debugsource package).
> 
> Rename addPackageRequires to addPackageRequiresRecommends with an argument
> to indicate whether the package is required or recommended. Move up so it
> can be used with filterDebuginfoPackage. Add Package dbgsrc as argument to
> filterDebuginfoPackage so it can be added as recommendation. Add a new
> function findDebugsourcePackage. Use it to add a requires to the main
> debuginfo file and/or the debuginfo subpackages.
> 
> Extend the various rpmbuild.at tests that create debugsource and/or
> debuginfo subpackages to check the debugsource (or main debuginfo)
> package is recommended.

No objections to the functionality, but some style nits + suggestions 
below...

> 
> Signed-off-by: Mark Wielaard <mark at klomp.org>
> ---
>   build/files.c     | 59 ++++++++++++++++++++++++++++++++++++++++---------------
>   tests/rpmbuild.at | 46 ++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 88 insertions(+), 17 deletions(-)
> 
> diff --git a/build/files.c b/build/files.c
> index 5e84532..86ec80a 100644
> --- a/build/files.c
> +++ b/build/files.c
> @@ -2773,6 +2773,21 @@ static void patchDebugPackageString(Package dbg, rpmTag tag, Package pkg, Packag
>       _free(newsubst);
>   }
>   
> +/* add a requires or recommends for package "to" into package "from". */
> +static void addPackageRequiresRecommends(Package from, Package to,
> +                                         bool recommends)
> +{
> +    const char *name;
> +    char *evr, *isaprov;
> +    enum rpmTag_e tag = recommends ? RPMTAG_RECOMMENDNAME : RPMTAG_REQUIRENAME;
> +    name = headerGetString(to->header, RPMTAG_NAME);
> +    evr = headerGetAsString(to->header, RPMTAG_EVR);
> +    isaprov = rpmExpand(name, "%{?_isa}", NULL);
> +    addReqProv(from, tag, isaprov, evr, RPMSENSE_EQUAL, 0);
> +    free(isaprov);
> +    free(evr);
> +}

Moving an entire function while also changing it is a bit of a no-no, 
because it's makes it hard to see what actually changed. Moving things 
around also obfuscates git history, which is why I prefer adding a 
prototype to the top of the file instead. But if you need to move stuff 
around (such as different file entirely), do so in a separate commit 
that doesn't change anything and also state this in the commit message.

Also please split refactoring changes like this to separate commits:
one commit that does the necessary enhancements/changes to 
addPackageRequires() and updates existing callers, and another commit to 
add the new functionality. It just makes things the diffs so much more 
obvious and nicer to review.


> +
>   /* create a new debuginfo subpackage for package pkg from the
>    * main debuginfo package */
>   static Package cloneDebuginfoPackage(rpmSpec spec, Package pkg, Package maindbg)
> @@ -2805,7 +2820,8 @@ static Package cloneDebuginfoPackage(rpmSpec spec, Package pkg, Package maindbg)
>   /* collect the debug files for package pkg and put them into
>    * a (possibly new) debuginfo subpackage */
>   static void filterDebuginfoPackage(rpmSpec spec, Package pkg,
> -		Package maindbg, char *buildroot, char *uniquearch)
> +				   Package maindbg, Package dbgsrc,
> +				   char *buildroot, char *uniquearch)
>   {
>       rpmfi fi;
>       ARGV_t files = NULL;
> @@ -2914,6 +2930,8 @@ static void filterDebuginfoPackage(rpmSpec spec, Package pkg,
>   	else {
>   	    Package dbg = cloneDebuginfoPackage(spec, pkg, maindbg);
>   	    dbg->fileList = files;
> +	    /* Recommend the debugsource package (or the main debuginfo).  */
> +	    addPackageRequiresRecommends(dbg, dbgsrc ?: maindbg, true);

Omitting middle operand of conditional expressions is a gcc extension, 
those are best avoided in general and especially when there's no actual 
need to use one.

[...]

> @@ -3038,6 +3056,12 @@ rpmRC processBinaryFiles(rpmSpec spec, rpmBuildPkgFlags pkgFlags,
>   	    if (rpmExpandNumeric("%{?_unique_debug_names}"))
>   		uniquearch = rpmExpand("-%{VERSION}-%{RELEASE}.%{_arch}", NULL);
>   	}
> +    } else if (dbgsrcpkg != NULL) {
> +	/* We have a debugsource package, but no debuginfo subpackages.
> +	   The main debuginfo package should recommend the debugsource one. */
> +	Package dbgpkg = findDebuginfoPackage(spec);
> +	if (dbgpkg)
> +	    addPackageRequiresRecommends(dbgpkg, dbgsrcpkg, true);
>       }
>   
>       for (pkg = spec->packages; pkg != NULL; pkg = pkg->next) {
> @@ -3055,6 +3079,8 @@ rpmRC processBinaryFiles(rpmSpec spec, rpmBuildPkgFlags pkgFlags,
>   		deplink = extradbg;
>   	    if (addDebugSrc(extradbg, buildroot))
>   		deplink = extradbg;
> +	    if (dbgsrcpkg != NULL)
> +		addPackageRequiresRecommends(extradbg, dbgsrcpkg, true);
>   	    maindbg = NULL;	/* all normal packages processed */
>   	}
>   
> @@ -3071,9 +3097,10 @@ rpmRC processBinaryFiles(rpmSpec spec, rpmBuildPkgFlags pkgFlags,
>   	    goto exit;
>   
>   	if (maindbg)
> -	    filterDebuginfoPackage(spec, pkg, maindbg, buildroot, uniquearch);
> +	    filterDebuginfoPackage(spec, pkg, maindbg, dbgsrcpkg,
> +				   buildroot, uniquearch);
>   	else if (deplink && pkg != deplink)
> -	    addPackageRequires(pkg, deplink);
> +	    addPackageRequiresRecommends(pkg, deplink, false);

Hmm, I think this would be both more obvious and generally useful if you 
just call the function addPackageDeps() and pass the relevant dependency 
tag instead of true/false for require/recommend, eg

addPackageDeps(pkg, deplink, RPMTAG_REQUIRENAME);
addPackageDeps(extradbg, dbgsrcpkg, RPMTAG_RECOMMENDNAME);

vs

addPackageRequiresRecommends(pkg, deplink, false);
addPackageRequiresRecommends(extradbg, dbgsrcpkg, true);

That way we wont end up with 
addPackageRequiresRecommendsConflictsObsoltetes() one day :)

	- Panu -