Hi David, As I am already spending quite some time on giving you feedback, I am considering sponsoring you (my first time). However, as I have never used this package, I need to learn it a little first. And I am not very knowledgeable with C, so figuring out the patches is time-consuming.
On 16-11-12 18:30, David Smith wrote: >>>> Are really all these bugs of the proper severity? I.e. bug 692007 >>>> does not seem to qualify in my view (although *you* marked it as >>>> important later, but when you submitted you did not think it >>>> important). You can try, but I am unsure if this would qualify for >>>> an unblock (maybe in addition to the rest). >>> In my opinion, they are. Originally I was using this software with >>> only a few feeds. When I added more feeds, I realized this was a >>> more serious bug as it then started spamming the user's desktop with >>> notifications on feeds that have already been read. Further, they >>> keep triggering every refresh so it's pretty much endless. I >>> suppose the user could just turn the notifications off for the app, >>> but I thought it better to just fix it. Liferea is a news reader >>> and people use it to get live updates to newstreams via RSS so I >>> personally wouldn't use the package without the patch so I thought >>> it should be "important". >> I understand. It is just that the release-team might disagree. But >> off-course you can try. Next time, (or even now as a comment) add this >> kind of justification to the command where you raise the severity. > > OK. I've gone back and now added justifications for the bugs that I made > important and included the fixes for in this NMU. Reading it back, I still think this bug does not qualify as important. Yes, it's annoying, but it does not "have a major effect on the usability of a package, without rendering it completely unusable to everyone". And why did you remove the forwarded tag? Why did you not forward your patch to the upstream bug-report? I am afraid that this bug will have to wait until after Wheezy. I suggest you reconsider. Just for my testing, how do you set/unset these notifications? And do I understand correctly it ONLY is relevant for google feeds? >> The release team made it clear that changes should be clean, i.e. your >> changes of getting an unblock are higher if you remove everything that >> is not necessary. > > * Cleaned up the names of the patches so they more clearly describe what > they accomplish. Good. > * Moved the upstream changes of the copyright to the patch header so the > patch is a lot smaller. Good. However, I would appreciate it if you could also add the Bug: and Bug-Debian: tags to the patch headers [0]. If you don't then at the very least mention in the changelog which patch belongs to which bug. You might want to do this anyway. [0] http://dep.debian.net/deps/dep3/ > * I also added a patch for adding the hardened build wrapper. Liferea > has a parser that parses feeds from the Internet so I thought it best to > harden the compile, and this also fixes two lintian warnings. I was > told on IRC that hardened packages is a release goal so they should > automatically be "important" , but I was also told that using the > hardened wrapper to do it is deprecated. It was suggested that using the > other way (build flags) to harden the package may be too invasive as the > package is only compat 7 and has it's own custom build flags.. So I'm > not sure what to do here if simply adding the hardened build wrapper > isn't acceptable. Hmm, I am not sure. Reading [1], I don't see the wrapper being deprecated. But I do see that you have more options in the recommended dpkg-buildflags section than just to switch to dh, i.e. using them directly. I don't have experience with these hardening options, but I think I like the following better: DPKG_EXPORT_BUILDFLAGS = 1 include /usr/share/dpkg/buildflags.mk CFLAGS += -g -O$(if $(findstring noopt,$(DEB_BUILD_OPTIONS)),0,2) LDFLAGS += "-Wl,--as-needed" But maybe other mentors can state their opinion as well. [1] http://wiki.debian.org/Hardening > Going to try my best to get everything through to Wheezy, but if it > doesn't I suppose I can go back and just remove what didn't make it and > try again. If I sponsor your nmu, you should request the unblock. If that is rejected, you can still adjust the package, depending on the reason of course. > Attached is the new NMU diff. Reading it. > Target is Wheezy. Loud and clear. > +++ liferea-1.8.6/debian/changelog 2012-11-11 22:07:08.000000000 +0800 > + * Replaced build-depends on transitional package libwebkit-dev with > + libwebkitgtk-dev. (Closes: #677749) Not an important bug, so I suggest you remove it from your nmu if you target Wheezy as you do. > + * Added hardening-wrapper since liferea has a parser and should be > + built with hardening. (Closes: #692527) See above. And please tag the bug such that it shows up at [2] [2] http://bugs.debian.org/cgi-bin/pkgreport.cgi?tag=goal-hardening;users=hardening-disc...@lists.alioth.debian.org > + -- David Michael Smith <sidic...@gmail.com> Sat, 10 Nov 2012 10:43:14 +0800 Please also update the timestamp. And you have several spaces at the end of lines. Please remove them. > diff -Nru liferea-1.8.6/debian/control liferea-1.8.6/debian/control > - libwebkit-dev (>= 1.2.2), > + libwebkitgtk-dev (>= 1.8.1), Although I suggest to remove it now, why also update the version? This is not documented in the changelog. > diff -Nru liferea-1.8.6/debian/patches/fix-browser-selections > liferea-1.8.6/debian/patches/fix-browser-selections > +Patch by David Smith <sidic...@gmail.com> > +Fixed a bug where web browser doesn't launch due to not > +having gnome desktop installed, or the wrong web browser is > +launched due to not having an appropriate fallback. > +Posted by him to #668197 > +Last-Update 2012-11-07 It would be nice if you use the tags and syntax from deb3 [0]. > diff -Nru liferea-1.8.6/debian/patches/fix-google-reader-notifications > liferea-1.8.6/debian/patches/fix-google-reader-notifications > + if (allowStateChanges) { > ++ if ((!oldItem->readStatus) && > (newItem->readStatus)) > + oldItem->readStatus = > newItem->readStatus; > + oldItem->flagStatus = > newItem->flagStatus; > + } Can you explain how this is supposed to work? And wouldn't you want the same change for the flagStatus as well? Paul btw I think you can close bug 496886
signature.asc
Description: OpenPGP digital signature
