Tobias Frost <t...@debian.org> 于2021年6月9日周三 上午2:03写道： > > 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…
Take your advice, now patch only changed one line. > > 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.) Add arm64 now. > > > > - 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. Previously, because there has a problem with the upstream manpage, So I used help2man to regenerate a manpage then modify it, so I claim copyright on it. But now I have contacted the upstream to repaired his manpage and remove my manpage. And I rebuild the package with a new version and upload to mentors: https://mentors.debian.net/package/cpufetch You can download the package with dget using this command: dget -x https://mentors.debian.net/debian/pool/main/c/cpufetch/cpufetch_0.98-1.dsc Thanks again. > > 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
