Bug 492164 - Review Request: healpix - Hierarchical Equal Area isoLatitude Pixelization of a sphere
Review Request: healpix - Hierarchical Equal Area isoLatitude Pixelization of...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Susi Lehtola
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 493957
  Show dependency treegraph
 
Reported: 2009-03-25 13:30 EDT by Lubomir Rintel
Modified: 2009-04-07 11:50 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-07 03:17:02 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
susi.lehtola: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Patch to spec file that adds Fortran stuff (3.43 KB, patch)
2009-03-26 04:49 EDT, Susi Lehtola
no flags Details | Diff

  None (edit)
Description Lubomir Rintel 2009-03-25 13:30:01 EDT
SRPM: http://v3.sk/~lkundrak/SRPMS/healpix-2.11c-1.fc11.src.rpm
SPEC: http://v3.sk/~lkundrak/SPECS/healpix.spec

Description:

HEALPix is an acronym for Hierarchical Equal Area isoLatitude Pixelization
of a sphere. As suggested in the name, this pixelization produces a
subdivision of a spherical surface in which each pixel covers the same
surface area as every other pixel.
Comment 1 Susi Lehtola 2009-03-25 14:20:18 EDT
What about the Fortran stuff? That's important as well.
Comment 2 Lubomir Rintel 2009-03-25 14:29:17 EDT
(In reply to comment #1)
> What about the Fortran stuff? That's important as well.  

And Java, and IDL, etc. HEALPix contains a half dozen of bindings. I'm packaging C bindings, because I need C bindings, placing them in a separate subpackage, so that anyone who needs the other bindings can easily and consistently add them.

Do you need the Fortran bindings? I'll gladly accept a patch for the package and provide help creating it if it is needed.
Comment 3 Susi Lehtola 2009-03-26 04:49:08 EDT
Created attachment 336762 [details]
Patch to spec file that adds Fortran stuff
Comment 4 Susi Lehtola 2009-03-26 04:50:07 EDT
Okay, got the Fortran stuff to build after a few hours of work. Feel free to rearrange the header files, libraries and subpackage names as you see fit, as this was just the first draft :)
Comment 5 Susi Lehtola 2009-03-26 04:52:04 EDT
I'll do the review once you have updated the package to provide the Fortran stuff too.
Comment 6 Lubomir Rintel 2009-04-03 09:09:47 EDT
Thanks, Jussi. I've incorporated your changes, and adjusted it into using a shared library instead of static one.

Here you are:

SPEC: http://v3.sk/~lkundrak/SPECS/healpix.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/healpix-2.11c-3.el5.src.rpm
Comment 7 Susi Lehtola 2009-04-03 12:00:31 EDT
rpmlint output:
chealpix.x86_64: W: no-soname /usr/lib64/libchealpix.so
chealpix.x86_64: W: shared-lib-calls-exit /usr/lib64/libchealpix.so exit@GLIBC_2.2.5
chealpix-devel.x86_64: W: no-documentation
healpix-fortran.x86_64: W: no-soname /usr/lib64/libgif.so
healpix-fortran.x86_64: W: no-soname /usr/lib64/libhealpix.so
healpix-fortran-devel.x86_64: W: no-documentation
6 packages and 0 specfiles checked; 0 errors, 6 warnings.

- Did you send the shared library patch upstream? IIRC some package sponsors have frowned on Fedora specific patches to build shared libraries.

- Even though there is only one C header, it might be logical to put it into the same place as the Fortran module files.

- Maybe Fortran package should be named just 'healpix'.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. OK
 - The java library seems to be under LGPLv2+, but since it isn't included no problem.

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. OK
MUST: Optflags are used and time stamps preserved. OK

MUST: Packages containing shared library files must call ldconfig. NEEDSFIX
- chealpix and healpix-fortran need to call ldconfig in %post phase.

MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. OK
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. OK
MUST: Static libraries must be in a -static package. OK
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. OK
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. OK

MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. NEEDSFIX
- fortran-devel must require fortran since the libraries are in fortran.

MUST: Packages does not contain any .la libtool archives. OK
MUST: Desktop files are installed properly. OK
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK
Comment 8 Lubomir Rintel 2009-04-04 02:36:24 EDT
(In reply to comment #7)
> rpmlint output:
> chealpix.x86_64: W: no-soname /usr/lib64/libchealpix.so
> chealpix.x86_64: W: shared-lib-calls-exit /usr/lib64/libchealpix.so
> exit@GLIBC_2.2.5
> chealpix-devel.x86_64: W: no-documentation
> healpix-fortran.x86_64: W: no-soname /usr/lib64/libgif.so
> healpix-fortran.x86_64: W: no-soname /usr/lib64/libhealpix.so
> healpix-fortran-devel.x86_64: W: no-documentation
> 6 packages and 0 specfiles checked; 0 errors, 6 warnings.

I believe these are OK, right? I mean, we don't want to craft a SONAME, do we? We can still ask upstream though.

> - Did you send the shared library patch upstream? IIRC some package sponsors
> have frowned on Fedora specific patches to build shared libraries.

Heh, well, no, I had a little motivation fixing utterly broken makefiles. I may send it upstream, but I'm quite sure it breaks something about the static library compilation, and in fact does it impossible to compile statically w/o modifying the code. I'm not sure they would accept this.

> - Even though there is only one C header, it might be logical to put it into
> the same place as the Fortran module files.

Well, yes, I had exactly the same thought. You see, I decided not to change location when there was not Fortran module. I believe it might be a bad decision as well. We would need to change each program that includes it to -I%{_includedir}/healpix it, the very same thing we do with cfitsio. I believe leaving it in its upstream-decided traditional location has its upsides as well.

I have no problem moving it if you insist on it though.

> - Maybe Fortran package should be named just 'healpix'.

You decide. I see F90 library is called "libhealpix" in contrast to C's "libchealpix", which makes me tend to agree. Depends on where would a typical user of that library expect it to find. Honestly, I'm not exactly that kind of person (and am, in fact, secretly expecting you to comaintain the package and take care of the Fortran bindings ;)

Shortly put -- you seem to know the library much better than me, so I'd prefer stuff like naming packages and shifting files around up to you.

> MUST: Packages containing shared library files must call ldconfig. NEEDSFIX
> - chealpix and healpix-fortran need to call ldconfig in %post phase.

Good catch, will fix.

> MUST: In the vast majority of cases, devel packages must require the base
> package using a fully versioned dependency. NEEDSFIX
> - fortran-devel must require fortran since the libraries are in fortran.

Hah, this was your code, no? ;)
Anyways, will fix.

New package (probably just fixing the MUST items) following shortly.
Comment 9 Susi Lehtola 2009-04-04 04:25:27 EDT
(In reply to comment #8)
> (In reply to comment #7)
> > rpmlint output:
> > chealpix.x86_64: W: no-soname /usr/lib64/libchealpix.so
> > chealpix.x86_64: W: shared-lib-calls-exit /usr/lib64/libchealpix.so
> > exit@GLIBC_2.2.5
> > chealpix-devel.x86_64: W: no-documentation
> > healpix-fortran.x86_64: W: no-soname /usr/lib64/libgif.so
> > healpix-fortran.x86_64: W: no-soname /usr/lib64/libhealpix.so
> > healpix-fortran-devel.x86_64: W: no-documentation
> > 6 packages and 0 specfiles checked; 0 errors, 6 warnings.
> 
> I believe these are OK, right? I mean, we don't want to craft a SONAME, do we?
> We can still ask upstream though.

Probably yes, these are just warnings anyway.

> > - Did you send the shared library patch upstream? IIRC some package sponsors
> > have frowned on Fedora specific patches to build shared libraries.
> 
> Heh, well, no, I had a little motivation fixing utterly broken makefiles. I may
> send it upstream, but I'm quite sure it breaks something about the static
> library compilation, and in fact does it impossible to compile statically w/o
> modifying the code. I'm not sure they would accept this.

Yeah; I spent some time some months ago trying to figure out how to package this, but very quickly found out that I had no idea of how to do it. Your example with the C part helped me get the Fortran stuff working.

> > - Even though there is only one C header, it might be logical to put it into
> > the same place as the Fortran module files.
> 
> Well, yes, I had exactly the same thought. You see, I decided not to change
> location when there was not Fortran module. I believe it might be a bad
> decision as well. We would need to change each program that includes it to
> -I%{_includedir}/healpix it, the very same thing we do with cfitsio. I believe
> leaving it in its upstream-decided traditional location has its upsides as
> well.

Well, if upstream uses includedir, then I don't see any reason to change it. It's just one file anyway.

Still, I'm not sure about how logical it is; to compile a Fortran binding you still need to supply -I/usr/include/healpix. You should make a comment about this to healpix-devel.

> I have no problem moving it if you insist on it though.
> 
> > - Maybe Fortran package should be named just 'healpix'.
> 
> You decide. I see F90 library is called "libhealpix" in contrast to C's
> "libchealpix", which makes me tend to agree. Depends on where would a typical
> user of that library expect it to find. Honestly, I'm not exactly that kind of
> person (and am, in fact, secretly expecting you to comaintain the package and
> take care of the Fortran bindings ;)
> 
> Shortly put -- you seem to know the library much better than me, so I'd prefer
> stuff like naming packages and shifting files around up to you.

Yes, in retrospect I think "healpix" is best for the name of the Fortran package and healpix-devel for the Fortran development modules, since IIUC Healpix started out as a purely Fortran package and got the C, IDL and Java stuff later.

I don't use healpix myself (I'm in materials physics), but I have a few friends who use it. I can comaintain the package with you.

> > MUST: In the vast majority of cases, devel packages must require the base
> > package using a fully versioned dependency. NEEDSFIX
> > - fortran-devel must require fortran since the libraries are in fortran.
> 
> Hah, this was your code, no? ;)
> Anyways, will fix.

Yes, it was :)

> New package (probably just fixing the MUST items) following shortly.  

OK. Also, it occurred to me that there is a check phase that should be enabled:

make c-test
and
make f90-test
Comment 10 Susi Lehtola 2009-04-04 07:36:29 EDT
I had a look at the check phases, and realized there'd be no sense in running them as they are designed to be interactive.

Call me a masochist, but I also packaged the C++ stuff. Seemed straightforward at first, but the makefile is a bit more sick than Fortran.

Also, I was a bit too hasty with
MUST: No file conflicts with other packages and no general names.
since the Fortran stuff I added include binaries called "map2gif" and "hotspot" which are a bit too general to go in without modifications.

Also the shared library you made, libgif.so, has a too general name.

I have fixed these, and the stuff that came up in the review.

http://theory.physics.helsinki.fi/~jzlehtol/rpms/healpix.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/healpix-2.11c-4.fc10.src.rpm

rpmlint output is now:
chealpix.x86_64: W: no-soname /usr/lib64/libchealpix.so
chealpix.x86_64: W: shared-lib-calls-exit /usr/lib64/libchealpix.so exit@GLIBC_2.2.5
chealpix-devel.x86_64: W: no-documentation
healpix.src: W: strange-permission healpix-f90test.sh 0775
healpix.x86_64: W: file-not-utf8 /usr/share/doc/healpix-2.11c/test/map_mf.fits
healpix.x86_64: W: file-not-utf8 /usr/share/doc/healpix-2.11c/test/map.fits
healpix.x86_64: W: file-not-utf8 /usr/share/doc/healpix-2.11c/test/map_ngfs.fits
healpix.x86_64: W: file-not-utf8 /usr/share/doc/healpix-2.11c/test/almdec.fits
healpix.x86_64: W: file-not-utf8 /usr/share/doc/healpix-2.11c/test/alm.fits
healpix.x86_64: W: file-not-utf8 /usr/share/doc/healpix-2.11c/test/map_LOres.fits
healpix.x86_64: W: file-not-utf8 /usr/share/doc/healpix-2.11c/test/map_ext.fits
healpix.x86_64: W: file-not-utf8 /usr/share/doc/healpix-2.11c/test/map_sm.fits
healpix.x86_64: W: no-soname /usr/lib64/libhealpix_gif.so
healpix.x86_64: W: no-soname /usr/lib64/libhealpix.so
healpix-c++.x86_64: W: spurious-executable-perm /usr/share/doc/healpix-c++-2.11c/test/runtest.sh
healpix-c++.x86_64: W: no-soname /usr/lib64/libhealpix_cxxsupport.so
healpix-c++.x86_64: W: no-soname /usr/lib64/libhealpix_cxx.so
healpix-c++.x86_64: W: no-soname /usr/lib64/libhealpix_fft.so
healpix-c++-devel.x86_64: W: no-documentation
healpix-devel.x86_64: W: no-documentation
8 packages and 0 specfiles checked; 0 errors, 20 warnings.
Comment 11 Lubomir Rintel 2009-04-04 09:24:33 EDT
(In reply to comment #9)
> Yeah; I spent some time some months ago trying to figure out how to package
> this, but very quickly found out that I had no idea of how to do it. Your
> example with the C part helped me get the Fortran stuff working.

Exactly, package's configure scripts is kind of sickest shit I've seen for ages. After running it I had to run around and show all my friends :)

(In reply to comment #10)
> I had a look at the check phases, and realized there'd be no sense in running
> them as they are designed to be interactive.

I've always been amazed how people writing scientific software can get everything they can wrong, even the regression testing :)

I see you've packaged them, I'm fine with that, just moved them to -devel, since they're of more-or-less no use unless you're a developer, and needlessly occupy space when dragged in as dependency.

> Call me a masochist, but I also packaged the C++ stuff. Seemed straightforward
> at first, but the makefile is a bit more sick than Fortran.

Um, yes, you're insane :)

I'm honestly hoping that Java bindings aren't going to follow until the package is imported :) Seriously, you're going to have commit access so why delay the review with new features.

I'm pretty happy with your changes. I've removed some useless compiler flags, which are not typically used for Fedora, as they're not generic enough, or cause other sort of trouble (such as -fomit-frame-pointer making debugging more difficult, etc.)

I've also done some cosmetic changes, such as getting rid of backticks (`) just for the sake of consistency with the rest of the SPEC file, and a personal taste.

> Also, I was a bit too hasty with
> MUST: No file conflicts with other packages and no general names.
> since the Fortran stuff I added include binaries called "map2gif" and "hotspot"
> which are a bit too general to go in without modifications.
> 
> Also the shared library you made, libgif.so, has a too general name.

Sounds reasonable enough. I probably would not have renamed libgif, but I don't object either. You've done pretty good job documenting the changes, so why not.

> I have fixed these, and the stuff that came up in the review.

I'm starting to wonder who's the packager and who's reviewer here :)

If I had been the reviewer, I'd most likely approved the packages now, since I'm reasonably with your changes now. Anyways, I've done some changes, see the changelog.

> rpmlint output is now:
> chealpix.x86_64: W: no-soname /usr/lib64/libchealpix.so
> healpix-c++.x86_64: W: no-soname /usr/lib64/libhealpix_cxxsupport.so
> healpix-c++.x86_64: W: no-soname /usr/lib64/libhealpix_cxx.so
> healpix-c++.x86_64: W: no-soname /usr/lib64/libhealpix_fft.so
> healpix.x86_64: W: no-soname /usr/lib64/libhealpix_gif.so
> healpix.x86_64: W: no-soname /usr/lib64/libhealpix.so

I would not artifically add soname here, unless absolutely required. I don't think this is going to cause much trouble, I believe we're not going to have many packages that depend on this and API seems to  be reasonably sane.

> chealpix.x86_64: W: shared-lib-calls-exit /usr/lib64/libchealpix.so
> exit@GLIBC_2.2.5

This is in printerror(), which is specifically meant to exit the program. No problem here, I'd waive this.

> chealpix-devel.x86_64: W: no-documentation
> healpix-devel.x86_64: W: no-documentation
> healpix-c++-devel.x86_64: W: no-documentation

Documentation in main packages depended on by these. Again, no problem here.

> healpix.src: W: strange-permission healpix-f90test.sh 0775
> healpix-c++.x86_64: W: spurious-executable-perm
> /usr/share/doc/healpix-c++-2.11c/test/runtest.sh

I've fixed the mode, but am not sure what is this script good for.
Why pack this in a separate file and not run it in %check?

> healpix.x86_64: W: file-not-utf8 *.fits

No need to comment.

SRPMS: http://v3.sk/~lkundrak/SPECS/healpix.spec
SRPMS: http://v3.sk/~lkundrak/SRPMS/healpix-2.11c-5.el5.src.rpm
Comment 12 Susi Lehtola 2009-04-04 09:50:49 EDT
(In reply to comment #11)
> (In reply to comment #10)
> > I had a look at the check phases, and realized there'd be no sense in running
> > them as they are designed to be interactive.
> 
> I see you've packaged them, I'm fine with that, just moved them to -devel,
> since they're of more-or-less no use unless you're a developer, and needlessly
> occupy space when dragged in as dependency.

Okay, fine by me.

> 
> > Call me a masochist, but I also packaged the C++ stuff. Seemed straightforward
> > at first, but the makefile is a bit more sick than Fortran.
> 
> Um, yes, you're insane :)
> 
> I'm honestly hoping that Java bindings aren't going to follow until the package
> is imported :) Seriously, you're going to have commit access so why delay the
> review with new features.

Well, considering how sick the packaging is, I prefer to get everything in the review and make the least changes possible after that.

I have no interest in the Java stuff, if anyone needs it then s/he'll have to send a patch :)

> > I have fixed these, and the stuff that came up in the review.
> 
> I'm starting to wonder who's the packager and who's reviewer here :)
> 
> If I had been the reviewer, I'd most likely approved the packages now, since
> I'm reasonably with your changes now. Anyways, I've done some changes, see the
> changelog.

Yes, it seems that the spec file has now been mostly written by me :D

Well, I guess this kind of thing is normal with software that is as complicated as this.

> > healpix.src: W: strange-permission healpix-f90test.sh 0775
> > healpix-c++.x86_64: W: spurious-executable-perm
> > /usr/share/doc/healpix-c++-2.11c/test/runtest.sh
> 
> I've fixed the mode, but am not sure what is this script good for.
> Why pack this in a separate file and not run it in %check?

Because, IIUC, it creates a bunch of images and it's up to the user to check that they make sense.

--

We both agree on the contents and that the rpm packaging is sane. The package has been found to adhere to the guidelines and is thus

APPROVED
Comment 13 Lubomir Rintel 2009-04-04 10:04:56 EDT
New Package CVS Request
=======================
Package Name: healpix
Short Description: Hierarchical Equal Area isoLatitude Pixelization of a sphere
Owners: lkundrak, jussilehtola
Branches: EL-5 F-10 F-11
InitialCC: (just watchbugzilla please) astronomy-sig
Comment 14 Kevin Fenzi 2009-04-06 23:24:15 EDT
cvs done.
Comment 15 Lubomir Rintel 2009-04-07 03:17:02 EDT
Thanks for the review and enhancements, Jussi.
Imported and built.
Comment 16 Fedora Update System 2009-04-07 03:53:32 EDT
healpix-2.11c-3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/healpix-2.11c-3.fc10
Comment 17 Susi Lehtola 2009-04-07 04:03:19 EDT
There's something wrong with the import (and your SRPM!). The SRPM spec & sources are still revision 3 instead of 5!
Comment 18 Lubomir Rintel 2009-04-07 04:40:30 EDT
(In reply to comment #17)
> There's something wrong with the import (and your SRPM!). The SRPM spec &
> sources are still revision 3 instead of 5!  

Uff. I suck.
I'll fix that and respin the update you've created.
Sorry.
Comment 19 Lubomir Rintel 2009-04-07 04:47:00 EDT
That was probably since [1] pointed at the old package. I copied it from -3 and intended to rsync -5 there, but probably the rsync was not successful.

[1] http://v3.sk/~lkundrak/SRPMS/healpix-2.11c-5.el5.src.rpm

Anyways, the correct version in builds now. Thank you.
Comment 20 Fedora Update System 2009-04-07 11:50:48 EDT
healpix-2.11c-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

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