Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Review Request: xmds - the eXtensible Multi-Dimensional Simulator https://bugzilla.redhat.com/show_bug.cgi?id=326421 ------- Additional Comments From pertusus@xxxxxxx 2007-10-16 16:52 EST ------- (In reply to comment #2) > (In reply to comment #1) > > The source should be an url to the upstream source. > > See: > > http://fedoraproject.org/wiki/Packaging/SourceURL > > I've updated this in the .spec and uploaded the files again: > > Spec URL: http://downloads.sourceforge.net/xmds/xmds.spec > SRPM URL: http://downloads.sourceforge.net/xmds/xmds-1.6-3.src.rpm This is not right. You should increment the release number of your .src.rpm each time you post a fixed version (except maybe in very specific cases). The meaning of the release is here: http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-5ea39bbc33cf351b41b51325ac3527eff4c58dac In general there is a difference between the 'upstream release' and the package release. there may be many package release for one upstream release. In fact it seems that you should release xcmds-1.6.3 Having Source: http://downloads.sourceforge.net/xmds/%{name}-%{version}-%{release}.tar.gz is necessarily wrong, since the %{release} should never appear in upstream sources. Part of the confusion comes from the fact that you are both packager and upstream. This is a situation that I dislike, I prefer when both roles are clearly separated. > > loadxsil.m shouldn't be in %_bindir, it may better be in > > %doc since there is no specific directory for matlab scripts > > currently in fedora. > > I've added a patch which stops loadxsil.m from being installed in %_bindir and > added code to put it into %_docdir Your patch is wrong since it modifies Makefile.am only, it should also modify Makefile.in. Also it should not be removed, but noinst, like (if I recall well) noinst_SCRIPTS = loadxsil.m Also (this is just a suggestion, contrary to most of what I say otherwise in the review which are issues blocking the inclusion of the package), I suggest the following format for patch: Patch0: xmds-1.6-loadxsil_nobin.patch and %patch0 -p0 -b .loadxsil_nobin (as a side note, using -b loadxsil.m is a bit.... strange...) > > Docs are missing. Without doc, the package is not very useful. > > There is an example directory, it should be shipped in %doc. > > Also the manual would be a must, if it is covered by a free > > documentation license. > > They're actually in a separate part of the repository. Should they be a > separate package? Say xmds-doc? If you want, but, in that case I think that the main package should depend on the doc package. Doing that, in general is wrong, but this is a special package, see below. > I've added the example xmds files, but I'm not 100% sure if I've done things > correctly. Indeed it is not correct. You should make use of %doc. rpmbuild will take care to install correctly all the files and directories specified as %doc. In your case, should be along %doc examples/ source/loadxsil.m README AUTHORS NEWS COPYING > Actually the output should work fine with Octave which is properly free. In the > current svn version of xmds we've got R and Gnuplot output working as well, > however, Octave should work "out of the box" with this version of xmds. Ok. Worth saying somewhere in the description or the like. > > Unless I am wrong, xsil2graphics and loadxsil are useful by > > themselves, maybe they could be in a subpackage, since > > as far as I can tell from a quick browsing, there aren't many > > other xsil tools. > > Atm they only make sense with xmds. AFAIK xsil is a format which isn't likely > to stick around much longer, so having separately packaged tools isn't going to > be worth the effort, I think. Right. > > I suggest using %dist since this is a binary package. Also > > why use a release of 3 in the submission, why not begin with > > 1? > > That's because this is xmds version 1.6 revision 3. Am I supposed to do this > another way? Is the revision number the number of the xmds.spec file or something? Indeed. You are using it as a minor version number, but it is wrong the minor version number should be in the version of your upstream archive. > > It seems to me that a requires on gcc-c++ would be in > > order, since it is called during model compilation. Same > > for fftw2-devel. > > Added these dependencies to the new .spec file. After rereading the configure.in, it seems that fftw3 can be used instead, it would be better to use fftw3. > > Is libxmds.a meant to be used separately from xmds? > > Um, no. Should we be putting this somewhere else? Well, this package is very different from most of the packages in fedora. Indeed it is a code generator, compiler driver. So it is more like a preprocessor or a compiler than like a application or a library. For libraries there is a systematic split of the package in 2, the shared library in the main package, which is installed when something linked against the library is installed, and the -devel subpackage which holds the .so symbolic link, the header files, the api description. But xmds doesn't fit well in this scheme, the library is more like an internal library. Similarly the package is useless without documentation, like a library without api description. In my opinion it should be treated like a devel package, like binutils, or cpp. You should also read http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7 although I think that it doesn't apply here. 2 suggestions: * use wildcards for man pages to catch different or no compression: %{_mandir}/man1/xmds.1* * use %defattr(-,root,root,-) instead of %defattr(-,root,root). -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug, or are watching someone who is. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review