Bug 466737

A few comments:

Why would you need to define _default_patch_fuzz for a new package?  It seems that at least the initial package should have patches which apply cleanly.

matio-1.3.3-fortanpath.patch seems to lack an 'r'.

There are a few commented-out bits I don't quite understand:

  #sed -i.fortranpath2 -e 's|src/fortran/matio_t.inc|src/matio_t.inc|' 
    configure.ac configure
Is this made unnecessary by the .fortranpath2 patch?

  #To disable rpath
  #./bootstrap
Not sure why this is in there.

What's the doxygen bit for?  Did you just not want to ship the pdf file?  (Nothing would require doing that, and if you're not going to ship it then it makes sense not to generate it, but a comment about why might help.)

The need to move the source files (and the patches you have to carry to support that) just because rpm doesn't generate debuginfo properly is troubling.  It's certainly OK if you want to do that, but I wonder if the problem is fully understood.  Is there a bug open against rpm for this issue?

I'll finish this review up tomorrow.

Patch_fuzz: I'm on Fedora-8. So, patches are generated that way with gendiff. Also, I Plan to have matio in F-9 (maybe F-8). The patches have been sent upstream anyway. (still not sure they have been received).

Missing r: True, typo.

#sed -i.fortranpath2 ... : yes, matio-1.3.3-fortranpath2.patch prevent to  dynamic patch configure and configure.ac.

rpath: there is differents ways to disable rpath related to the reason why they are there. Here is is because of patched autotools. our autotools doesn't create rpathes, so it is easier to avoid with patching libtool.

docs and the pdf: The comment leave in the %install section. pdf is disabled because it duplicates with doxygen. it doesn't seems interesting to have docs twices.

The problem with the path for the fortran files is that they are listed in the src/Makefile.am whereas present in the src/fortran/ directory. There is two way to fix this either move back the src/fortran/* to src/* or to list the fortran files in their own directory (src/fortran/Makefile.am  using subdir += fortran in src/Makefile.am)
This problem has nothing to do with rpm but with "autotools preference" 
At least, that's what I expect.

I'm not sure I understand about the patch fuzz; are you saying that gendiff generates patches which need fuzz to apply or that the F8-generated patches simply need fuzz to apply on rawhide?  Either of those cases seems somewhat bizarre to me.

I understand that there are muliple ways to disable rpath; my concern is thatthe  sed call shouldn't be there at all if it's commented out.  At least not without some explanatory comment about it.

I understand that it doesn't seem interesting include both PDF and HTML documentation; my point is that I can't tell from the comments in your spec that you have code to accomplish that.  "#Fake the pdf creation" isn't sufficient elaboration.

> The problem with the path for the fortran files is that they are listed in the
> src/Makefile.am whereas present in the src/fortran/ directory. 

Well, your comments indicate that the problem with this is improper debuginfo generation.  Is that the case?

All I'm asking is that you comment your spec such that I can understand it.  This spec has several different hacks in it, yet it doesn't describe why they're needed clearly enough that someone else can look at it and understand why the hacks are all there.

I've decided not to review this package.

1. Why don't you have just one patch for the fortran-related stuff? Remove commented sed line.

2. To my understanding you don't need the zlib patch, since you require zlib >= 1.2.3 (the bug is only present in zlib 1.2.2). Remove patch0.

3. Remove all rpath related stuff, matio builds fine without them.

4. Please don't delete man files - rename them all to begin with, say matio-.

5. Add requires:
%if 0%{?fedora} > 8
BuildRequires:  texlive-latex
%else
BuildRequires:  tetex-latex
%endif

and get rid of the doxygen stuff in the spec file.

6. Move PDF to builddir and add it in %doc.

(In reply to comment #5)
> 1. Why don't you have just one patch for the fortran-related stuff? Remove
> commented sed line.
The first is the patch sent upstream. The second is the patch against configure which ends to be specific with the sources version used.
> 2. To my understanding you don't need the zlib patch, since you require zlib >=
> 1.2.3 (the bug is only present in zlib 1.2.2). Remove patch0.
wrong, there is nothing common with the zlib 1.2.2 bug and the undefined-non-weak-symbol related to a missing -lz at link time.
> 3. Remove all rpath related stuff, matio builds fine without them.
wrong, not in x86_64 at least. The cause is the autotools version used to generate the source archive.
> 4-5-6. man doxygen and pdf is the same content. Thus to provide the 3

(In reply to comment #6)
> > 3. Remove all rpath related stuff, matio builds fine without them.
> wrong, not in x86_64 at least. The cause is the autotools version used to
> generate the source archive.

Actually, I built matio-1.3.3 in x86_64 with and had no trouble with rpaths. Didn't patch anything, though.

You need to add
%__arch_install_post   /usr/lib/rpm/check-rpaths   /usr/lib/rpm/check-buildroot
in ~/.rpmmacros

No, rpmbuild does it automatically. At least in F9.

so what output rpmlint on installed files ?

Clean.

Well actually i was wrong. the rpath is in the test binary packages.
So there is many ways to remove rpathes, it depends on the cause. 

In the F-8 x86 _64 case, there is a rpath defined which is removed if the configure  script is regenerated or libtool patched. so either one or the others options works, and both fixes are valid (even if they is no rpath after all).

There is a typo on the group for the test package, i plan to change it to Development/Tools

(In reply to comment #12)
> Well actually i was wrong. the rpath is in the test binary packages.
> So there is many ways to remove rpathes, it depends on the cause. 

OK. Actually I hadn't built the test binaries, only the library. So you are right, you need to have the libtool patch.

> There is a typo on the group for the test package, i plan to change it to
> Development/Tools

OK. I tested the build without the zbib patch, and I don't any errors from rpmlint you mentioned (using zlib-1.2.3-18.fc9.x86_64).

I still don't think you need to do any special doxygen / pdf / html -related stuff, please remove it and add latex to BRs as described in comment #5.

Also, you shouldn't change the timestamps of the HTML documentation.

hmm, I don't get it. Why is matio.h shipped by -devel ? Please, give me a test case, A USER can write and read .mat files without matio.h in %name. I believe matio is an exception to the rule and matio.h should be shipped by %name.

Can you update the spec please with respect to the above comments ?

I support Jason Tibbitts's comments. Please do comment your spec properly and removed useless commented lines that confuse the reviewer.

Also, in the description, change "libmatio" to "matio" (first word)

For:
>> # According to the README - zlib 1.2.2 is possible but require a patch
>> BuildRequires:  zlib-devel >= 1.2.3
Even F-7 has zlib-devel 1.2.3. Drop the version and the comment

Is it useful to add doxygen commands in the spec file ? I understand the BuildRequires: doxygen. It is needed for the compilation, however extra doxygen commands in the spec file ?


remove this %define _default_patch_fuzz 2, the package builds fine without it on F-8. Remember early next year, the F-8 will not be supported.

Did you mock matio?
sh: latex: command not found
Problems running latex. Check your installation or look for typos in _formulas.tex and check _formulas.log!
sh: dvips: command not found
Problems running dvips. Check your installation!
cd latex;.././format_api.sh;.././textopdf.sh
.././textopdf.sh: line 3: pdflatex: command not found
.././textopdf.sh: line 4: makeindex: command not found
.././textopdf.sh: line 5: pdflatex: command not found

Please use your real name in the changelog.

(In reply to comment #14)
> hmm, I don't get it. Why is matio.h shipped by -devel ? Please, give me a test
> case, A USER can write and read .mat files without matio.h in %name. I believe
> matio is an exception to the rule and matio.h should be shipped by %name.

discard this comment #14.

Any update from your side ?

It's been over five months since the last comment from the submitter.  I think it's time this was closed.

There is no reviewer at this time, so there I've no one to talk to.

The spec if readable as it is:
-doc are disabled because they are rundantant. (That's written within the spec file already).
-Patching libtool remains needed as prevention to rpath, they is no need to disable.
-zlib 1.2.3 remains documented as future introduction to EL-5 branch. (strangely, some wants more spec's comments, others want important comment's removed)

I agree to change some trivial issue with default_patch_fuzz removed, libmatio > matio in desc, and typo in Group of the test sub-package.

Futhermore one patch was adopted upstream.
http://sourceforge.net/tracker/?func=detail&aid=2507202&group_id=176643&atid=878064
Other work is awaited because of this review is stalled.

If someone want to review this package, I will move to action, until then, please review the pre-reviewer's comments.

So you won't even address comments made until someone sets the fedora-review flag?  What's the point in that?

ping?

yep, I'm still here.

As discussed by mail, reviewing this one in return for you reviewing bug 502189.

Full review done, here are the must and should fix's I found:

Must Fix:
---------
* Remove the "%define _default_patch_fuzz 2", your patches have no fuzz, so
  its not needed (I tested on F-11)
* For the test package, the Group tag is wrong (spelling error,
  currently you have: "Develdment/Libraries"
* matio.src:18: W: non-break-space line 18

Should Fix:
-----------
* Rename the matio-1.3.3-fortanpath.patch to
  matio-1.3.3-fortranpath.patch (so add the missing r)
* libmatio -> matio in %description
* Remove these 2 commented lines from %setup please, as you've already
  solved this in another way in %build, they only confuse the reader
  (atleast they confused me):
  #To disable rpath
  #./bootstrap
* Consider renaming the manpages to mathio-<foo> and installing them
* Is it really useful to put the test files in a subpacke, wouldn't it be better
  to run them as %check and not put them in an rpm at all ?

(In reply to comment #23)
...
> Should Fix:
> -----------
...
> * Consider renaming the manpages to mathio-<foo> and installing them
Actually, thoses manpages are removed from matio svn trunk. My view is that they provide the same information as doxygen (since both pdf and manpages are generated from doxygen). Keeping html doxygen is lightweight and
> * Is it really useful to put the test files in a subpacke, wouldn't it be
> better
>   to run them as %check and not put them in an rpm at all ?  
Nice suggest. I've done that since the test_* file are likely not useful beyond build time.

All others point are fixed.

Spec URL: http://kwizart.fedorapeople.org/SPECS/matio.spec
SRPM URL: http://kwizart.fedorapeople.org/SRPMS/matio-1.3.3-3.fc11.src.rpm
Description: Library for reading/writing Matlab MAT files

(In reply to comment #24)
> (In reply to comment #23)
> ...
> > Should Fix:
> > -----------
> ...
> > * Consider renaming the manpages to mathio-<foo> and installing them
> Actually, thoses manpages are removed from matio svn trunk. My view is that
> they provide the same information as doxygen (since both pdf and manpages are
> generated from doxygen). Keeping html doxygen is lightweight and

Ok.

> > * Is it really useful to put the test files in a subpacke, wouldn't it be
> > better
> >   to run them as %check and not put them in an rpm at all ?  
> Nice suggest. I've done that since the test_* file are likely not useful beyond
> build time.
>

Much better :)
 
> All others point are fixed.
> 

Ack, looks good now, APPROVED!

New Package CVS Request
=======================
Package Name: matio
Short Description: Library for reading/writing Matlab MAT files  
Owners: kwizart
Branches: F-11 F-10 F-9 EL-5
Cvsextras Commits: yes

CVS done.

Thx for the review