Bug 556628 - Review Request: pymilia - Python wrappers for milia
Summary: Review Request: pymilia - Python wrappers for milia
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Thomas Spura
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 556625
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-01-18 22:04 UTC by Sergio Pascual
Modified: 2010-02-04 13:59 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-02-04 13:59:10 UTC
Type: ---
Embargoed:
tomspur: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Sergio Pascual 2010-01-18 22:04:27 UTC
Spec URL: http://guaix.fis.ucm.es/~spr/pymilia.spec
SRPM URL: http://guaix.fis.ucm.es/~spr/pymilia-0.3.0-1.src.rpm
Description:
Python wrappers for milia. Milia is a C++ library created to 
compute cosmological distances and ages in the 
Friedmann-Lemaître-Robertson-Walker metric.

Comment 1 Sergio Pascual 2010-01-19 17:11:25 UTC
I have upload the files to fedorapeople also


Spec URL: http://sergiopr.fedorapeople.org/pymilia.spec
SRPM URL: http://sergiopr.fedorapeople.org/pymilia-0.3.0-1.fc12.src.rpm

Comment 2 Thomas Spura 2010-01-20 04:10:15 UTC
First same here, like in your milia package:

Sources do not match upstream (diff -r is empty), so what happend here?
1f9d6d2d4de2f14a90aa5762b5f67156  your sources
3b9d7aa9e4033ee945b879e9aee0525c  upstream url


Review:

Good:
- name ok
- license ok
- %clean there
- %install the python way
- %doc ok
- permissions ok
- no static libs
- rpmlint clean
- macros everywhere


TODO:
- changelog format:
  add a '- ' to the last line
- Are BR ok?
  This is a wrapper for milia so why shouldn't milia-devel >= %{version} be needed?
  It would be the sane way ;)
  python-setuptools is missing.


Waiting for the sources comment, till I'll approve this (There are no other blocker, not to do so...).

Comment 3 Sergio Pascual 2010-01-20 09:05:10 UTC
Spec URL: http://sergiopr.fedorapeople.org/pymilia.spec
SRPM URL: http://sergiopr.fedorapeople.org/pymilia-0.3.0-2.fc12.src.rpm

I have fixed the typo in the changelog and added python-setuptools. I have removed python-devel, because it's already required by milia-devel

The tarballs, again, only differ in the timestamp. It's fixed now

Comment 4 Susi Lehtola 2010-01-20 11:18:00 UTC
Are you sure the Fedora optimization flags are used in the build...? Normally one needs to pass e.g.

# Remove CFLAGS=... for noarch packages (unneeded)
CFLAGS="$RPM_OPT_FLAGS" %{__python} setup.py build

as in the python spec file template.

Comment 5 Sergio Pascual 2010-01-20 11:38:02 UTC
I get the same flags with ot without CFLAGS="$RPM_OPT_FLAGS", namely

-pthread -fno-strict-aliasing -DNDEBUG -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -D_GNU_SOURCE -fPIC -fPIC

Where is this python spec template?

Comment 6 Christoph Wickert 2010-01-20 11:47:13 UTC
If you can rpmdev-newspec with a name that includes "py", it will automatically use the python template. To manually select python, run

  rpmdev-newspec -t python foo

rpmdev-newspec --help has more options and templates.

Comment 7 Thomas Spura 2010-01-20 12:21:13 UTC
- Please add the CFLAGS as in the template.
  But because you use %{buildroot}, use %{optflags} instead $RPM_OPT_FLAGS.
 
https://fedoraproject.org/wiki/Packaging/Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS
  (Thanks Jussi.)

- Add BR: python-devel again, please.
  Why is it required by milia-devel?
  Didn't you make two packages to avoid milia-devel requires python*?

  Another reason for adding this would be to not interfer with python3*.

Provides: libmilia.so.3()(64bit)
Requires(interp): /sbin/ldconfig /sbin/ldconfig
Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <=
4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
Requires(post): /sbin/ldconfig
Requires(postun): /sbin/ldconfig
Requires: libc.so.6()(64bit) libc.so.6(GLIBC_2.2.5)(64bit)
libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgsl.so.0()(64bit)
libgslcblas.so.0()(64bit) libm.so.6()(64bit) libm.so.6(GLIBC_2.2.5)(64bit)
libmilia.so.3()(64bit) libstdc++.so.6()(64bit)
libstdc++.so.6(CXXABI_1.3)(64bit) libstdc++.so.6(GLIBCXX_3.4)(64bit)
libstdc++.so.6(GLIBCXX_3.4.9)(64bit) rtld(GNU_HASH)
Processing files: milia-devel-0.3.0-1.fc12.x86_64
Provides: pkgconfig(milia) = 0.3.0
Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <=
4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 rpmlib(VersionedDependencies)
<= 3.0.3-1
Requires: /usr/bin/pkg-config libmilia.so.3()(64bit) pkgconfig(gsl)


Don't see a python* here.

Comment 8 Sergio Pascual 2010-01-20 12:34:17 UTC
Sorry if I'm missing something, but pymilia already depends in python-devel. 

BuildRequires: python-devel python-setuptools milia-devel >= %{version}

I've added the CFLAGS line

Spec URL: http://sergiopr.fedorapeople.org/pymilia.spec
SRPM URL: http://sergiopr.fedorapeople.org/pymilia-0.3.0-3.fc12.src.rpm

Comment 9 Thomas Spura 2010-01-20 12:49:23 UTC
(In reply to comment #8)
> Sorry if I'm missing something, but pymilia already depends in python-devel. 

Yes, it does in this case, because there is BR python-setuptools, but I believe this is no reason to always trust this and leave it out.
I'm unsure, if it's allowed to leave it you (Guidelines say should, so probably yes), but adding is definately the saner way ;)


The new dependency (milia) is already approved.


##########################

APPROVED

Comment 10 Sergio Pascual 2010-01-20 12:59:58 UTC
New Package CVS Request
=======================
Package Name: pymilia
Short Description: Python wrappers for milia
Owners: sergiopr
Branches: F-12 F-11 EL-5
InitialCC:

Comment 11 Jason Tibbitts 2010-01-24 19:21:53 UTC
The requested package name (pymilia) doesn't match the package name in the bug
summary (Pymilia).  Please verify that the lower case version is correct, and
edit the bug summary if so.

Comment 12 Sergio Pascual 2010-01-24 22:07:40 UTC
Lower case is correct, fixed in the summary

Comment 13 Jason Tibbitts 2010-01-27 05:12:05 UTC
CVS done (by process-cvs-requests.py).

Comment 14 Thomas Spura 2010-02-04 13:59:10 UTC
Build in rawhide:

http://koji.fedoraproject.org/koji/buildinfo?buildID=153248


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