Various fixes in debian dir

Loïc: can you take a look at it?

Hi Julien,

0) I had a look at your proposed changes, but since I don't know AWN at all, I need a little more background: was this change discussed upstream? Is the patch from upstream or blessed by upstream?

1) Could you explain the rationale for the move of awn-manager to Depends? (Perhaps Breaks would be a better fit?)

2) On the patch itself, it might be a temporary workaround, but I still want to note a couple of things:

a) you might want to follow the upstream coding style when patching upstream sources, for example you mix tabs and spaces for indentation, you use "if (...) {" and "if (...)\n{", you use "/*comment*/" and "/* comment */" etc.

b) I see fields to access .desktop files are mostly used via #defines (e.g. GNOME_DESKTOP_ITEM_TYPE), perhaps you want to #define some macro to "X-AWN-AppletType" as well?

c) usually, one uses stderr to output error messages like the one you added; you could change the printf to fprintf and use stderr instead? Perhaps upstream code has some logging facility / macros for such messages?

d) the error message isn't very helpful when the name field isn't set ("Please inform the developer(s) of the applet 'Unknown'[...]"); I recommend you include the name of the .desktop file in the message or use only that instead of the name (the name doesn't really matter to report it)

Bye,

Hi Loïc,

Thanks for the review.

0) The patch is from upstream.

1) As far as I know, apt on Ubuntu don't install Recommand. So to be sure to install awn-manager, I move it to depends. It's just to be sure it'll be install with avant-window-navigator (because a "normal" user will use awn-manager which is the GUI for configuration). You can use it without awn-manager, that why I put it as Recommand.

2) Yes, even for upstream. I did make this patch, but I can contact the author about your comments :)

i) Ok, so I personally find the patch a bit ugly and would love you sending my critics and/or fixes upstream; especially the remarks about the error message being useless and being sent to stdout instead of stderr. I would feel a bit dirty to bless the addition of such a patch. If it was merged as is in the upstream code base, then I think it needs fixing upstream.

Also, I would be more happy with a slightly longer / more precise explanation on the patch in the changelog (or if you prefer, in the patch header itself); perhaps something like:
  * New patch, 03-Accept-specific-desktop-file, now use X-AWN-AppletType in AWN applets' .desktop files or output a warning if this field is missing.

ii) Concerning the Recommends, indeed Ubuntu doesn't install Recommends by default; to keep the rationale for the bump in some place, and hence to allow us to switch back from Depends to Recommends when Ubuntu installs these by default, I'd suggest being a little more explicit in the changelog:
  * Promote Recommends to Depends to force installation of awn-manager as Ubuntu doesn't install Recommends by default.

(NB: If you open an upstream bug / start a mailing-list discussion for the first point, then probably I'll sponsor an updated package with the proposed changelog tweaks, seeing that the upstream fix is making progress on its own.)

I subscribe the person which made the patch. Upstream developpement is quite "unstable" (the main developper is not available). I added the modification you suggest for the changelog for the two changes. Thanks for the suggest.
Tell me if there is other changes to do.
Thanks for the review.

It might be worthwhile to take a look at the the version of the patch that made it into awn-core-testing. It was significantly cleaned up, and slightly restructured, over the initial patch.

1) printf was replaced with g_warning which is consistent with awn-core practices.

2) The coding style was changed to reflect that of the file being patched. It should be noted that there does tend to be a certain level of deviation in coding style with the core itself though it's _relatively_ consistent. awn-extras is much less consistent. I do apologize for the tabs - my editor does not seem to be doing what I want it to do.

3) Ack'd on #define.

4) The error message comment is not without merit. In the unlikely, though still possible, chance that the name field is not set this error message will be immediately preceded by a message indicating the desktop file(s) in question. But, in theory, it probably would be a bit more solid if the change was made and I will make a note about getting that done.

Thanks for the comments. We do appreciate hearing about these things.

Perfect; would one of you please update the debdiff with the patch which made it to the tree?

@Julien, your new changelog looks nice, but make sure you wrap it at 78 or so; the Recommends line in the changelog looks longer in the diff, but I could be wrong.

Thanks!

Problem : other things have been merge in the awn-core-testing branch, which introduce new stuff on this part of the code, so the patch will not apply.

I'll try and get you a patch that will apply. Kind of busy so it might take a couple days.

Just specify the branch and revision that you would like it against.

I've unsubscribed ubuntu-univese-sponsors pending the updated patch. Please resubscribe to request upload once it is available.

This is new update of the debian package without the patch. After some test, it appears that it need also many modifications to awn-extras-applets to be usefull. So I prefer to remove it for now, maybe I'll re-add it for a future release.

Hi Julien,

I had a look at your debdiff with fresh eyes and noticed some areas where the packaging could be enhanced a little, but I've decided to upload the changes nevertheless as it was already quite good. I'm listing some suggestions below, you're welcome to fix them in a new debdiff in this report or open a new report with a new debdiff, but you don't need to.

a) You change the watch file as follows:
-https://launchpad.net/awn/+download https://launchpad.net/awn/*/*/+download/avant-window-navigator-(.*)\.tar\.bz2
+https://launchpad.net/awn/+download https://launchpad.net/awn/.*/avant-window-navigator-(.+).tar

presumably upstream had some releases as .tar.bz2 and others as .tar; perhaps you can simply change the end of the regexp to match both types of releases?

b) libawn-dev could depend on ${shlibs:Depends}; for example at some point it might ship binaries which are not linked to the exact same libraries; it's good practice to list these in all cases and be on the safe side

c) Not sure you need to honor the Maintainer field spec here as I couldn't find a corresponding Debian source package; you might simply be able to take the Maintainer: and forget about XSBC-OriginalMaintainer; I'm not 100% sure though.

d) "XS-Python-Version: current" isn't terribly supported; you might want to list a version such as ">= 2.4" and use the matching python dep (e.g. python (>= 2.4))

e) The python-awn package is arch: any which suggests it builds python extensions; in this case, you're supposed to change the build to be run one time per python version (as returned by "pyversions -r") and if you like one time with the debug python

f) python-awn doesn't "Provides:" ${python:Provides}

g) You might want to fix this lintian warning about the casing of Python:
W: python-awn: spelling-error-in-description python Python

Thanks again for the update!

Hi Loïc,

Thanks for the comment, I'm always happy to have good advises :)

a) Actually, there is only tar files for release, first watch file was not good at all. But I'll add .bz2 + tar.gz for the upload.

b) Good point.

c) The package is waiting in the NEW queue for Debian, so I think I should let the Maintainer field as it is now in Ubuntu.

d) Yes, it's a better way.

e) I don't know how to do this. Do you have an example of package doing this ? I tried to add PYVER to the debian/rules, but it's only build for 2.4 (testing on a Debian system).

f) I added this, but it didn't change anything when I created the package.

g) Oups forget this one.

I'll try to add this to the Debian package and synchronise it with Ubuntu to reduce the diff.

Thanks again for the comments :)

> e) I don't know how to do this. Do you have an example of package doing this ? I tried to add PYVER to the debian/rules, but it's only build for 2.4 (testing on a Debian system).

Depends of your packaging; if it's CDBS, it's a bit awkward, but you can check e.g. gnome-menus; if it's debhelper only, you can check gst0.10-python.

f) That's quite bad; check you have a dh_pysupport / dh_pycentral call for this package; perhaps you need to pass some arguments to this call to tell about private dirs; you should end up with python2.5-awn in provides since you build the 2.5 version.