Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 185951 - Review Request: amsn : msn messenger clone
Summary: Review Request: amsn : msn messenger clone
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Wart
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On: 186327 193853
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-03-20 13:21 UTC by Sander Hoentjen
Modified: 2007-11-30 22:11 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-06-21 05:28:03 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Sander Hoentjen 2006-03-20 13:21:16 UTC
Spec Name or Url: http://amsn.sourceforge.net/tjikkun/amsn.spec
SRPM Name or Url: http://amsn.sourceforge.net/tjikkun/amsn-0.96-0.1.20060320cvs.src.rpm
Description: This is a Microsoft Messenger (MSN Messenger) clone for Unix, Windows, and Macintosh platforms.
It is written in tcl/tk and supports file transfers, groups, and many more features.
Visit http://amsn.sourceforge.net/ for details.

I am a new packager (I work on the amsn-project) so I am seeking for a sponsor.

Comment 1 Christopher Stone 2006-03-21 10:21:17 UTC
Here is my first review.

*MUSTs*
- Package fails rpmlint checks, shorten descriptions, fix symlinks and set the
correct executable bits on files, and fix line endings (see package guidelines)
use rpmlint to check your packages for errors and warnings
- Package name OKAY
- spec file name OKAY
- license OKAY
- written in english OKAY
- legible OKAY
- url link OKAY
- md5sums do not match that of SRPM.  You should provide instructions in
comments on how to get the exact sources found in the SRPM.
98871a6bd5b19cc267b4221cedaec863  amsn_cvs.tar.gz
d9dcd3587b6df37e39f0c4b59073fd31  ../SOURCES/amsn_cvs.tar.gz
- sources build on FC5.x86_64 OKAY
- no exceptions in BR OKAY
- package not owning its directories BAD
- package contains duplicate files BAD
- permissions are not set properly on every file BAD
- package contains clean section OKAY
- macro use is consistant OKAY
- package contains permissable content OKAY
- %doc files do not affect runtime OKAY
- no static libraries or .la files OKAY
- desktop file looks okay and desktop-file-utils in BR OKAY
- package does not own files in other packages OKAY
- application runs without crashing OKAY

Additional comments:  consider breaking this package up into sub-packages, like
amsn-plugins and amsn-skins

This is my first ever review, sorry if it's a bad one.

Comment 2 Sander Hoentjen 2006-03-22 22:29:04 UTC
Thank you very nuch for your review, I think I fixed the problems.
Spec Name or Url: http://amsn.sourceforge.net/tjikkun/amsn.spec
SRPM Name or Url:
http://amsn.sourceforge.net/tjikkun/amsn-0.96-0.1.20060322cvs.src.rpm

about md5sums not matching: a new amsn_cvs.tar.gz is created every 6 hours. I
can do 2 things:
-do a cvs checkout with a date attached so it is reproducable
-copy the tar.gz to a different location/name and reference that. Which would be
the prefered one?
for now i put it up at http://amsn.sourceforge.net/tjikkun/amsn_cvs20060322.tar.gz



Comment 3 Christopher Stone 2006-03-23 23:41:08 UTC
Probably copying the tar.gz to a different location/name would be eaiser for the
reviewers.

Comments on this second release:
- The package should have a release of 0.2.%{alphatag}%{?dist} and there should
be changelog entries showing what you have changed since release 0.1

- This does not build correctly under a mock environment.  You must add atleast
which and mlocate to the buildrequires.  In order to build with FC4 you will
need to replace mlocate with slocate.  Even with mlocate I get the following
error in the configure:

checking tcl build dir... locate: can not open `/var/lib/mlocate/mlocate.db': No
such file or directory

-It also requires an X11 devel package.  I tried adding libX11-devel to the
buildrequires, but configure still does not find X11:

checking for X... no
"Could not find X11 devel package

NOTE:  Even the error message in configure is not being displayed correctly,
there must be something wrong in the configure.ac file.  Unless the proper
Buildrequires are set, the X11 will be set to NO when compiling this package.

-I still think the package should be divided up into sub packages. 
amsn-plugins, amsn-skins, amsn-utils?  I'm not sure if this is a requirement or
not, but a real reviewer might require it.

Comment 4 Christopher Stone 2006-03-23 23:44:12 UTC
Oh one more thing I forgot to mention.  The Requires of tcl >= 8.4, tk >= 8.4,
tcltls >= 1.5 should be removed.  rpm automatically detects the requirement of
tcl and tk, and the tcltls package is not part of Fedora.

Comment 5 Christopher Stone 2006-03-23 23:50:28 UTC
Oh nevermind my comment about tcltls, I didn't notice the tcltls package you
also submitted.

Comment 6 Sander Hoentjen 2006-03-27 08:08:57 UTC
Ok I think I fixed all problems:
Spec: http://amsn.sourceforge.net/tjikkun/amsn.spec
SRPM: http://amsn.sourceforge.net/tjikkun/amsn-0.96-0.2.20060327cvs.src.rpm

"-I still think the package should be divided up into sub packages. 
amsn-plugins, amsn-skins, amsn-utils?  I'm not sure if this is a requirement or
not, but a real reviewer might require it."

I agree, my intent is to divide this package into amsn, amsn-plugins and
amsn-skins. amsn-utils is not very usefull i think, since everything that would
be in there would be required by amsn. For utils that can be used outside of
amsn I will create seperate packages, if not yet existing.



Comment 7 Jef Spaleta 2006-04-20 20:50:04 UTC
Having difficulty getting to the srpm and the spec at the provided urls. sf is
giving me 404 errors.

Is this submission request and the associated 186327 submission request dead?

-jef

Comment 8 Sander Hoentjen 2006-04-21 03:54:32 UTC
No,

they are not dead. We had space problems at our sf.net group account so they had
to be removed. I will put up new versions of both packages today or next monday
on a different server. Unfortunatly i did not have had ANY free time before,
sorry for that.

Comment 10 Sander Hoentjen 2006-05-18 08:31:49 UTC
I updated the spec and srpm to a new upstream version.
I also removed some tcl packages that were shipped with amsn that i now Require:

amsn 0.96 is going to be released soon so I hope someone will sponsor me so I
can release the rpm at the same time.

http://amsn.hoentjen.eu/download/amsn.spec
http://amsn.hoentjen.eu/download/amsn-0.96-0.5.20060517svn.src.rpm

Comment 11 Hugo Cisneiros 2006-05-18 08:56:09 UTC
BTW, I don't think "soon" is enough. You should provide a package to the 
current version too, getting into Extras first, then updating it as the next 
release comes. I say this because leaving the release for later isn't good. I 
think the most important thing is to bring this good app to Extras right now, 
with the current stable version, and then updating it when a new version is 
release (we all don't know when this will occur, and looking at amsn's 
history, I think we got to wait ;-)

So the summary is: please make a package for version 0.95 :)
I would be glad to follow your steps and help on what I can.

Comment 12 Sander Hoentjen 2006-05-18 09:14:38 UTC
Ok, I did it this way because I fixed some stuff inside amsn, to get it to
package correctly. I understand your point and I know amsn doesn't really have a
reliable release cycle, but 0.96 might actually be released less then a year
after 0.95 ;)
Also when I said: "I hope someone will sponsor me so I can release the rpm at
the same time" I didn't mean i wouldn't release the svn version (since it is
actually more stable then 0.95, 0.96 is more or less a bugfix release).
Anyway, I will do an amsn 0.95 package as soon as possible and wait for 0.96 to
be released if that is the way to go.

Thanks for your comment

Comment 13 Wart 2006-05-18 18:59:37 UTC
amsn builds against local copies of libpng, libjpeg, and zlib, which is a no-no
for Fedora Extras.  This must be changed to build against the system copies. 
You don't have to remove them from the sources, just make sure it links against
the existing libpng/libjpeg/zlib, and add "BuildRequires: libpng-devel
libjpeg-devel zlib-devel"

Comment 14 Wart 2006-05-18 19:09:07 UTC
It also includes a copy of the BWidget tcl package, which I've already packaged
for FE.  Just add "Requires: bwidget" and don't include the local copy.

Comment 15 Paul Howarth 2006-05-18 19:30:26 UTC
(In reply to comment #13)
> amsn builds against local copies of libpng, libjpeg, and zlib, which is a no-no
> for Fedora Extras.  This must be changed to build against the system copies. 
> You don't have to remove them from the sources, just make sure it links against
> the existing libpng/libjpeg/zlib, and add "BuildRequires: libpng-devel
> libjpeg-devel zlib-devel"

I had a similar issue to this with gtkwave, which bundles zlib and bzip2. I made
absolutely sure that the system libraries were used by not only patching the
Makefiles but deleting the bundled libraries from the unpacked sources in %prep
so that there was no possibility of building and linking against them.

Comment 16 Sander Hoentjen 2006-05-20 11:02:48 UTC
(In reply to comment #14)
> It also includes a copy of the BWidget tcl package, which I've already packaged
> for FE.  Just add "Requires: bwidget" and don't include the local copy.
The one that is used by amsn is modified, amsn won't work with the original one.


(In reply to comment #15)
> (In reply to comment #13)
> > amsn builds against local copies of libpng, libjpeg, and zlib, which is a no-no
> > for Fedora Extras.  This must be changed to build against the system copies. 
> > You don't have to remove them from the sources, just make sure it links against
> > the existing libpng/libjpeg/zlib, and add "BuildRequires: libpng-devel
> > libjpeg-devel zlib-devel"
> 
> I had a similar issue to this with gtkwave, which bundles zlib and bzip2. I made
> absolutely sure that the system libraries were used by not only patching the
> Makefiles but deleting the bundled libraries from the unpacked sources in %prep
> so that there was no possibility of building and linking against them.

Done, see:
http://amsn.hoentjen.eu/download/amsn.spec
http://amsn.hoentjen.eu/download/amsn-0.96-0.6.20060517svn.src.rpm


Comment 17 Wart 2006-05-22 23:15:18 UTC
(In reply to comment #16)
> (In reply to comment #14)
> > It also includes a copy of the BWidget tcl package, which I've already packaged
> > for FE.  Just add "Requires: bwidget" and don't include the local copy.
> The one that is used by amsn is modified, amsn won't work with the original one.

How was it modified?  Were these changes sent back upstream?  Depending on the
nature of the changes and upstream's willingness to include them, we might
consider patching bwidgets in FE instead of bundling an extra copy of bwidget here.

Comment 18 Sander Hoentjen 2006-05-24 07:23:44 UTC
I will try to find this out. If they were not send upstream I will contact them
to see if they accept the changes. If they accept I will also let you know. In
the meantime, is it ok to have Amsn_BWidget included?

Comment 19 Wart 2006-05-25 17:40:26 UTC
I took a look at Amsn_BWidget to see what the changes are, and they don't seem
very major to me.  Hopefully upstream will agree and accept the changes.

* Some documentation and image updates
* improvements to the font selector to allow disabling the font color, size, and
style
* Some minor widget layout changes (reordering buttons, changing borderwidths)
* New methods in the scrolledwidget for resizing
* A handful of modified RCS strings on otherwise-unmodified files

But I don't understand the reasoning behind a couple of the changes:

* Removal of -background setting in scrolledwindow
* The extra rename of ::$path:cmd in widget.tcl

A possible alternative is to include only the modified files in Amsn_BWidget,
and rely on the FE bwidget package for all of the rest:

  # Pull the base BWidget from FE
  package require BWidget

  # Pull in only the modified files from Amsn
  package require Amsn_BWidget

This might take some work to do right (hacking the pkgIndex.tcl file, probably),
but should allow you to use modified BWidget files without including the entire
BWidget library in amsn.

Comment 20 Sander Hoentjen 2006-06-08 14:52:12 UTC
http://amsn.hoentjen.eu/download/amsn.spec
http://amsn.hoentjen.eu/download/amsn-0.96-0.7.20060608svn.src.rpm

This version uses upstream BWidget, not aMSN's one. Also this version is one
very close to the 0.96 release. Only some changes to the TLS downloader (which
this FE package does not use) are expected.

(In reply to comment #19)

> But I don't understand the reasoning behind a couple of the changes:
> 
> * Removal of -background setting in scrolledwindow
> * The extra rename of ::$path:cmd in widget.tcl
Those were made for the tile plugin

Comment 21 Wart 2006-06-16 17:55:02 UTC
Sorry for the delay in the review.  I'll do a full review this weekend.

Comment 23 Paul F. Johnson 2006-06-18 22:21:23 UTC
The $RPM_BUILD_ROOTs need to be consistent

either ${RPM_BUILD_ROOT} or easier, %{buildroot}, but don't mix them.

You're also taking ownership of directories (by the looks of it)

Instead of

%{_datadir}/amsn/foo/

use

%{_datadir}/amsn/foo/*.zip (for example)

You cannot guarantee that later on someone won't create a plugin which needs to
be added to the directory "owned" by the package

Is there not other languages for this package?

If there is - under the %install

%find_lang {name}

Then on the files line

%files -f %{name}.lang

However, some apps install the language files as part of the make install, so
you may find this step fails.

Comment 24 Wart 2006-06-18 22:27:38 UTC
> Is there not other languages for this package?
> 
> If there is - under the %install
> 
> %find_lang {name}
> 
> Then on the files line
> 
> %files -f %{name}.lang
> 
> However, some apps install the language files as part of the make install, so
> you may find this step fails.

There are other language files, but they're stored in %{_datadir}/%{name}/lang,
not %{_datadir}/locale.  I understood that %find_lang was only needed for apps
that use %{_datadir}/locale.

Comment 25 Wart 2006-06-18 22:54:27 UTC
MUST
====
* rpmlint output:
  W: amsn-plugins no-documentation

* Package named appropriately
* GPL license ok, license file included
* Spec file legible
* compiles and packages on FC5 i386 and FC5 x86_64.
* No language files in %{_datadir}/locale, no need for %find_lang
  All translations are stored in %{_datadir}/%{name}/lang
* No shared libs in the default linker path
* Not relocatable
* File permissions look ok.
* $RPM_BUILD_ROOT cleaned where needed.
* Contains code, not content
* Not enough docs to warrant a -doc subpackage
* No reason for a -devel subpackage (no headers, libs, pkgconfig files)
* Does not own directories owned by other packages

MUSTFIX
=======
* Version comparison for tk-devel is not necessary since FC4 and later all
  use tk 8.4.
* The URL in %description isn't necessary since it's already in the URL:
  tag.
* Remove the Version: tag from the plugins subpackage so that it inherits
  the full %{version}-%{release} from the base package.  Otherwise you run
  the risk of building a second, slightly different plugins subpackage later
  that has the same version.
* The 48x48 icon for the .desktop file should go in
  ${RPM_BUILD_ROOT}%{_datadir}/icons/hicolor/48x48/apps/
  and you need to update the icon cache in both %post and %postun:

  touch --no-create %{_datadir}/icons/hicolor || :
  if [ -x %{_bindir}/gtk-update-icon-cache ]; then
     %{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :
  fi
* Missing BuildRequires: autoconf
* BuildRequires: libXt-devel is redundant since it's also required by tk-devel.
* The binaries are installed into /usr/share/amsn/ and linked into /usr/bin.
* There are a number of duplicate files in %{_datadir}/amsn and %doc:
  AGREEMENT, CREDITS, ... These should be removed from %{_datadir}/amsn.
* amsn/lang/LANG-HOWTO should be included in %doc
* The desktop file is placed in %{_datadir}/applications and %{_datadir}/amsn.
  Remove the extra copy in %{_datadir}/amsn since it's not needed there.
* Missing "Requires: mozilla".  I would recommend, however, changing the
  default browser command from 'mozilla $url' to 'htmlview $url' and
  adding "Requires: htmlview".  This will launch the url in the
  user's preferred web browser.
* Missing "Requires: sox" for /usr/bin/play for playing sounds.
* As pointed out above, be consistent about $RPM_BUILD_ROOT vs.
  ${RPM_BUILD_ROOT}
* The %description for the -plugins subpackage doesn't need to include the
  full description for the base package.  Just describe the purpose
  of the subpackage, such as "Extra plugins for amsn to enable foo, bar,
  and baz."

QUESTIONS
=========
* Official upstream sources come from sourceforge.net, but this rc release
  comes from elsewhere.  Since I know you're one of the upstream developers,
  I'm not too concerned about this, but it would be better if Source0:
  pointed to upstream's official download page.

* You might consider submitting cximage as a separate package instead of
  including it in amsn.  Since Fedora doesn't include cximage already,
  this won't block the review.

* Is there a special reason why the amsn commands (amsn, amsn-remote,
  amsn-remote-CLI) are located in %{_datadir}/amsn and only linked to
  %{_bindir}?  Why can't they just be installed directly into %{_bindir}?

* Is it necessary to include the %{_datadir}/amsn/lang/missing.py, convert.tcl,
  and sortlang utilities in the package?


Comment 26 Wart 2006-06-18 22:56:57 UTC
(In reply to comment #23)
> You're also taking ownership of directories (by the looks of it)
> 
> Instead of
> 
> %{_datadir}/amsn/foo/
> 
> use
> 
> %{_datadir}/amsn/foo/*.zip (for example)
> 
> You cannot guarantee that later on someone won't create a plugin which needs to
> be added to the directory "owned" by the package

Except that plugins are stored in %{_datadir}/amsn/plugins.  This plugin dir is
owned by the base package, but not by the -plugins subpackage.  This should be
ok, as future plugins would go into %{_datadir}/amsn/plugins/<foo> and not own
the parent plugins/ directory.

Comment 27 Sander Hoentjen 2006-06-19 10:10:26 UTC
Thank you very much for your comments Paul and Wart.
Paul:
(In reply to comment #23)
I fixed the $RPM_BUILD_ROOT inconsistency. The rest of your comments are already
answered by Wart.
Wart:
(In reply to comment #25)

> * Missing "Requires: mozilla".  I would recommend, however, changing the
>   default browser command from 'mozilla $url' to 'htmlview $url' and
>   adding "Requires: htmlview".  This will launch the url in the
>   user's preferred web browser.
I changed mozilla to htmlview but I am not sure about requiring it. aMSN runs
fine without it, only when you try to open an url it tells you you should check
your preferences. People not having htmlview have chosen for that i guess, since
it is installed by default.
> * Missing "Requires: sox" for /usr/bin/play for playing sounds.
Same as htmlview, aMSN runs fine without sound.
> QUESTIONS
> =========
> * Official upstream sources come from sourceforge.net, but this rc release
>   comes from elsewhere.  Since I know you're one of the upstream developers,
>   I'm not too concerned about this, but it would be better if Source0:
>   pointed to upstream's official download page.
Sorry, I meant to write the reason in my previous comment but forgot. At the
time I had trouble uploading the file to sf, but in the meantime i have done so.
I changed Source0 accordingly. Just be aware that the release is still hidden so
you won't find the file on the project website yet.
> 
> * You might consider submitting cximage as a separate package instead of
>   including it in amsn.  Since Fedora doesn't include cximage already,
>   this won't block the review.
The CxImage supplied with amsn is much different from upstream, and i think not
all patches were accepted so it will stay that way but i am not sure. (I will
check if this is really true)
> 
> * Is there a special reason why the amsn commands (amsn, amsn-remote,
>   amsn-remote-CLI) are located in %{_datadir}/amsn and only linked to
>   %{_bindir}?  Why can't they just be installed directly into %{_bindir}?
Yes, the file perform some magic to see where they are located, and set some
variables accordingly to be able to find plugins etc.
> 
> * Is it necessary to include the %{_datadir}/amsn/lang/missing.py, convert.tcl,
>   and sortlang utilities in the package?
> 
No, language maintainers should use the svn version anyway, so i removed all tools.

http://amsn.hoentjen.eu/download/amsn.spec
http://amsn.hoentjen.eu/download/amsn-0.96-0.9.rc1.src.rpm

Comment 28 Sander Hoentjen 2006-06-19 10:54:12 UTC
Oh I almost forgot, I think I should require tk >= 8.4.13 since you can get a
bug with older versions. Do I put that in for FC5 only or also for devel?

Comment 29 Wart 2006-06-19 16:40:16 UTC
(In reply to comment #28)
> Oh I almost forgot, I think I should require tk >= 8.4.13 since you can get a
> bug with older versions. Do I put that in for FC5 only or also for devel?

That would be fair.

Definitely put it in for FC5, as the tk 8.4.13 package will almost certainly get
pushed to FC5 (if it's not there already).  It should be in devel as well since
releases < 8.4.13 have been published for devel.

Comment 30 Sander Hoentjen 2006-06-19 18:55:28 UTC
(In reply to comment #29)
> That would be fair.
> 
> Definitely put it in for FC5, as the tk 8.4.13 package will almost certainly get
> pushed to FC5 (if it's not there already).  It should be in devel as well since
> releases < 8.4.13 have been published for devel.

ok, I added it

http://amsn.hoentjen.eu/download/amsn.spec
http://amsn.hoentjen.eu/download/amsn-0.96-0.10.rc1.src.rpm

Comment 31 Wart 2006-06-19 21:08:23 UTC
(In reply to comment #27)
> (In reply to comment #25)
> 
> > * Missing "Requires: mozilla".  I would recommend, however, changing the
> >   default browser command from 'mozilla $url' to 'htmlview $url' and
> >   adding "Requires: htmlview".  This will launch the url in the
> >   user's preferred web browser.
> I changed mozilla to htmlview but I am not sure about requiring it. aMSN runs
> fine without it, only when you try to open an url it tells you you should check
> your preferences. People not having htmlview have chosen for that i guess, since
> it is installed by default.

I still think you should add "Requires: htmlview, sox".  If the application  
uses it, it should be in the Requires: list.  Even though htmlview is
installed by default, it's best to make sure that the package has it
available.  Fortunately, amsn won't crash if it's not there, it will just
print out a warning that the user needs to modify their configuration.
I don't think the additional Requires: will hurt anything.

> > * Missing "Requires: sox" for /usr/bin/play for playing sounds.
> Same as htmlview, aMSN runs fine without sound.

See above.

> > QUESTIONS
> > =========
> > * Official upstream sources come from sourceforge.net, but this rc release
> >   comes from elsewhere.  Since I know you're one of the upstream developers,
> >   I'm not too concerned about this, but it would be better if Source0:
> >   pointed to upstream's official download page.
> Sorry, I meant to write the reason in my previous comment but forgot. At the
> time I had trouble uploading the file to sf, but in the meantime i have done so.
> I changed Source0 accordingly. Just be aware that the release is still hidden so
> you won't find the file on the project website yet.

Are there plans to make the RC1 release public?  It'd be really nice to have a
working Source0: link in the spec file that points to SF.  Right now I can't get
to it using curl, wget, or a browser.

> > * You might consider submitting cximage as a separate package instead of
> >   including it in amsn.  Since Fedora doesn't include cximage already,
> >   this won't block the review.
> The CxImage supplied with amsn is much different from upstream, and i think not
> all patches were accepted so it will stay that way but i am not sure. (I will
> check if this is really true)

Ok.

> > * Is there a special reason why the amsn commands (amsn, amsn-remote,
> >   amsn-remote-CLI) are located in %{_datadir}/amsn and only linked to
> >   %{_bindir}?  Why can't they just be installed directly into %{_bindir}?
> Yes, the file perform some magic to see where they are located, and set some
> variables accordingly to be able to find plugins etc.

Ok.  That makes sense.  You might suggest to upstream :) that they validate
$program_dir before trying to resolve symlinks.  This would allow you to use sed
in the spec file to set program_dir to %{_datadir}/%{name} and bypass the
symlink dereferencing in the program.  As a result, the program should still
work as it does now, but it would also work if amsn weren't installed in the
data directory.

A few other items:

MUSTFIX
=======
* Remove the extra " from the comment in the .desktop file
* It turns out that /usr/share/amsn/README is needed at runtime for the
  About box.  You can go ahead and remove it from the list of doc files
  that are deleted during %install.  Do you know of any other doc files
  that are used at runtime?

SHOULD
======
* The Help -> Contents menu item doesn't seem very useful, since it
  describes how to install and start amsn, which the user must have
  already done if they can activate the menu.  I'd suggest to upstream that
  they either remove this menu item, or replace the Help ->Contents text with
  something more useful.

Comment 32 Sander Hoentjen 2006-06-20 07:30:19 UTC
(In reply to comment #31)
> (In reply to comment #27)
> > (In reply to comment #25)
> > 
> > > * Missing "Requires: mozilla".  I would recommend, however, changing the
> > >   default browser command from 'mozilla $url' to 'htmlview $url' and
> > >   adding "Requires: htmlview".  This will launch the url in the
> > >   user's preferred web browser.
> > I changed mozilla to htmlview but I am not sure about requiring it. aMSN runs
> > fine without it, only when you try to open an url it tells you you should check
> > your preferences. People not having htmlview have chosen for that i guess, since
> > it is installed by default.
> 
> I still think you should add "Requires: htmlview, sox".  If the application  
> uses it, it should be in the Requires: list.  Even though htmlview is
> installed by default, it's best to make sure that the package has it
> available.  Fortunately, amsn won't crash if it's not there, it will just
> print out a warning that the user needs to modify their configuration.
> I don't think the additional Requires: will hurt anything.
> 
Ok, in the past I have been pissed off when i wanted to remove something, and it
pulled in lots of other stuff which I didn't see the point in. I added the
requires for sox, htmlview and tkdnd (since that seems to fall in the same
category, without it drag and drop doesn't work)
> 
> Are there plans to make the RC1 release public?  It'd be really nice to have a
> working Source0: link in the spec file that points to SF.  Right now I can't get
> to it using curl, wget, or a browser.
Yes there are plans, but we want all packages created first, and synced to
mirrors. In the meanwhile go to:
http://prdownloads.sourceforge.net/amsn/amsn-0.96rc1.tar.bz2?download since what
is in Source0 should work, but doesn't because of sf slowness I think.
> > > * Is there a special reason why the amsn commands (amsn, amsn-remote,
> > >   amsn-remote-CLI) are located in %{_datadir}/amsn and only linked to
> > >   %{_bindir}?  Why can't they just be installed directly into %{_bindir}?
> > Yes, the file perform some magic to see where they are located, and set some
> > variables accordingly to be able to find plugins etc.
> 
> Ok.  That makes sense.  You might suggest to upstream :) that they validate
> $program_dir before trying to resolve symlinks.  This would allow you to use sed
> in the spec file to set program_dir to %{_datadir}/%{name} and bypass the
> symlink dereferencing in the program.  As a result, the program should still
> work as it does now, but it would also work if amsn weren't installed in the
> data directory.

No changes were needed, after some looking at the code I could do it with one
sed line in the spec
> 
> A few other items:
> 
> MUSTFIX
> =======
> * Remove the extra " from the comment in the .desktop file
ok, did it in the spec and upstream as well
> * It turns out that /usr/share/amsn/README is needed at runtime for the
>   About box.  You can go ahead and remove it from the list of doc files
>   that are deleted during %install.  Do you know of any other doc files
>   that are used at runtime?
Only HELP, which you mention below and CREDITS, which I now don't delete as well
> 
> SHOULD
> ======
> * The Help -> Contents menu item doesn't seem very useful, since it
>   describes how to install and start amsn, which the user must have
>   already done if they can activate the menu.  I'd suggest to upstream that
>   they either remove this menu item, or replace the Help ->Contents text with
>   something more useful.
Ok, I told upstream :) and they told me it will be fixed before next release.
Good catch, I guess we never read the help documents ourselves..

http://amsn.hoentjen.eu/download/amsn.spec
http://amsn.hoentjen.eu/download/amsn-0.96-0.11.rc1.src.rpm

Comment 33 Wart 2006-06-20 17:53:16 UTC
(In reply to comment #32)
> Yes there are plans, but we want all packages created first, and synced to
> mirrors. In the meanwhile go to:
> http://prdownloads.sourceforge.net/amsn/amsn-0.96rc1.tar.bz2?download since what
> is in Source0 should work, but doesn't because of sf slowness I think.

This now works.  Maybe it took longer than expected to propogate to the mirrors?

Source matches upstream:
1b90fdbb0a51c7646f4d2e6b22f18711  amsn-0.96rc1.tar.bz2

All MUSTFIX items fixed.

APPROVED

Comment 34 Sander Hoentjen 2006-06-21 05:28:03 UTC
built for FC5 and devel


Note You need to log in before you can comment on or make changes to this bug.