Bug 492164
Summary: | Review Request: healpix - Hierarchical Equal Area isoLatitude Pixelization of a sphere | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Lubomir Rintel <lkundrak> | ||||
Component: | Package Review | Assignee: | Susi Lehtola <susi.lehtola> | ||||
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | low | ||||||
Version: | rawhide | CC: | fedora-package-review, notting, susi.lehtola | ||||
Target Milestone: | --- | Flags: | susi.lehtola:
fedora-review+
kevin: fedora-cvs+ |
||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2009-04-07 07:17:02 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Bug Depends On: | |||||||
Bug Blocks: | 493957 | ||||||
Attachments: |
|
Description
Lubomir Rintel
2009-03-25 17:30:01 UTC
What about the Fortran stuff? That's important as well. (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. Created attachment 336762 [details]
Patch to spec file that adds Fortran stuff
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 :) I'll do the review once you have updated the package to provide the Fortran stuff too. 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 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.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 (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.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. (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.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 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.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. (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.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 (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 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 cvs done. Thanks for the review and enhancements, Jussi. Imported and built. healpix-2.11c-3.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/healpix-2.11c-3.fc10 There's something wrong with the import (and your SRPM!). The SRPM spec & sources are still revision 3 instead of 5! (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. 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. 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. |