Application Review Request: news 0.4.5

[Emmanuel Thomas-Maurin]

Application Review For news 0.4.5

== About You ==

 * Emmanuel Thomas-Maurin
 * <email address hidden>

The Application:

 * News - GTK-based highly graphically-configurable RSS Ticker
 * GPL
 * https://launchpad.net/~manu-tm/+archive/news-rss-ticker
 * http://www.newsrssticker.com/contact.php
 * KNOWN ISSUES:

[Allison Randal (allison)]

Thanks for the submission, I'll review it today and give you feedback.

[Allison Randal (allison)]

General review: The idea of a small RSS ticker is good, and appropriate for the ARB process. The app installed and removed cleanly, with all dependencies met, and it integrated with the Applications menu. The content is suitable under the Ubuntu Code of Conduct, and the package includes the needed copyright and licensing information.

On the behavior of the app:

1) When I first ran the application, clicking on scrolling ticker text gave me the error message:
---
News: Can't create process - Failed to execute child process "" (No such file or directory)
---
After I set the "'Open in Browser' command line" option to "firefox", it started to work. Can you set browser option to a sensible default value so the user doesn't get the obscure error message? Or at least catch the error and give the user a more meaningful error message (like "Please set the 'Open in Browser' option in Preferences").

2) When I run "File > Open RSS Feed" for the first time I get the following error message:
---
Can't load URL list: /home/allison/.news/news_url_list No such file or directory
--
After I open one URL, it saves it in that file and the error disappears. Can you catch that error? It seems to be an expected error the first time a user runs the app.

3) I tweaked down the font size, switched to a transparent background, hid the clock, and repositioned the ticker to the bottom of my screen. These features performed well. Overall, it seems like a useful app for unobtrusively keeping up to date on RSS feeds.

On the packaging:

1) The ARB process requires your app to be installed in the directory /opt/extras.ubuntu.com/news. That is, you need to install the 'news' binary in /opt/extras.ubuntu.com/news/bin and the libetm library in /opt/extras.ubuntu.com/news/lib. It looks like you support a --prefix= option in your configure scripts, so using that option in the debian/rules file will be the main change needed. You'll also need to modify the Exec= option in the news.desktop file with the full path /opt/extras.ubuntu.com/news/bin/news.

2) I suggest modifying the name of the app in the news.desktop file to "News RSS Ticker", to make it clearer in the Applications menu what the app does.

3) ARB apps aren't allowed to run cron jobs. Is the cron job setup in debian/cron.d.ex really necessary? To pass the ARB process, you'll need to remove the cron setup from the package (or make a custom version of the package without it).

4) ARB apps aren't allowed to use custom maintainer scripts like your preinst.ex, postinst.ex, prerm.ex, and postrm.ex. It doesn't look like you're actually using these files at all (they're just shells with no functionality), can you simply remove them from the package?

5) Apps for the ARB process use custom version numbers, so the first line in your debian/changelog file should be:
---
news (0.4.5-0extras10.10.1) maverick; urgency=low
---

6) Your debian/README.source is an unmodified template dump. You can simply delete the file from the package.

7) Your watch.ex file appears to be an unmodified template dump. You should probably replace it with something like:
---
version=3

http://sites.google.com/site/linuxnewsticker/home/news_(.*)\.tar\.g...

[Emmanuel Thomas-Maurin (manu-tm)]

Thank you for the feedback. I've almost completed the changes but I'm not sure of which version number I should use now. Maybe 0.4.6-beta-0extras10.10.1 ? I mean, is there some precise rules I must follow?

[Allison Randal (allison)]

There are a few variations, depending on what you're looking for. It basically boils down to the simple rule that every new version of the package you put up in your PPA has to be higher than the previous one.

The final version that we put into the Software Center will be versioned something like 0.4.6-0extras10.10.1, but I'm guessing you're looking for a way to build a few test packages in your PPA before you package the final 0.4.6 release? In that case, I suggest:

0.4.6~beta1-0extras10.10.1

The tilde ('~') is handled specially in the version, so this will count as a lower version than any other 0.4.6 version.

You can read more about version numbers here: https://help.launchpad.net/Packaging/PPA/BuildingASourcePackage

And see an example of another project that uses this naming strategy here: https://launchpad.net/ubuntu/+source/bzr/2.3.0~beta3-1build1

Notice in that example, the .orig.tar.gz also includes the ~beta3. That's important to allow you to upload testing tarballs before the final 0.4.6 tarball.

[Emmanuel Thomas-Maurin (manu-tm)]

Thanks for all the info. I've just uploaded version 0.4.6~beta1-0extras10.10.1.

[Emmanuel Thomas-Maurin (manu-tm)]

Actually it's v 0.4.6~beta2-0extras10.10.1.

[Allison Randal (allison)]

Excellent, thanks. I'm taking a look now.

[Allison Randal (allison)]

Looks great, you made all the requested fixes. I really like the new default look, it's very eye-catching. The sample RSS feeds for first-time users are good too.

A couple little fixes remaining:

1) The doc files (README and others) are still installing in /usr/share/doc/news instead of /opt/extras.ubuntu.com/news/doc/. You should be able to change this with another line in the debian/install file, if it's not configurable elsewhere.

2) The debian/watch file still needs a little work. You can test it by running 'uscan' on the unpacked tarball:

 $ uscan --report news-0.4.6/

This gives you the warning:

uscan warning: In watchfile debian/watch, reading webpage
  http://www.newsrssticker.com/bin-packages failed: 403 Forbidden
uscan warning: In watchfile debian/watch, reading webpage
  http://www.newsrssticker.com/src failed: 403 Forbidden
uscan warning: In watchfile debian/watch, reading webpage
  http://sites.google.com/site/linuxnewsticker/home/news failed: 404 Not Found

(For even more detailed error messages use the command: uscan --report-status --debug news-0.4.6/)

Testing the watch file with uscan will let you tweak it until the URL patterns match your tarballs. The third URL works as:

http://sites.google.com/site/linuxnewsticker/home /site/linuxnewsticker/home/news-([^-]+)\.tar\.gz(?:.*)

(That looks a little redundant, it's because uscan is loading the HTML page at the first part 'http://www.newsrssticker.com/src' and searching that page for an '<a href=...>' tag that matches the second part '(.*)\.tar\.gz'.)

You can read more about the format of debian/watch file entries at:

http://wiki.debian.org/debian/watch/

BTW, in src/news.h, the path is listed in a comment as /opt/extras.ubunutu.com... (with an extra 'u'). The actual INSTALL_DIR is right, so it installs fine, I just happened to notice the comment while I was doing the source code review (and the rest looked good).

The Application Review Board will meet again next Tuesday. This app is close enough that I expect it'll be ready to vote on (and hopefully approve) then.

[Emmanuel Thomas-Maurin (manu-tm)]

I modified debian/install like that:

news.desktop /usr/share/applications
news_url_list /opt/extras.ubuntu.com/news/share
README /opt/extras.ubuntu.com/news/share/doc
TODO /opt/extras.ubuntu.com/news/share/doc
ChangeLog /opt/extras.ubuntu.com/news/share/doc
debian/copyright /opt/extras.ubuntu.com/news/share/doc

So the doc files install now in: /opt/extras.ubuntu.com/news/share/doc

But it seems (when looking at the resulting binary deb) that they still
install in /usr/share/doc as well and I can't find how to fix this issue.

On 01/26/2011 03:43 AM, Allison Randal wrote:

> Looks great, you made all the requested fixes. I really like the new
> default look, it's very eye-catching. The sample RSS feeds for first-
> time users are good too.
>
> A couple little fixes remaining:
>
> 1) The doc files (README and others) are still installing in
> /usr/share/doc/news instead of /opt/extras.ubuntu.com/news/doc/. You
> should be able to change this with another line in the debian/install
> file, if it's not configurable elsewhere.
>
> 2) The debian/watch file still needs a little work. You can test it by
> running 'uscan' on the unpacked tarball:
>
> $ uscan --report news-0.4.6/
>
> This gives you the warning:
>
> uscan warning: In watchfile debian/watch, reading webpage
> http://www.newsrssticker.com/bin-packages failed: 403 Forbidden
> uscan warning: In watchfile debian/watch, reading webpage
> http://www.newsrssticker.com/src failed: 403 Forbidden
> uscan warning: In watchfile debian/watch, reading webpage
> http://sites.google.com/site/linuxnewsticker/home/news failed: 404 Not Found
>
> (For even more detailed error messages use the command: uscan --report-
> status --debug news-0.4.6/)
>
> Testing the watch file with uscan will let you tweak it until the URL
> patterns match your tarballs. The third URL works as:
>
> http://sites.google.com/site/linuxnewsticker/home
> /site/linuxnewsticker/home/news-([^-]+)\.tar\.gz(?:.*)
>
> (That looks a little redundant, it's because uscan is loading the HTML
> page at the first part 'http://www.newsrssticker.com/src' and searching
> that page for an '<a href=...>' tag that matches the second part
> '(.*)\.tar\.gz'.)
>
> You can read more about the format of debian/watch file entries at:
>
> http://wiki.debian.org/debian/watch/
>
> BTW, in src/news.h, the path is listed in a comment as
> /opt/extras.ubunutu.com... (with an extra 'u'). The actual INSTALL_DIR
> is right, so it installs fine, I just happened to notice the comment
> while I was doing the source code review (and the rest looked good).
>
>
> The Application Review Board will meet again next Tuesday. This app is close enough that I expect it'll be ready to vote on (and hopefully approve) then.
>

--
Emmanuel Thomas-Maurin <email address hidden>

[Emmanuel Thomas-Maurin (manu-tm)]

More precisely, after removing all entries in debian/docs, I still have:

/usr/share/doc/news/changelog.Debian.gz
/usr/share/doc/news/changelog.gz
/usr/share/doc/news/copyright

[Allison Randal (allison)]

You'll also need to disable the standard documentation install. In the debian/rules file add the following lines:

override_dh_installdocs:

override_dh_installchangelogs:

(These tell debian/rules to "do nothing" instead of the standard actions for installing docs and changelogs.) There will be a nicer way of doing this in Natty, but this works for now.

[Emmanuel Thomas-Maurin (manu-tm)]

Thanks, it solved the problem.

So, I've just uploaded version beta3 as it seems ok.

On 01/27/2011 12:11 AM, Allison Randal wrote:

> You'll also need to disable the standard documentation install. In the
> debian/rules file add the following lines:
>
> override_dh_installdocs:
>
> override_dh_installchangelogs:
>
> (These tell debian/rules to "do nothing" instead of the standard actions
> for installing docs and changelogs.) There will be a nicer way of doing
> this in Natty, but this works for now.
>

--
Emmanuel Thomas-Maurin <email address hidden>

[Allison Randal (allison)]

This looks great! I'll submit it for a vote in the meeting tomorrow.

[Emmanuel Thomas-Maurin (manu-tm)]

Thank you Allison.

[Allison Randal (allison)]

The app was approved! The next step will be creating a final version of the package and getting it into the system for distribution. This will involve releasing a version of the package with the official numbering scheme (0.4.6-0extras10.10.1), and setting up a screenshot and thumbnail image for the package (see https://wiki.ubuntu.com/PostReleaseApps/Metadata for more details). Let me know if you have any questions or need any help (especially with the images, that process isn't as straightforward as it needs to be yet).

[Emmanuel Thomas-Maurin (manu-tm)]

Great! :) That's very good news! I'm gonna look at the next step now
(I'll probably have questions soon...)

[Emmanuel Thomas-Maurin (manu-tm)]

About uploading screenshot and thumbnail to:
lp:~commercial-ppa-uploaders/software-center/additional-screenshots? I
know I need to create a bzr branch but I'm not sure how to do this the
right way...

[Emmanuel Thomas-Maurin (manu-tm)]

I've modified debian/control and debian/rules:

***debian/control***

Source: news
Section: net
Priority: extra
Maintainer: Emmanuel Thomas-Maurin <email address hidden>
Build-Depends: debhelper (>= 7), autotools-dev, libgtk2.0-dev, libxml2-dev
Standards-Version: 3.8.3
Homepage: http://www.newsrssticker.com

Package: news
Architecture: any
XB-AppName: News RSS Ticker
XB-Icon: news_icon.xpm
XB-Screenshot-Url: https://software-center.ubuntu.com/screenshots/n/news-lucid.png
XB-Thumbnail-Url: https://software-center.ubuntu.com/screenshots/n/news-lucid.thumb.png
XB-Category: GNOME;GTK;Network;News
Depends: ${shlibs:Depends}, ${misc:Depends}, libgtk2.0-0, libxml2
Description: GTK-based highly graphically-customizable RSS Ticker
 News is a GTK-based RSS Ticker that displays feeds as a smooth
 scrolling line on your Desktop, as known from TV stations.
 - Open feed links in your favourite Browser.
 - Graphics are highly customizable.

***debian/rules***

#!/usr/bin/make -f
# -*- makefile -*-
# Sample debian/rules that uses debhelper.
# This file was originally written by Joey Hess and Craig Small.
# As a special exception, when this file is copied by dh-make into a
# dh-make output file, you may use that output file without restriction.
# This special exception was added by Craig Small in version 0.37 of dh-make.

# Uncomment this to turn on verbose mode.
export DH_VERBOSE=1

%:
 dh $@

override_dh_installdocs:
override_dh_installchangelogs:

common-install-indep::
 cp images/news_icon.xpm ..
 dpkg-distaddfile news_icon.xpm raw-meta-data -

So I could upload to Launchpad as soon as I know if the icon / screenshot / thumbnail names and urls are right.
(BTW I still have troubles with the bzr branch.)

[Allison Randal (allison)]

If you change "lucid" to "maverick", the screenshot/thumbnail names will be perfect.

You'll find the one-line command for creating the bzr branch at:

https://code.launchpad.net/~commercial-ppa-uploaders/software-center/additional-screenshots.

Then you'll need to 'bzr add' the files, 'bzr commit', and then 'bzr push' your branch to your launchpad account. You can see an example of another app developer's branch (which was already merged back into the original) here:

https://code.edge.launchpad.net/~stefanor/software-center/suspended-sentence-screenshots

[Emmanuel Thomas-Maurin (manu-tm)]

The screenshots are now at:

https://code.launchpad.net/~manu-tm/+junk/screenshots

I have also uploaded the source package.

Is it ok like that? If yes, do I have something else to do?

[Allison Randal (allison)]

That's perfect, thanks! I'll nudge the right people to merge the branch and launch the source package into extras.

[Emmanuel Thomas-Maurin (manu-tm)]

Nice, and thanks!.

[Emmanuel Thomas-Maurin (manu-tm)]

I still have questions: once the app is in extras, how can it be updated to new versions? Is there a review process each time? Finally, where could I find info about the whole process?
Thanks.

[Emmanuel Thomas-Maurin (manu-tm)]

The branch has just been merged.

[Stéphane Graber (stgraber)]

Yes, we're just waiting on Canonical IS to merge the branch on the web server before uploading the package to extras.
Thanks

[Emmanuel Thomas-Maurin (manu-tm)]

About future versions, how does it work? Where can I find info about that?
Thanks.

[Stéphane Graber (stgraber)]

I'm uploading your current version now as the screenshot is now present on the web server.

For future versions, just open a new bug report and we'll review it as we did for the first version.
It should go slightly faster as the screenshot will already be there and we're already familiar with the code.

[Stéphane Graber (stgraber)]

I'm looking at news_0.4.6-0extras10.10.1 in your PPA.

I see it's only been built for lucid, then copied to maverick. Did you try building it directly on maverick to make sure there's no build-dep issues or other weirdness ?

Also, the package in your PPA is a native package, as in, it doesn't have a .orig.tar.gz. It can be acceptable in some cases but in you case I'd strongly recommend you make an "upstream" tarball, then apply your packaging on top of that.

Let me know once you've solved that small packaging issue and built it in your PPA on maverick. I'll then quickly review again and upload it to extras.ubuntu.com

Thanks

[Emmanuel Thomas-Maurin (manu-tm)]

The reason it was built for lucid is I'm still using it and I remember there were dependencies issues when I uploaded and tried to build a maverick "labelled" version. But I probably missed something.

Also, about the "upstream" tarball, could you give me a link to some relevant doc (I'm still quite "novice" to debian packaging) ?

Thank you.

[Stéphane Graber (stgraber)]

https://wiki.ubuntu.com/PackagingGuide/Complete describes the use of .orig.tar.gz in packaging.

Basically you want to make a tarball that only contains your source code and that'd be called news_0.4.6.orig.tar.gz, then unpack it, add the debian directory and run: debuild -S -sa

That should give you the following files:
 - something.dsc
 - something.changes
 - something.orig.tar.gz
 - something.diff.gz or something.debian.tar.gz (if using 3.0 quilt format)

[Emmanuel Thomas-Maurin (manu-tm)]

I made the changes and it seems ok.

I had to make a new ppa (the new url is: https://launchpad.net/~manu-tm/+archive/newsrssticker instead of .../news-rss-ticker) so that I could upload.

[Emmanuel Thomas-Maurin (manu-tm)]

There is now another issue. I have sources and binaries for maverick but can't have stuff for lucid in the same ppa anymore. So should I have different ppa's for different series? What's the recommended way to do that (and avoid futur weirdness.)

[Stéphane Graber (stgraber)]

It's indeed impossible to have multiple packages with the exact same version string in the same PPA.
That's a limitation caused by the way packages are stored in an apt repository.

Usually, I recommend pushing the main package with the final version string and for other distros, append a ~<version>1 to your version string. ~lucid1 for example for the first build of the package on lucid.

[Emmanuel Thomas-Maurin (manu-tm)]

For maverick, it's: news_0.4.6-0extras10.10.1

So, for lucid: news_0.4.6-0extras10.10.1~lucid1

If this is ok, I will upload the src package for lucid now.

[Emmanuel Thomas-Maurin (manu-tm)]

It worked fine and everything seems ok now. Thanks.

[Emmanuel Thomas-Maurin (manu-tm)]

I have uploaded the "upstream" tarball on my PPA and built it on maverick.

[Stéphane Graber (stgraber)]

Looks good, I'll upload it now.

Just a note for next time, you may want to drop the .orig from the directory name.
Currently in news_0.4.6.orig.tar.gz you have a directory called news_0.4.6.orig which should instead be called news_0.4.6.

Thanks for all the work

[Stéphane Graber (stgraber)]

Checking signature on .changes
gpg: Signature made Tue 29 Mar 2011 01:39:46 PM EDT using RSA key ID 64792D67
gpg: Good signature from "Stéphane Graber <email address hidden>"
gpg: aka "Stéphane Graber <email address hidden>"
Good signature on /home/stgraber/Desktop/news/news_0.4.6-0extras10.10.1_source.changes.
Checking signature on .dsc
gpg: Signature made Tue 29 Mar 2011 01:39:43 PM EDT using RSA key ID 64792D67
gpg: Good signature from "Stéphane Graber <email address hidden>"
gpg: aka "Stéphane Graber <email address hidden>"
Good signature on /home/stgraber/Desktop/news/news_0.4.6-0extras10.10.1.dsc.
Package includes an .orig.tar.gz file although the debian revision suggests
that it might not be required. Multiple uploads of the .orig.tar.gz may be
rejected by the upload queue management software.
Uploading to ppa (via ftp to ppa.launchpad.net):
  Uploading news_0.4.6-0extras10.10.1.dsc: done.
  Uploading news_0.4.6.orig.tar.gz: done.
  Uploading news_0.4.6-0extras10.10.1.diff.gz: done.
  Uploading news_0.4.6-0extras10.10.1_source.changes: done.
Successfully uploaded packages.

Marking fix commited for now until it reaches the repository.

[Emmanuel Thomas-Maurin (manu-tm)]

Thanks!

[Emmanuel Thomas-Maurin (manu-tm)]

It's in extras.ubuntu.com now.

[Stéphane Graber (stgraber)]

Finally ;)

Ok, closed the bug as fix released. People should now see it in software center.

Thanks for all the work.