[Bug 326421] Review Request: xmds - the eXtensible Multi-Dimensional Simulator

Tue Oct 16 20:52:43 UTC 2007

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 at free.fr  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.