Ask for review
Hi Marcos,
thanks for looking at my review! Let's keep pkg-security-team@ in CC, as
we may get some additional feedback. At the very least it should make
life for the reviewing DD easier later if they are aware of our
discussion.
On Mon, 24 Apr 2017 00:08:43 +0200
Marcos Fouces <mfouces at yahoo.es> wrote:
> > * calling ./bootstrap.sh (in debian/rules) is unnecessary; it only
> > performs what the dh_autoreconf sequence does anyways. This also
> > means you can drop one of the patches?
> I disagree with this. I added it to rules files because build system
> doesn't do it.
Strange, in my pbuilder (sid) environment, removing the
`./bootstrap.sh` line from debian/rules produces byte-identical
contents in the binary packages.
It's true that the build system doesn't call `bootstrap.sh`, but all
that script does is creating the m4 directory and doing
autoconf/automake processing (and the git submodule stuff you removed
with your patch anyways).
> > * you disabled the test suite completely. I think it would be
> > better to keep as much as possible around. After adding procps to
> > the build-depends, four tests fail in my enivornment:
> > - datasource_egid.sh
> > - datasource_egroup.sh
> > - combined-filter-exclude_uid-drop.sh
> > - combined-filter-only_uid-pass.sh
> > The tests probably fail due to pbuilder's build environment (uid
> > mismatch between what snoopy thinks and what's reported by
> > `ps`). If I disable those four (in tests/combined/Makefile.am and
> > tests/datasource/Makefile.am using a patch) the build works.
> I achieved a similar conclusion here: the test fails because of build
> system. So it seems to me that it is safe to disable all of them.
If you think it's better to disable testing completely, I'd suggest you
add a comment in `debian/rules` explaining why you did so. Probably this
should also go into the changelog because (as far as I can tell) the
tests were enabled in the previous release.
> I compiled it with all data sources and it seems to performs well.
What do you mean by "seems to perform well"?
> > * you should probably make a pull request with your spelling fixes
> > on github
> Yes, i also will ask upstream to change the content of its debian
> folder (under contrib/ directory) to this new one.
Good :) .
Regards
Lukas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.alioth.debian.org/pipermail/pkg-security-team/attachments/20170424/42be96e6/attachment.sig>
Reply to: