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

[bugzilla at redhat.com]

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 paultcochrane at gmail.com  2007-10-17 04:37 EST -------
(In reply to comment #3)
> (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 in the urls numbers happen to be the same this time as it's my third go at
this :-)

> 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.

Yes, it can be somewhat confusing.  I'm hoping that the next xmds release will
be easier to package.  Hopefully we can merge some of your comments into the
trunk and so make both of our lives easier.

> > > 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

Corrected.  I've also added a patch for Makefile.in.  I *believe* this patch
works, as it is a bit of a hack; unfortunately the last person to release xmds
had a much older version of the autotools package than I do and consequently the
changes between the different autogenerated Makefile.in's makes the differences
not patch cleanly.  The patch I've supplied should work I hope.  Like I
mentioned above, hopefully the next release of xmds will be easier to package.

> 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...)

this is a consequence of me not knowing completely what I'm doing :-(  Anyway,
this issue should be corrected in this update.

> > > 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 pdf docs (which is all that gets released now; we used to release
the LaTeX sources as well...  Maybe it'd be better to merge the two back
together again).

> > 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

Corrected in this update.

> > 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.

I've added a small change mentioning Octave in the README file.  I hope this is
sufficient.  In the current xmds trunk there is more reference to Octave, but
for this package I don't know if the extra effort is worthwhile.  Maybe I'm just
hoping that xmds-1.6-4 will get released sometime soon....

> > > 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.

This means, unfortunately, that one can't use MPI (fftw2 handles MPI, but not
fftw3, although it's coming).  Actually, this leads me to a point I'd not
thought about until just now.  MPI is optional in xmds, but it needs to be
turned on at configure time.  How does one handle this in a package such as rpm?
 Should two packages be maintained?  One with MPI support and one without?

> > > 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.

Does this mean that I need to add something like:

%package devel
Provides: xmds-static = %{version}-%{release}

to the spec file?

> You should also read
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7
> although I think that it doesn't apply here.

Read this, and I agree, I don't think it applies in this (rather odd) case.  (to
be honest, I don't know why we use a library at all, that was added long after
my time, I've just only recently become involved in a more maintenance capacity).

> 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).

Both added in this update.

Again:  many thanks for your feedback!!!



-- 
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.