Bug 225863 - Merge Review: gsl
Merge Review: gsl
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Patrice Dumas
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 13:58 EST by Nobody's working on this, feel free to take it
Modified: 2008-04-26 12:35 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-11-07 07:13:11 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
pertusus: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
patch to satisfy packaging guidelines (1.71 KB, patch)
2007-02-08 09:23 EST, José Matos
no flags Details | Diff
patch spec and .nousr patch to simplify multiarch changes (2.71 KB, patch)
2007-10-23 17:06 EDT, Patrice Dumas
no flags Details | Diff
spec file patch to place correctly the gsl-config man page (1.47 KB, patch)
2007-11-01 10:40 EDT, Patrice Dumas
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 13:58:58 EST
Fedora Merge Review: gsl

http://cvs.fedora.redhat.com/viewcvs/devel/gsl/
Initial Owner: varekova@redhat.com
Comment 1 José Matos 2007-02-08 09:21:58 EST
I have some suggestions regarding the spec file:

- Use the BuildRoot defined in the guidelines (this is suggested but in no way 
a blocker)

- for gsl-devel require gsl = %{version}-%{release} it is better to be 
safe :-)

 - make %{?_smp_mflags} - it works, I have tested it

 - in %install it is necessary to clean the buildroot and also %makeinstall 
can be replaced with make install DESTDIR=$RPM_BUILD_ROOT, again I have tested 
an it works.

  - finally the file attributes should be %defattr(-,root,root,-)

  There are other issues I would like to search, mainly related with the 
static libraries, but before going there I would like to hear you on these 
changes.

  One final note, a matter of style, I like more the description used in the 
spec file distributed in the tar ball, while the one present in the spec file 
is, maybe, too terse. Again this is a matter of taste so it is in no way a 
blocker.

  I will put in the next entry a patch with these changes...
Comment 2 José Matos 2007-02-08 09:23:45 EST
Created attachment 147653 [details]
patch to satisfy packaging guidelines
Comment 3 José Matos 2007-02-08 13:26:46 EST
Another small nit, gsl should not require /sbin/install-info, instead that 
requirement should pass to -devel:

Requires(post): /sbin/install-info
Requires(preun): /sbin/install-info

Is there any reason to distribute the .la version in gsl?

The case can be made for the .a's libraries, but the rules are very clear 
concerning the libtool archives: 
http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7
Comment 4 Patrice Dumas 2007-02-08 14:51:34 EST
static libs are very useful in math packages. The static libs
may be in a separate -static package, though.
Comment 5 José Matos 2007-03-08 08:11:59 EST
Note that a new version is available upstream, 1.9:

http://www.gnu.org/software/gsl/

I agree with Patrice that the static libs could go into a separate -static 
package, in line other packages that have been doing this, according to the 
packaging guidelines.
Comment 6 Ivana Varekova 2007-03-14 11:39:37 EDT
Fixed in gsl-1.8-3.fc7 (it is quite late to upgrade to 1.9 - so this version
will be in fc8). 
Comment 7 Patrice Dumas 2007-03-15 19:03:05 EDT
* URL seems to be
http://www.gnu.org/software/gsl/

and source should certainly come from gnu.

* missing dot at the end of %descriptions

* Why isn't the pkgconfig file distributed????

* the gsl-config script created on the fly certainly needs a comment.
And is strange anyway. 

It would be better, in my opinion either
  - to create a wrapper around pkg-config calls that recreates the 
    gsl-config output.
  - or leave gsl-config as is, if not using pkgconfig it is expected
    to have multilib stuff broken.

* if the code is still usefull, instead of
gslcsuffix=`echo "%{_libdir}" | sed s,/usr/,,`
I suggest using %_lib

* .la files are still there

Suggestions:
* remove .gz from install-info snippets

* use %defattr(-,root,root,-) instead of %defattr(-,root,root)
Comment 8 Orion Poplawski 2007-06-29 16:38:40 EDT
I'd like to see EPEL branches for this when the review is complete in order to
build gdl.  Review seems stalled though.  Any progress?
Comment 9 Ivana Varekova 2007-10-23 08:00:07 EDT
* URL seems to be
http://www.gnu.org/software/gsl/
and source should certainly come from gnu.
-> you are right, changed


* missing dot at the end of %descriptions
-> added

* Why isn't the pkgconfig file distributed????
-> right suggestion - gsl.pc is added

* the gsl-config script created on the fly certainly needs a comment.
And is strange anyway. 

It would be better, in my opinion either
  - to create a wrapper around pkg-config calls that recreates the 
    gsl-config output.
  - or leave gsl-config as is, if not using pkgconfig it is expected
    to have multilib stuff broken.
-> I can't imagine any think it there is not any better way - it is isomorphic
to use gsl.pc or to use the present solution. It is impossible to leave
gsl-config as it is because of that version causes multilib problem and it would
be impossible to install 64 and 32 versions.

* if the code is still useful, instead of
gslcsuffix=`echo "%{_libdir}" | sed s,/usr/,,`
I suggest using %_lib
-> changed, thanks

* .la files are still there
-> removed, thanks

Suggestions:
* remove .gz from install-info snippets

* use %defattr(-,root,root,-) instead of %defattr(-,root,root)
-> added
Comment 10 Ivana Varekova 2007-10-23 08:17:35 EDT
Thanks Patrice for your great work, please could you look to gsl-1.10-3.fc9.
Comment 11 Ivana Varekova 2007-10-23 08:25:27 EDT
Package Change Request
======================
Package Name: gsl
New Branches: EL-5
Comment 12 Toshio Ernie Kuratomi 2007-10-23 13:47:11 EDT
Before making cvs requests this needs to be approved.

Patrice, are you taking over the review from Jose?
Comment 13 Patrice Dumas 2007-10-23 17:03:53 EDT
I am commenting until I can accept it in which case
I'll take the review.

rpmlint says:
gsl.i386: W: file-not-utf8 /usr/share/doc/gsl-1.10/THANKS
gsl.i386: W: invalid-license BSD license

static lib should be in a separate -static subpackage.

Missing pkgconfig Requires.

/usr/share/aclocal/ is unowned.

Instead of the complicated patches for multilib issue, 
I'll attach a patch.
Comment 14 Patrice Dumas 2007-10-23 17:06:25 EDT
Created attachment 235541 [details]
patch spec and .nousr patch to simplify multiarch changes

Also keep timestamps.

You can remove the .nousr patch afterwards.
Comment 15 Ivana Varekova 2007-10-24 11:09:51 EDT
Thanks for comments. Incorporated to 1.10-4fc9, thanks for #14 it is a good idea.
Comment 16 Patrice Dumas 2007-10-24 11:46:36 EDT
It is cleaner if the static package 
Requires: %{name}-devel = %{versions}-%{release}

Also, for -devel there is a missing 
Requires: pkgconfig

/usr/share/aclocal/ is still unowned.
Comment 17 Ivana Varekova 2007-10-25 07:50:44 EDT
Thanks. Fixed in 1.10-5.fc9.
Comment 18 Patrice Dumas 2007-10-25 13:04:09 EDT
You should either have 
%{_datadir}/aclocal

or
%{_datadir}/aclocal/*
%dir %{_datadir}/aclocal

Thefollowing is not needed
rm -rf $RPM_BUILD_ROOT%{_sysconfdir}

And why do you remove the man pages?

I suggest replacing
rm -rf $RPM_BUILD_ROOT%{_libdir}/*.la
with
rm $RPM_BUILD_ROOT%{_libdir}/*.la

Only a suggestion, to keep the THANKS file timestamp, you can do
touch -r THANKS THANKS.aux
before mv.

Also it should be better to remove the .gz from install-info 
scriptlets since install-info can detect itself if .gz is needed. 
This is not a blocker, but I can't see why you shouldn't do it.
Comment 19 Orion Poplawski 2007-10-25 15:00:33 EDT
Instead of owning aclocal, the devel package should require automake.
Comment 20 Patrice Dumas 2007-10-25 15:25:10 EDT
(In reply to comment #19)
> Instead of owning aclocal, the devel package should require automake.

I disagree, this is up to the packager.
Comment 21 José Matos 2007-10-26 03:10:25 EDT
(In reply to comment #20)
> (In reply to comment #19)
> > Instead of owning aclocal, the devel package should require automake.
> 
> I disagree, this is up to the packager.

There is now a proposal for that directory to be owned by filesystem. This 
would simplify a lot of cases like this.
Comment 22 Ivana Varekova 2007-10-26 07:14:13 EDT
Thanks. Fixed in gsl-1.10-6.fc9. 

>You should either have 
>%{_datadir}/aclocal
>
>or
>%{_datadir}/aclocal/*
>%dir %{_datadir}/aclocal
Fixed.

> And why do you remove the man pages?
gsl man-pages are not in a good state, so I think it is better to remove them at
all (there could be use documentation instead)

> I suggest replacing
> rm -rf $RPM_BUILD_ROOT%{_libdir}/*.la
> with
> rm $RPM_BUILD_ROOT%{_libdir}/*.la
I prefer the present version.
 
> Only a suggestion, to keep the THANKS file timestamp, you can do
> touch -r THANKS THANKS.aux
> before mv.
I prefer the present version.

> Also it should be better to remove the .gz from install-info 
> scriptlets since install-info can detect itself if .gz is needed. 
> This is not a blocker, but I can't see why you shouldn't do it.
Done
Comment 23 Patrice Dumas 2007-10-27 06:22:50 EDT
(In reply to comment #22)

> > And why do you remove the man pages?
> gsl man-pages are not in a good state, so I think it is better to remove them at
> all (there could be use documentation instead)

Why aren't they in a good state? They seem perfect to me. I used the 
example in gsl-histogram and it looks like a Cauchy distribution.
And the gsl.3 page is just a reference to other doc.

> > I suggest replacing
> > rm -rf $RPM_BUILD_ROOT%{_libdir}/*.la
> > with
> > rm $RPM_BUILD_ROOT%{_libdir}/*.la
> I prefer the present version.

At least remove the -r, it doesn't make any sense.

> > Only a suggestion, to keep the THANKS file timestamp, you can do
> > touch -r THANKS THANKS.aux
> > before mv.
> I prefer the present version.

Ok. But you should be aware than in a in a multiarch setting,
a rpm -V will should wrong timestamp. In my opinion this is a 
sufficient reason to keep timestamps right for noarch files.
Comment 24 Ivana Varekova 2007-10-29 10:30:59 EDT
Hello Patrice,
thanks for your effort.
* The main problem I see with man-pages is that they are cover only short range
of gsl interface. I think doc documentation is sufficient.
* You are right I will remove -r option in the next build. 
But I think both these problems are not so big, so should you please grant
review flag to this package? Or is there any important issue which blocks the
review flag.
Comment 25 Patrice Dumas 2007-10-29 11:34:53 EDT
(In reply to comment #24)
> Hello Patrice,
> thanks for your effort.
> * The main problem I see with man-pages is that they are cover only short range
> of gsl interface. I think doc documentation is sufficient.

For the commands, the man pages are necessary. The gsl.3 man 
page, indeed is unuseful.

> * You are right I will remove -r option in the next build. 
> But I think both these problems are not so big, so should you please grant
> review flag to this package? Or is there any important issue which blocks the
> review flag.

Those problems are not big, the -r is not a blocker. Not shipping
useful man pages is a blocker, though. In any case there I don't see why 
we would need to hurry. The package is already in fedora, so what does it
change?

In any case if you want a more rapid reviewer, you can always try
to find one (in any case Jose is the reviewer so last word is for him).
Comment 26 José Matos 2007-10-29 11:46:03 EDT
(In reply to comment #25)
> Those problems are not big, the -r is not a blocker. Not shipping
> useful man pages is a blocker, though. In any case there I don't see why 
> we would need to hurry. The package is already in fedora, so what does it
> change?

  To add this package to EPEL. See comment #12.

> In any case if you want a more rapid reviewer, you can always try
> to find one (in any case Jose is the reviewer so last word is for him).

  I agree with your points regarding the man pages. BTW you can take this 
review, after all you made most of the review work. :-)
Comment 27 Patrice Dumas 2007-10-29 11:58:07 EDT
(In reply to comment #26)
> (In reply to comment #25)
> > Those problems are not big, the -r is not a blocker. Not shipping
> > useful man pages is a blocker, though. In any case there I don't see why 
> > we would need to hurry. The package is already in fedora, so what does it
> > change?
> 
>   To add this package to EPEL. See comment #12.

Can't this be added to EPEL without a finished merge review?
Packages in RHEL don't even go through a review!


>   I agree with your points regarding the man pages. BTW you can take this 
> review, after all you made most of the review work. :-)

Ok.
Comment 28 Ivana Varekova 2007-10-30 08:26:05 EDT
Thanks for your work.
I agree with the idea that the EPEL branch could be created before the review is
done, but I'd like to know what are the reasons not to give review flag to this
package.
Man pages added to gsl-1.10-7.fc9, -r option is removed there too. 
Is there any other problem or is this package OK now?
Comment 29 Patrice Dumas 2007-10-30 16:45:57 EDT
There is now an issue in the %files section that leads to:
gsl.i386: E: standard-dir-owned-by-package /usr/share/man/man1
gsl.i386: E: standard-dir-owned-by-package /usr/share/man/man3

Moreover the man3 man pages are in general in the devel
packages. I suggest, something along, in the main package:

%{_mandir}/man1/*.1*

And in the devel package

%{_mandir}/man3/*.3*
Comment 30 Ivana Varekova 2007-11-01 09:53:37 EDT
You are right, thanks (fixed in gsl-1.10-8.fc9).
Comment 31 Patrice Dumas 2007-11-01 10:40:59 EDT
Created attachment 245731 [details]
spec file patch to place correctly the gsl-config man page

Little cleanup patch:
- put the gsl-config man page in the devel subpackage
- correct devel description, static libs are not there
- keep the THANKS file timestamp
Comment 32 Patrice Dumas 2007-11-01 10:42:45 EDT
The source file timestamps are not kept:
-rw-rw-r-- 1 dumas dumas 2842422 sep 14 16:01 gsl-1.10.tar.gz
-rw-rw-r-- 1 dumas dumas 2842419 sep 19 09:51 gsl-1.10.tar.gz-orig

You can use wget -N or spectool -g to get sources with timestamp
kept.

More worrisome, unless I did a mistake, the source file isn't the one
from upstream:
$ md5sum gsl-1.10.tar.gz*
d67be4f2e5560d6cf907e18a428becdc  gsl-1.10.tar.gz
faf5e178855ec952fe745a03d815da1d  gsl-1.10.tar.gz-orig
Comment 33 Ivana Varekova 2007-11-02 05:05:18 EDT
Ops, nice catch(#32) - fixed and I apply the changes suggested in 31 too. Thanks
for your excellent work. Fixed in gsl-1.10-9.fc9.
Comment 34 Ivana Varekova 2007-11-06 07:36:08 EST
Package Change Request
======================
Package Name: gsl
New Branches: EL-5
Comment 35 Kevin Fenzi 2007-11-06 14:23:03 EST
cvs done.
Comment 36 Patrice Dumas 2007-11-06 15:25:11 EST
Now source match upstream

d67be4f2e5560d6cf907e18a428becdc  gsl-1.10.tar.gz

rpmlint is silent.

I haven't checked the file license, but which one
is GPLv3 and not GPLv3+? Anyway not a blocker since 
GPLv3 is stricter than GPLv3+.

APPROVED. I assign to me as said above.
Comment 37 Ivana Varekova 2007-11-07 07:13:11 EST
Thanks Patrice. I will change the license tag before the next build. I'm closing
this bug.
Comment 38 Richard W.M. Jones 2008-04-25 15:19:07 EDT
[Note: This is to support ocaml-gsl, requested for EPEL-4, see bug 435983]

Package Change Request
======================
Package Name: gsl
New Branches: EL-4
Comment 39 Kevin Fenzi 2008-04-26 12:35:40 EDT
cvs done.

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