This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 466737 - Review Request: matio - Library for reading/writing Matlab MAT files
Review Request: matio - Library for reading/writing Matlab MAT files
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
:
Depends On:
Blocks: RussianFedoraRemix 472639
  Show dependency treegraph
 
Reported: 2008-10-13 06:53 EDT by Nicolas Chauvet (kwizart)
Modified: 2009-05-26 18:20 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-05-26 18:20:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
hdegoede: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Nicolas Chauvet (kwizart) 2008-10-13 06:53:45 EDT
Spec URL: http://kwizart.fedorapeople.org/SPECS/matio.spec
SRPM URL: http://kwizart.fedorapeople.org/SRPMS/matio-1.3.3-2.fc8.kwizart.src.rpm
Description: Library for reading/writing Matlab MAT files

koji build on Fedora Rawhide 
http://koji.fedoraproject.org/koji/taskinfo?taskID=877064
Comment 1 Jason Tibbitts 2008-10-16 00:01:38 EDT
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.
Comment 2 Nicolas Chauvet (kwizart) 2008-10-16 10:31:06 EDT
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.
Comment 3 Jason Tibbitts 2008-10-16 15:39:03 EDT
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.
Comment 4 Jason Tibbitts 2008-10-17 15:40:53 EDT
I've decided not to review this package.
Comment 5 Susi Lehtola 2008-10-26 09:37:09 EDT
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.
Comment 6 Nicolas Chauvet (kwizart) 2008-10-27 05:09:55 EDT
(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
Comment 7 Susi Lehtola 2008-10-27 05:34:15 EDT
(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.
Comment 8 Nicolas Chauvet (kwizart) 2008-10-27 10:26:45 EDT
You need to add
%__arch_install_post   /usr/lib/rpm/check-rpaths   /usr/lib/rpm/check-buildroot
in ~/.rpmmacros
Comment 9 Susi Lehtola 2008-10-27 10:33:23 EDT
No, rpmbuild does it automatically. At least in F9.
Comment 10 Nicolas Chauvet (kwizart) 2008-10-27 14:04:36 EDT
so what output rpmlint on installed files ?
Comment 11 Susi Lehtola 2008-10-27 14:45:19 EDT
Clean.
Comment 12 Nicolas Chauvet (kwizart) 2008-10-27 16:47:36 EDT
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
Comment 13 Susi Lehtola 2008-10-28 04:00:17 EDT
(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.
Comment 14 Chitlesh GOORAH 2008-11-19 15:07:37 EST
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.
Comment 15 Chitlesh GOORAH 2008-11-19 15:59:35 EST
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
Comment 16 Chitlesh GOORAH 2008-11-19 16:16:31 EST
Please use your real name in the changelog.
Comment 17 Chitlesh GOORAH 2008-12-01 13:19:34 EST
(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 ?
Comment 18 Jason Tibbitts 2009-03-30 21:31:37 EDT
It's been over five months since the last comment from the submitter.  I think it's time this was closed.
Comment 19 Nicolas Chauvet (kwizart) 2009-03-31 19:11:18 EDT
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.
Comment 20 Jason Tibbitts 2009-03-31 19:13:30 EDT
So you won't even address comments made until someone sets the fedora-review flag?  What's the point in that?
Comment 21 Susi Lehtola 2009-04-24 02:14:53 EDT
ping?
Comment 22 Nicolas Chauvet (kwizart) 2009-04-24 19:07:26 EDT
yep, I'm still here.
Comment 23 Hans de Goede 2009-05-22 14:17:44 EDT
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 ?
Comment 24 Nicolas Chauvet (kwizart) 2009-05-22 21:08:35 EDT
(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
Comment 25 Hans de Goede 2009-05-23 03:57:07 EDT
(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!
Comment 26 Nicolas Chauvet (kwizart) 2009-05-23 04:07:35 EDT
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
Comment 27 Jason Tibbitts 2009-05-26 18:02:47 EDT
CVS done.
Comment 28 Nicolas Chauvet (kwizart) 2009-05-26 18:20:39 EDT
Thx for the review

Note You need to log in before you can comment on or make changes to this bug.