[Bug 492164] Review Request: healpix - Hierarchical Equal Area isoLatitude Pixelization of a sphere

bugzilla at redhat.com bugzilla at redhat.com
Sat Apr 4 13:24:35 UTC 2009


Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=492164





--- Comment #11 from Lubomir Rintel <lkundrak at v3.sk>  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 at 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

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.




More information about the Fedora-package-review mailing list