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

[Panu Matilainen]

On 09/21/2017 05:55 PM, Mark Wielaard wrote:

On Thu, 2017-09-21 at 10:41 +0300, Panu Matilainen wrote:

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.


OK. Patch split in two and added an early prototype instead of moving
the function around.


Thanks, much much nicer this way :)




@@ -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.


I am surprised it is a GNU extension. IMHO it should be standard C. I
like it, because it is more concise and precise than duplicating the
condition. But OK. Changed to the long form.



I too had to check whether it actually is part of C99 before 
complaining, but AFAICS not.

https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/Conditionals.html#Conditionals

I find the construct entirely alien and incomprehensible. Matter of 
(not) getting used to of course, but since it's an extension...





[...]

+   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 :)


Makes sense. Changed the function name (and argument).


Revised patches applied, thanks!

- Panu -



Thanks,

Mark



___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

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

[Mark Wielaard]

Debuginfo packages are useful without debugsource files. But it is often
useful to also have the debugsource 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).

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.

Signed-off-by: Mark Wielaard 
---
 build/files.c | 33 +++--
 tests/rpmbuild.at | 46 +-
 2 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/build/files.c b/build/files.c
index aad9c5e..92f982e 100644
--- a/build/files.c
+++ b/build/files.c
@@ -2773,6 +2773,9 @@ static void patchDebugPackageString(Package dbg, rpmTag 
tag, Package pkg, Packag
 _free(newsubst);
 }
 
+/* Early prototype for use in filterDebuginfoPackage. */
+static void addPackageDeps(Package from, Package to, enum rpmTag_e tag);
+
 /* create a new debuginfo subpackage for package pkg from the
  * main debuginfo package */
 static Package cloneDebuginfoPackage(rpmSpec spec, Package pkg, Package 
maindbg)
@@ -2805,7 +2808,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 +2918,9 @@ 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).  */
+   addPackageDeps(dbg, dbgsrc ? dbgsrc : maindbg,
+  RPMTAG_RECOMMENDNAME);
}
 }
 }
@@ -2976,6 +2983,16 @@ static int addDebugSrc(Package pkg, char *buildroot)
 return ret;
 }
 
+/* find the debugsource package, if it has been created.
+ * We do this simply by searching for a package with the right name. */
+static Package findDebugsourcePackage(rpmSpec spec)
+{
+Package pkg = NULL;
+if (lookupPackage(spec, "debugsource", PART_SUBNAME|PART_QUIET, ))
+   return NULL;
+return pkg && pkg->fileList ? pkg : NULL;
+}
+
 /* find the main debuginfo package. We do this simply by
  * searching for a package with the right name. */
 static Package findDebuginfoPackage(rpmSpec spec)
@@ -3009,6 +3026,9 @@ rpmRC processBinaryFiles(rpmSpec spec, rpmBuildPkgFlags 
pkgFlags,
 char *uniquearch = NULL;
 Package maindbg = NULL;/* the (existing) main debuginfo 
package */
 Package deplink = NULL;/* create requires to this package */
+/* The debugsource package, if it exists, that the debuginfo package(s)
+   should Recommend.  */
+Package dbgsrcpkg = findDebugsourcePackage(spec);
 
 #if HAVE_LIBDW
 elf_version (EV_CURRENT);
@@ -3039,6 +3059,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)
+   addPackageDeps(dbgpkg, dbgsrcpkg, RPMTAG_RECOMMENDNAME);
 }
 
 for (pkg = spec->packages; pkg != NULL; pkg = pkg->next) {
@@ -3056,6 +3082,8 @@ rpmRC processBinaryFiles(rpmSpec spec, rpmBuildPkgFlags 
pkgFlags,
deplink = extradbg;
if (addDebugSrc(extradbg, buildroot))
deplink = extradbg;
+   if (dbgsrcpkg != NULL)
+   addPackageDeps(extradbg, dbgsrcpkg, RPMTAG_RECOMMENDNAME);
maindbg = NULL; /* all normal packages processed */
}
 
@@ -3072,7 +3100,8 @@ 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)
addPackageDeps(pkg, deplink, 

[Rpm-maint] [PATCH 1/2] Rename addPackageRequires to addPackageDeps in build/files.

[Mark Wielaard]

Also add the dependency tag as argument so it can be used to add either
a Requires or Recommends. Adjust the one caller to pass RPMTAG_REQUIRENAME.

Signed-off-by: Mark Wielaard 
---
 build/files.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/build/files.c b/build/files.c
index 5e84532..aad9c5e 100644
--- a/build/files.c
+++ b/build/files.c
@@ -2986,15 +2986,16 @@ static Package findDebuginfoPackage(rpmSpec spec)
 return pkg && pkg->fileList ? pkg : NULL;
 }
 
-/* add a requires for package "to" into package "from". */
-static void addPackageRequires(Package from, Package to)
+/* add a dependency (e.g. RPMTAG_REQUIRENAME or RPMTAG_RECOMMENDNAME)
+   for package "to" into package "from". */
+static void addPackageDeps(Package from, Package to, enum rpmTag_e tag)
 {
 const char *name;
 char *evr, *isaprov;
 name = headerGetString(to->header, RPMTAG_NAME);
 evr = headerGetAsString(to->header, RPMTAG_EVR);
 isaprov = rpmExpand(name, "%{?_isa}", NULL);
-addReqProv(from, RPMTAG_REQUIRENAME, isaprov, evr, RPMSENSE_EQUAL, 0);
+addReqProv(from, tag, isaprov, evr, RPMSENSE_EQUAL, 0);
 free(isaprov);
 free(evr);
 }
@@ -3073,7 +3074,7 @@ rpmRC processBinaryFiles(rpmSpec spec, rpmBuildPkgFlags 
pkgFlags,
if (maindbg)
filterDebuginfoPackage(spec, pkg, maindbg, buildroot, uniquearch);
else if (deplink && pkg != deplink)
-   addPackageRequires(pkg, deplink);
+   addPackageDeps(pkg, deplink, RPMTAG_REQUIRENAME);
 
 if ((rc = rpmfcGenerateDepends(spec, pkg)) != RPMRC_OK)
goto exit;
-- 
1.8.3.1

___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

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

[Mark Wielaard]

On Thu, 2017-09-21 at 10:41 +0300, Panu Matilainen wrote:
> 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.

OK. Patch split in two and added an early prototype instead of moving
the function around.

> > @@ -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.

I am surprised it is a GNU extension. IMHO it should be standard C. I
like it, because it is more concise and precise than duplicating the
condition. But OK. Changed to the long form.

> [...]
> > +   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 :)

Makes sense. Changed the function name (and argument).

Thanks,

Mark
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [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 
---
  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)
+