On Tue, Jun 08, 2021 at 02:50:47PM +0800, clay stan wrote:
> Tobias Frost <t...@debian.org> 于2021年6月3日周四 上午1:33写道：
> >
> > Control: tags -1 moreinfo
> >
> > Hi Clay,
> >
> > here's a review:
> > - The patch: The dep3 header, the field Bug-Debian is wrong, the ITP is not
> > related to the patch
>
> Thanks, I've updated.
>
> > - The patch looks strange to me: Why do you patch the Makefile? What do you
> > want to archieve? Parts of the patching seems ok (like avoiding stomping
> > over
> > CFLAGS, but other parts seems excessive, removing sane parts to me…
>
> The original compilation parameters will also cause Lintian to report errors,
> hardening no bind now, hardening no mandatory functions.
> I'll try to solve them in debian/rules by
> https://wiki.debian.org/Hardening, but it doesn't work
>
>and the sane parts, you mean?
You've basically completly rewritten a (mostly) working Makefile
With your comment I assume that you just wanted to have hardening,
so _just_ those parts should have been patched.
And that would be:
--- a/Makefile
+++ b/Makefile
@@ -1,6 +1,6 @@
CC ?= gcc
-CFLAGS+=-Wall -Wextra -pedantic -fstack-protector-all -pedantic -std=c99
+CFLAGS:=-Wall -Wextra -pedantic -fstack-protector-all -pedantic -std=c99
${CFLAGS} ${CPPFLAGS} ${LDFLAGS}
SANITY_FLAGS=-Wfloat-equal -Wshadow -Wpointer-arith
PREFIX ?= /usr
(+ adding export DEB_BUILD_MAINT_OPTIONS = hardening=+all to d/rules)
So, see it as recommendation: I'd keep patches really minimal. Or they
will create you a lot of work…
It is a burden to carry such an intrusive patch, especially if upstream
explictly expressed that he does not generally welcome patches
if they target the Makefile.
Just one example where I think the upstream part is better:
-PREFIX ?= /usr
+PREFIX = /usr
Some other stuff could be done with debhelper overrides, so
that the patch can get smaller…
But you do you… I just strongly recommend to revisit the topic.
On top, non-upstreamable patches are bad…
> > - Upstream seems to support arm, you patch that out?
>
> Upstream support arm means the mobile arm chips, like Snapdragon, MediaTek,
> not arm PC, They are not supported by Debian.
This is not a strong reason to disable arm support entirely…
(A quick internet search suggests that there is some support for Debian on e.g
Snapdragon. And there seems to be derivates; even if not, maybe someone wants
to enhance cpufetch on the basis of the Debian package.)
> > - There is no LDCFLAGS -> did you mean LDFLAGS?
>
> Yeah, I've updated. Thanks again.
>
> >
> > - (not a blocker) Please send the manpage upstream for inclusion.
Regarding the manpage: I've diffed the manpage (cpufetch.1 and
debian/cpufetch.1). They are almost identical. With just small fixes done by
you. However, you claim (sole) copyright on it. That is not appropiate.
I would patch the manpage, if it needs fixing: The header of the upstream ones
says that upstream builds it using help2man (but hav not integrated that into
their Makefile), so possibly this is another route to go: Rebuild at build
time.
--
tobi
