SRU: katapult

Debdiff attached

Original Bug
https://launchpad.net/products/katapult/+bug/60136

As previously discussed on IRC, the current version of katapult in edgy
does not work correctly with amarok in edgy.

This was due to the fact that amarok updated the way that their file
database worked with respect to mountpoints.

A patch for this was included in the current edgy version, however, this
was an immature patch which had not been tested, and while it compiles,
it does not create the functionality intended.

I have created a patch for this fix, which I have tested under a
pbuilder that it compiles, and have also tested that the functionality
the patch gives works correctly.

I would like this to be approved for an SRU so that the functionality
that was available in dapper is also available in edgy.

Bug for SRU:
https://launchpad.net/distros/ubuntu/+source/katapult/+bug/69479

Bug: https://launchpad.net/products/katapult/+bug/60136

Debdiff to current package:
http://librarian.launchpad.net/4941877/kat-debdiff

Kindest Regards,
Martin Meredith

The changes are hard to read due to diffing a diff; can you do an interdiff or otherwise show the actual changes being applied to the source?

Patch attached (replacement for current kubuntu_06_amarok_14.diff

Can someone interdiff the two patches to show what changed?

On Tue, 2006-10-31 at 22:44 +0000, Matt Zimmerman wrote:
> Can someone interdiff the two patches to show what changed?
>

Attached

Doesnt seem to want to let me attach via email

Bug 56918
(https://launchpad.net/distros/ubuntu/+source/katapult/+bug/56918) is
also affecting katapult in edgy, meaning that in some translated
environments, katapult will not display the launcher, or allow any use
of it without crashing)

In that bug I have created patches/interdiffs for the patches in the
current katapult debian package that need updating.

I will include a new .diff.gz and a debdiff between the updates and the
current edgy package

new debdiff for both issues

new .diff.gz

ping for sru team

debdiff shows no changes except the changelog

Sorry - dont know what happened there.

debdiff attached

try again... that was a listing of the files changed for some reason

On Mon, Nov 20, 2006 at 03:33:53AM -0000, Martin Meredith wrote:
> try again... that was a listing of the files changed for some reason
>
> ** Attachment added: "Here we go"
> http://librarian.launchpad.net/5112243/kat.debdiff

Remember to check the box that says this is a patch. Among other things,
that makes sure that the content-type is set correctly. Otherwise, it
defaults to HTML and is unreadable.

--
 - mdz

The debdiff has many unrelated changes in it, beyond the interdiff attached earlier. Please review it with Jonathan and re-submit to SRU once the changes are clear and obvious.

There are no unrelated changes.

The changes you might be thinking are unrelated are for Bug 56918 - as noted in comment 9 https://launchpad.net/distros/ubuntu/edgy/+source/katapult/+bug/69479/comments/9

On Wed, Nov 22, 2006 at 06:59:03AM -0000, Martin Meredith wrote:
> There are no unrelated changes.
>
> The changes you might be thinking are unrelated are for Bug 56918 - as
> noted in comment 9
> https://launchpad.net/distros/ubuntu/edgy/+source/katapult/+bug/69479/comments/9

The SRU team can't spend time tracking down the origin of changes; it needs
to be clear and obvious for review. The changelog must describe each change
and refer to the bug which it fixes. Entries such as "Update of
kubuntu_06_amarok_14.diff to working version" don't explain what the problem
was or how it was fixed; it's equivalent to saying "this file was changed".

--
 - mdz

Here's a debdiff which tidies up the amarok patch to remove unnecessary changes and replaces the other changes with the necessary typo fix to kubuntu_06_amarok_14.diff.

s/kubuntu_06_amarok_14.diff/kubuntu_01_o2display.diff/

The effective diff following Jonathan's proposed change in https://launchpad.net/distros/ubuntu/+source/katapult/+bug/69479/comments/20 (calculated by applying the patches on both sides and then excluding debian/patches from the resulting diff) is attached. This is somewhat more legible than the diff-of-a-diff nightmare that patch systems inflict upon us for this sort of thing.

The X-Katapult-ID change and the KURL setPath/addPath/cleanPath changes look fine for edgy-proposed.

Some qDebug statements are removed. Is this a good idea? It's not a minimal change, at any rate; if it's not necessary to remove them then please leave them be.

The sqlResult[2]=="-1" test was inverted, even though the body of each branch of that if seems to be logically the same. Was this intentional?

What is the purpose of this change?

- QString sqlQuery("SELECT a.name, t.title, t.deviceid, d.lastmountpoint, t.url, i.path, album.name FROM tags t, artist a, album LEFT JOIN statistics s ON t.url = s.url AND t.deviceid = s.deviceid LEFT JOIN images i ON (a.name = i.artist AND album.name = i.album) LEFT JOIN devices d ON t.deviceid = d.id WHERE t.album = album.id AND t.artist = a.id"); // AND
+ QString sqlQuery("SELECT a.name, t.title, t.deviceid, d.lastmountpoint, t.url, i.path, album.name FROM tags t LEFT JOIN statistics s ON t.url = s.url AND t.deviceid = s.deviceid LEFT JOIN artist a ON t.artist = a.id LEFT JOIN album ON t.album = album.id LEFT JOIN images i ON ( a.name = i.artist AND album.name = i.album) LEFT JOIN devices d ON t.deviceid = d.id WHERE 1");

It appears to be moving stuff from a WHERE into a LEFT JOIN. Is this necessary? Please explain. My SQL knowledge isn't adequate to approve this without an explanation.

The changelog is still not satisfactory for edgy-proposed. As Matt said, "Update of kubuntu_06_amarok_14.diff to working version" is not good enough. Please explain *what changes were made to that file*! For example, assuming that only the KURL changes are necessary, something like "Canonicalise dynamic collection URLs before passing them to amarok" would be more helpful (although please don't just copy my text as I'm sure you know the content of the change better than I do, and so should be able to describe it better). The reason why you need to do this is that this is the kind of level on which we need to be thinking when approving changes, and it's the sort of explanation that we ought to give to reasonably technically-minded users when explaining to them why we chose to issue a stable release update (particularly in the event that the update breaks).

Colin had some questions, please answer. Also, please update the bug status for Feisty. It needs to be 'fix released' for a SRU to be approved.

Martin, is this a dup of bug 60136?

The SQL query change is due to the fact that The SQL wasn't working correctly under both pgSQL and mySQL. The

- if (sqlResult[2]=="-1") {
- _result.setURL(KURL(sqlResult[3]+sqlResult[4].mid(1)));
+ if (sqlResult[2]!="-1") {

Changes are correct (It should be setting the "root" path as "/" if the result is -1 This was an error before.

Martin, 60136 is the original bug that this SRU relates to (https://launchpad.net/ubuntu/+source/katapult/+bug/69479/comments/2)

FYI, I unsubscribed ubuntu-sru from this bug to clean up the list. Please:

 - get this fixed in Feisty first
 - please answer/fix the package for Matt's and Colin's remarks.
 - subscribe ubuntu-sru again with a proper SRU proposal as per https://wiki.ubuntu.com/StableReleaseUpdates

Thank you!

No idea why ubuntu-release is subscribed to this; please file a proper UVFe request if that is what you want.