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-discuss@lists.alioth.debian.org
> + -- David Michael Smith <sidicas2@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 <sidicas2@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
Attachment:
signature.asc
Description: OpenPGP digital signature