[Debian GNUstep maintainers] preview.app uploaded to mentors
Federico Gimenez Nieto
fgimenez at coit.es
Sat Nov 28 10:51:55 UTC 2009
Hi Yavor,
On Fri, 2009-11-27 at 14:43 +0200, Yavor Doganov wrote:
[...]
> > The directory structure is modified to comply with the FHS and with
> > the policy, now the Resources directory contents are installed under
> > the directory /usr/share/GNUstep/Preview.app, and a symlink points
> > to it from
> > /usr/lib/GNUstep/Applications/Preview.app/Resources.
>
> This approach will not work; dpkg will not replace a real directory
> with a symlink so all upgrades will be broken (the app will fail to
> find the Info-gnustep.plist and the .gorm files and will complain with
> "Bad application class"). More importantly, it wont work anymore at
> all (you can reproduce this by installing preview.app from the
> official archive, and then your package).
>
> You need to add a preinst script to remove the old Resources dir. See
> the gnumail package from Etch for an example. You can then drop the
> preinst entirely for Squeeze+1.
>
Ok, done.
> > There are other minor changes
>
> Here are some comments, in decreasing order of importance (the last
> ones are entirely cosmetic):
>
> 1) The removal of the binary-arch target is very wrong. This is
> certainly an architecture-dependent package (see the output of
> `file /usr/lib/GNUstep/Applications/Preview.app/Preview').
>
Reverted, i mistook 'Architecture: any' for 'Architecture: all' at
debian/control.
> 2) Please export GNUSTEP_MAKEFILES and replace every occurrence of
> `gs_make' with `$(MAKE)'. Sub-make invocations should always use
> the $(MAKE) variable so that `make' passes all options and
> variables via its internal $(MAKEFLAGS) variable. Currently it
> works as it is, but probably won't work for parallel builds and may
> lead to weird bugs in the future that are hard to debug.
>
Ok, done, GNUSTEP_MAKEFILES exported at the beginning of debian/rules
> 3) GNUstep Make has a bug [1] (fixed in SVN trunk) that by default
> builds unoptimized binaries, which is a Debian Policy violation.
> It is trivial to make it do the right thing, for example:
>
> ifneq (,$(findstring noopt,$(DEB_BUILD_OPTIONS)))
> OPTFLAG := -O0
> else
> OPTFLAG := -O2
> endif
>
> build-stamp:
> ...
> $(MAKE) OPTFLAG=$(OPTFLAG)
Done
>
> 4) Please consider making the build log verbose by adding `messages=yes'
> to the $(MAKE) invocation. We've been bitten before by
> incompatible changes that were hidden in silent builds. (Like, for
> example, the cited gnustep-make bug).
>
Ok, done
> 5) The second `cp' after `mv' in debian/rules where you move the
> Resources dir seems redundant to me. `mv' will copy the whole
> contents recursively.
This cp came from the previous version. It seems to copy
English.lproj/Preview.gorm/* to French.lproj/Preview.gorm/ within the
Resources directory, and in my opinion it should be still needed after
the move of the whole Resources directory, please correct me if i am
wrong.
> It is better not to hardcode the path; it
> changed before and may change in the future. When it happens, your
> package will need amendments. Here's an example from agenda.app:
>
>
> ifeq ($(GS_USE_FHS),yes)
> dh_installdirs usr/share/GNUstep
> mv $(d_app)$(GNUSTEP_SYSTEM_APPS)/SimpleAgenda.app/Resources \
> $(d_app)/usr/share/GNUstep/SimpleAgenda.app
> dh_link usr/share/GNUstep/SimpleAgenda.app \
> $(GNUSTEP_SYSTEM_APPS)/SimpleAgenda.app/Resources
> endif
>
> The conditional is to be nice to the GNUstep LiveCD maintainer: he
> doesn't like the FHS very much :-) and prefers the Resources to be
> in the app bundle. It is not a requirement to conditionalize the
> move, of course.
>
> There's also a typo in the comment: s/FSH/FHS/.
Ok, added with the conditional move.
>
> 6) clean target: `$(MAKE) clean distclean' was necessary to workaround
> an ancient gnustep-make bug; you can simply do `$(MAKE) distclean'
> these days.
>
Done
> 7) Please consider adding an icon for debian/menu, for those people
> who use window managers that support icons in the menus. See
> agenda.app for one (easy) way to do it.
>
There are three tiff images that could be used, FileIcon_jpeg.tiff,
FileIcon_png.tiff and FileIcon_tiff.tiff, all of 48x48 pixels in size,
will any of them work as a menu icon?
> 8) debian/Preview.desktop is not valid according to
> `desktop-file-validate'. The Version field is the version of the
> spec (1.0, I think). The MimeType field should be removed
> entirely, because Preview.app doesn't support command-line options
> and right-clicking on an image file in environments that support it
> (e.g. GNOME) does not actually open it. IOW, if
>
> $ Preview foo.png
>
> doesn't work from the console, it shouldn't have MimeType defined
> in its .desktop file as it just clutters the context menu.
>
Done
> 9) You can remove the versioned build-dependency on libgnustep-gui-dev
> (0.14 is in Lenny) and gnustep-make entirely (pulled in by the
> dependency chain).
>
Ok, done
> 10) Add ${gnustep:Depends} to the Depends field, and invoke
> gsdh_gnustep in the binary-arch target to populate it. (This is
> again wishlist item -- to prevent dpkg from installing the package
> on systems that have Debian GNUstep packages configured not to
> follow the FHS.)
>
Done
> 11) You can get rid of the dpkg-shlibdeps warnings (if they annoy you)
> with LDFLAGS, see agenda.app for example. This is mostly
> cosmetic, because the extra linking imposed by gnustep-make
> doesn't bring in new dependencies via ${shlibs:Depends}, except
> libgcc1.
>
Ok, added LDFLAGS
> > Just one question regarding the package, there are two empty
> > directories in the installation,
> > Resources/English.lproj/Preview.help/ and
> > Resources/French.lproj/Preview.help/, is this intentional?
>
> Apparently they were supposed to contain the unwritten
> documentation...
>
> > Should we get rid of them?
>
> Yes, you can safely delete them; they're useless.
>
Done
> As a final comment, I'm not sure you're aware: This is an abandoned
> app with dead upstream, unfortunately like a lot of GNUstep packages.
> PRICE (price.app) and LaternaMagica (not in Debian) are considered
> replacements, although personally I still use Preview.app (old habits
> die hard, and I've translated it :-)). So you're pretty much on your
> own to fix all bugs.
Well, i'll try :)
> (Of course, we'll gladly help if needed.)
>
Ok, I'll take your offer for sure ;) I've uploaded a new version with
all these changes, [1]
[1] http://mentors.debian.net/debian/pool/main/p/preview.app/preview.app_0.8.5-6.dsc
Thanks a lot,
Federico
More information about the pkg-GNUstep-maintainers
mailing list