[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