Bug 225863

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

Created attachment 147653 [details]
patch to satisfy packaging guidelines

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

static libs are very useful in math packages. The static libs
may be in a separate -static package, though.

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.

Fixed in gsl-1.8-3.fc7 (it is quite late to upgrade to 1.9 - so this version
will be in fc8). 

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

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?

* 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

Thanks Patrice for your great work, please could you look to gsl-1.10-3.fc9.

Package Change Request
======================
Package Name: gsl
New Branches: EL-5

Before making cvs requests this needs to be approved.

Patrice, are you taking over the review from Jose?

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.

Created attachment 235541 [details]
patch spec and .nousr patch to simplify multiarch changes

Also keep timestamps.

You can remove the .nousr patch afterwards.

Thanks for comments. Incorporated to 1.10-4fc9, thanks for #14 it is a good idea.

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.

Thanks. Fixed in 1.10-5.fc9.

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.

Instead of owning aclocal, the devel package should require automake.

(In reply to comment #19)
> Instead of owning aclocal, the devel package should require automake.

I disagree, this is up to the packager.

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

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

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

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.

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

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

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

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?

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*

You are right, thanks (fixed in gsl-1.10-8.fc9).

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

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

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.

Package Change Request
======================
Package Name: gsl
New Branches: EL-5

cvs done.

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.

Thanks Patrice. I will change the license tag before the next build. I'm closing
this bug.

[Note: This is to support ocaml-gsl, requested for EPEL-4, see bug 435983]

Package Change Request
======================
Package Name: gsl
New Branches: EL-4

cvs done.