Bug 326421

The source should be an url to the upstream source.
See:
http://fedoraproject.org/wiki/Packaging/SourceURL

You should also certainly have a look at the guidelines
if you haven't already:
http://fedoraproject.org/wiki/Packaging/Guidelines

A buildrequires for fftw is missing. It seems to be fftw2-devel,
more precisely.

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.

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.

scilab and matlab are not part of fedora. But unless I am 
wrong, the .dat created by xsil2graphics -s are simple ascii
output file that may be used with any plotting program? Maybe
this could be said somewhere?

In summary it is unneeded to repeat the package name.

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.

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?

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.

Is libxmds.a meant to be used separately from xmds?

Hi Patrice,

Thanks for reviewing this submission!  :-)

(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

> A buildrequires for fftw is missing. It seems to be fftw2-devel,
> more precisely.

Added.

> 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

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

I've added the example xmds files, but I'm not 100% sure if I've done things
correctly.

> scilab and matlab are not part of fedora. But unless I am 
> wrong, the .dat created by xsil2graphics -s are simple ascii
> output file that may be used with any plotting program? Maybe
> this could be said somewhere?

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.

> In summary it is unneeded to repeat the package name.

Oops, I just realised I missed this.  The package still needs more review, so
I'll fix this with the next set of problems which I'm sure about to turn up...

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

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

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

> Is libxmds.a meant to be used separately from xmds?

Um, no.  Should we be putting this somewhere else?

Thanks again for your help!

Paul

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

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

(In reply to comment #4)

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

Sometimes it happens that upstream sources cannot be packaged as-is
and that the reviewing process points out issues in upstream
and that a new version is needed for proper packaging.

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

To mitigate this issue, you can use older autotool versions. There
are many autoconf and automake version in fedora, and using the
one that matches helps doing patches that can be reviewed.

> The patch I've supplied should work I hope.  Like I
> mentioned above, hopefully the next release of xmds will be easier to package.

I am beginning to think that a new release will be a requirement,
but it is not completly obvious either.


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

What is the license of the pdf?

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

Hoping that it will be named xmds-1.6.4 ....

 
> This means, unfortunately, that one can't use MPI (fftw2 handles MPI, but not
> fftw3, although it's coming).  

Right, this is a good reason to keep fftw2.

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

In general it is best to have all the possible features. But
in the case of mpi, this is really 2 different programs, so there
is a need for a separated subpackage. You can have a look at 
paraview which has a separated paraview-mpi subpackage. The -mpi 
subpackage may be added later.


> Does this mean that I need to add something like:
> 
> %package devel
> Provides: xmds-static = %{version}-%{release}
> 
> to the spec file?

No, the library should be in the main package. What would be better,
however, would be to install the library in a subdirectory, such that
it is not in the linker path. So install it in
%{_libdir}/xmds/lib....a

and modify apropriately the link command.

Similarly, the include files should be in a %{_includedir} 
subdirectory.


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

Having code generated and an internal library seems completely
right to me. There is another example in fedora, this is what 
gcc does.

You should also read
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Hi Patrice,

sorry for the long delay in replying to your most recent message, I've only just
now got the tuits to work on xmds again.

The current version of xmds is 1.6.5 and I've merged many of your comments into
the xmds trunk in order to reduce the packaging overhead (it's still not perfect
though...).

(In reply to comment #5)
> (In reply to comment #4)
> 
> > 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).
> 
> What is the license of the pdf?

The license of the pdf is the same as the software itself: GPL, however, in the
rpms I'm linking to now, the pdf isn't included as it wasn't that easy to add
the pdf to the rpm easily *and* have the other documentation installed via the
Makefiles etc. (the directory got removed and newly created, hence all the files
which had been installed were lost, and only the pdf remained).  How would an
xmds-doc package be as a replacement?  One could then have the source rpms for
the tex files etc if anyone's keen.

We've also updated the naming scheme of xmds, so the current version doesn't
have '-' in it anymore :-)

The library and header placement has also been improved since the last version
you would have seen, so I hope the rpms meet the requirements and that xmds can
soon become a part of Fedora.

The rpms are:

Spec URL: http://downloads.sourceforge.net/xmds/xmds-1.6.5.spec
SRPM URL: http://downloads.sourceforge.net/xmds/xmds-1.6.5.src.rpm

The spec file has the version in it to keep the sourceforge site happy, this is
*not* how it appears in the xmds repository.

Thanks, as always, for your help!

The src.rpm is not found?

Regarding the version in spec file name, I understand it and it is right as long
as the spec file in the src.rpm hasn't the version. 

Reading the spec file, it is indeed possible to use the provided install the
documentation files should have %doc in front.

I suggest using
%{_mandir}/man1/xmds.1*
instead of
%{_mandir}/man1/xmds.1.*

For the documentation, in fact if the .pdf is covered by the GPL, then you
should also provide the .tex somewhere, otherwise it cannot be covered by the
GPL. It can also be provided separately as a pdf file, but the source should be
available.

this review is stalled and response is required from reporter ASAP.

(In reply to comment #8)
> this review is stalled and response is required from reporter ASAP.

I have unfortunately had no tuits since the last time I sent an update to this review.  I can attempt to get an updated spec file and rpms ready hopefully by the end of this week.  Is this ok?

Thanks,

Paul

ping, it is again a month
I will close this if not updated within a week. In that you can you can open a bug again when you are free.

Thanks,

(In reply to comment #10)
> ping, it is again a month
> I will close this if not updated within a week. In that you can you can open a
> bug again when you are free.

I think this is a good idea.  Unfortunately, I don't have any more time at present to work on this project, so closing the bug and then reopening it again if I find the time is a good idea.  Thanks heaps for your time!  

Paul

Okay, feel free to reopen it as a new review request when you are ready.