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.
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.
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
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.
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.
Oh nevermind my comment about tcltls, I didn't notice the tcltls package you also submitted.
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.
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
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.
new specfile and srpm: http://amsn.hoentjen.eu/download/amsn.spec http://amsn.hoentjen.eu/download/amsn-0.96-0.4.20060425cvs.src.rpm
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
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.
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
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"
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.
(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.
(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
(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.
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?
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.
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
Sorry for the delay in the review. I'll do a full review this weekend.
updated to 0.96 rc1 http://amsn.hoentjen.eu/download/amsn.spec http://amsn.hoentjen.eu/download/amsn-0.96-0.8.rc1.src.rpm
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.
> 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.
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?
(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.
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
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?
(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.
(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
(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.
(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
(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
built for FC5 and devel