Bug#702588: RFS: rrep/1.3.5-1
Thank you very much for the detailed review. However, some of your
comments are not entirely clear to me, so I have a couple of questions.
I am preparing a new upstream version.
On 03/11/2013 07:54 AM, Paul Wise wrote:
> In future, you might want to X-Debbugs-CC your earlier sponsors on the
> RFS bug submission.
I will do that.
> I would encourage you to improve the watch file:
>
> http://wiki.debian.org/debian/watch#Common_mistakes
I improved the watch file so that more extensions are found:
http://sf.net/rrep/rrep-(.+)\.(?:zip|tgz|tbz|txz|(?:tar\.(?:gz|bz2|xz)))
Were you referring to that?
> I personally prefer --with autoreconf (using dh-autoreconf) to --with
> autotools_dev. This ensures that when Debian QA folks do their usual
> regular archive rebuilds, the build system gets checked for
> rebuildability too.
Done.
> Please add --parallel to your invocation of dh.
Done.
> I would suggest that the upstream build system should install
> doc/rrep.info, rather than dh_installinfo.
Why would you suggest that? The dh_installinfo way seems to be the
standard way that is described in the Maintainers Guide. I would prefer
to stick to that.
> It doesn't appear to be nessecary to distribute the upstream README in
> the binary package. The package description covers the second part and
> the rest isn't particular.y useful.
README will not be distributed anymore.
> The copyright/license info appears to be insufficient. The README is
> not GPL-3+ and various files are copyright by the FSF. Various *.m4
> files are also not GPL.
I added copyright/license texts for the FSF files to debian/copyright.
> Why do you have ${shlibs:Depends} in Pre-Depends instead of Depends?
> That looks like a mistake to me.
That was a mistake. Corrected.
> You don't appear to have any translations but you do have i18n, I
> would suggest registering rrep with transifex to get in contact with
> translators:
For now I have boldquot and quot. I registered the project at transifex.
> Please ensure that doc/rrep.info is regenerated at build time.
I added texinfo to Build-Depends. This ensures that makeinfo is
available during build time.
> Please update the debtags information for this package:
Done.
> rrep contains something with a command-line interface, it would be
> nice to create a screenshot or two showing typical usage.
Done.
> configure:
>
> configure: WARNING: libacl development library was not found or not usable.
> configure: WARNING: rrep will be built without ACL support.
I added libacl1-dev to Build-Depends and libacl1 to Suggests. No
warnings anymore.
> gcc:
>
> copy-file.c: In function 'qcopy_file_preserving':
> copy-file.c:143:9: warning: ignoring return value of 'chown', declared
> with attribute warn_unused_result [-Wunused-result]
copy-file.c is part of gnulib. I tried the Debian version of gnulib and
I tried the latest upstream gnulib. I get the warning in all cases. I am
reluctant to change any gnulib files.
> blhc:
>
> Lots of this: CFLAGS missing (-fPIE) ....
> One of this: LDFLAGS missing (-fPIE -pie -Wl,-z,now) ....
I added
export DEB_BUILD_MAINT_OPTIONS = hardening=+all
to rules. No flags missing anymore.
> cppcheck:
>
> [lib/rpmatch.c:113]: (error) Memory leak: safe_pattern
> [src/rrep.c:573] -> [src/rrep.c:575]: (error) Possible null pointer
> dereference: next_path - otherwise it is redundant to check it against
> null.
> [lib/regcomp.c:2848]: (error) Uninitialized variable: symb_table
> [lib/regcomp.c:2847]: (error) Uninitialized variable: table_size
> [lib/regcomp.c:2850]: (error) Uninitialized variable: table_size
I removed the redundant check in rrep.c. rpmatch.c and regcomp.c are
part of gnulib. See response to copy-file.c.
> msgfmt:
>
> msgfmt: ./po/rrep.pot: warning: source file contains fuzzy translation
rrep.pot is meant as a template for potential translators. Should I not
distribute it?
> lacheck:
>
> lots of warnings on build-aux/texinfo.tex
This is the standard texinfo file. I am reluctant to change anything here.
> clang compiler:
>
> Build failure: http://clang.debian.net/logs/2013-01-28/rrep_1.3.3-2_unstable_clang.log
Sorry, I have no clue about the linker error of the clang compiler. This
should work with any sane compiler.
> ports:
>
> Build failure on x32 port:
> http://buildd.debian-ports.org/status/package.php?p=rrep
Works for rrep >= 1.3.4.
Thanks,
Arno
Reply to: