(In reply to comment #0)
Ales,

The .spec as-is produces a binary python-hawkey package that can't be installed due to a missing requires on "libhawkey.so.0()(64bit)".  This is caused by _hawkey_testmodule.so having a DT_NEEDED entry (i.e. is linked with) _hawkeymodule.so and RPM automatically generating a requires from that.
We can stop RPM from generating that requires by adding this snippet:

 %global gitrev 04ecf00
 %global libsolv_version 0.0.0-11

+# filter out _hawkey_testmodule.so DT_NEEDED _hawkeymodule.so
+%{?filter_setup:
+%filter_requires_in %{python_sitearch}/hawkey/test/.*\.so$ 
+%filter_setup
+}
+
 Name:          hawkey

to the .spec file per http://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering#Removing_items_from_the_requires_stream_.28post-scan_filtering.29
b

But I must ask do you _REALLY_ want to ship the "hawkey.test" package in the RPM? Is it really useful?

I'm a bit mystified in how I managed to change the CC, Component, Assignee fields while submitting comment #1. Let me try to change it back without causing more damage.

(In reply to comment #1)
> (In reply to comment #0)
> Ales,
> 
> The .spec as-is produces a binary python-hawkey package that can't be
> installed due to a missing requires on "libhawkey.so.0()(64bit)".  This is\

Sorry about that. I didn't test the final package after adding the provides filtering on the python binary libs (something else that rpmlint complained about).

> caused by _hawkey_testmodule.so having a DT_NEEDED entry (i.e. is linked
> with) _hawkeymodule.so and RPM automatically generating a requires from that.
> We can stop RPM from generating that requires by adding this snippet:

It is strange the regular 'hawkey' python module doesn't cause the same thing.

> 
>  %global gitrev 04ecf00
>  %global libsolv_version 0.0.0-11
> 
> +# filter out _hawkey_testmodule.so DT_NEEDED _hawkeymodule.so
> +%{?filter_setup:
> +%filter_requires_in %{python_sitearch}/hawkey/test/.*\.so$ 
> +%filter_setup
> +}
> +
>  Name:          hawkey
> 
> to the .spec file per
> http://fedoraproject.org/wiki/Packaging:
> AutoProvidesAndRequiresFiltering#Removing_items_from_the_requires_stream_.
> 28post-scan_filtering.29
> b
> 
> But I must ask do you _REALLY_ want to ship the "hawkey.test" package in the
> RPM? Is it really useful?

I would like to avoid this if at all possible: hawkey.test is useful for building python unit tests for the library and for components using it, so if it's in the .rpm people won't have to build hawkey from source (and set PYTHONPATH) to unit test DNF for instance.

Let me think about how to unef this for a while.

(In reply to comment #3)

> > caused by _hawkey_testmodule.so having a DT_NEEDED entry (i.e. is linked
> > with) _hawkeymodule.so and RPM automatically generating a requires from that.
> > We can stop RPM from generating that requires by adding this snippet:
> 
> It is strange the regular 'hawkey' python module doesn't cause the same
> thing.

scratch that part i get it now.

Here's a new spec file and a new .srpm:

http://akozumpl.fedorapeople.org/review/hawkey_2/hawkey.spec
http://akozumpl.fedorapeople.org/review/hawkey_2/hawkey-0.2.4-6.git04ecf00.fc17.src.rpm

I updated the spec according to your suggestion with %filter_requires_in.

Thanks!

https://fedoraproject.org/wiki/Packaging:ReviewGuidelines

| MUST: rpmlint must be run on the source rpm and all binary rpms
| the build produces. The output should be posted in the review.[1]

That doesn't imply it's only the reviewer who must do this. rpmlint is also a tool for packagers.


> Summary:	A Library providing simplified C and Python API to libsolv

Summaries without leading articles are more readable in package tools and Anaconda.


> Group:		Development/Libraries

"System Environment/Libraries" is the primary group for shared lib packages.


> Source0:	hawkey-%{gitrev}.tar.xz

https://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control


> BuildRequires:	python2 python2-devel

This build requirement doesn't match the run-time requirement further below, which makes it unsafe and not precise enough:

  | %package -n python-hawkey
  | Requires:	python

Plus, 
https://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires
also applies here, and a comment in the spec file would be good.


> Requires:	libsolv >= %{libsolv_version}

https://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires

Also, consider using %{?_isa} in such explicit dependencies, too.


> %description
> A Library providing simplified C and Python API to libsolv

Here, creating full sentences including punctuation would be the way to go.


> %files
> %doc COPYING README.md

> %files devel
> %doc COPYING

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Subpackage_Licensing


> %_includedir/hawkey

When including entire directory trees like this, it's typically more readable to add a trailing slash and make clear that it's not a single file to be included only:

  %_includedir/hawkey/

Updated version: 
http://akozumpl.fedorapeople.org/review/hawkey_3/hawkey.spec
http://akozumpl.fedorapeople.org/review/hawkey_3/hawkey-0.2.4-7.git04ecf00.fc17.src.rpm

I tried to resolve your comments, except:

(In reply to comment #6)
> https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
> 
> 
> > BuildRequires:	python2 python2-devel
> 
> This build requirement doesn't match the run-time requirement further below,
> which makes it unsafe and not precise enough:
> 
>   | %package -n python-hawkey
>   | Requires:	python
> 
> Plus, 
> https://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires
> also applies here, and a comment in the spec file would be good.

Not sure what you are suggesting should happen here here besides changing 'python' to 'python2'.


Thanks, Ales

Although the section at
https://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires
focuses on explicit library package deps, its bottom is meant to be understood
like

"In the spec file, you should add comments to all explicit dependencies and explain why there is a manually added dependency on something."

For the python-hawkey package, there is an automatic dependency on

  python(abi) = 2.7

already, so your spec file comment should explain why a manually added dependency on "python" or "python2" would be needed.

(In reply to comment #8)
> Although the section at
> https://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires
> focuses on explicit library package deps, its bottom is meant to be
> understood
> like
> 
> "In the spec file, you should add comments to all explicit dependencies and
> explain why there is a manually added dependency on something."
> 
> For the python-hawkey package, there is an automatic dependency on
> 
>   python(abi) = 2.7
> 
> already, so your spec file comment should explain why a manually added
> dependency on "python" or "python2" would be needed.

I just removed the dependnecy as there was not a good reason I can remember why I put it in there. See the latest spec here: http://akozumpl.fedorapeople.org/review/hawkey_4/hawkey.spec

Scott, will you be able to approve this review now? Thanks, Ales

(In reply to comment #9)
Ales, with your latest patch, the Requires generated seems fine.

Looking at the files included in the hawkey RPMS, one thing that stands out is that it's a C library without a pkgconfig file. Would you consider including this patch to add one? https://github.com/akozumpl/hawkey/pull/1

(In reply to comment #10)
> (In reply to comment #9)
> Ales, with your latest patch, the Requires generated seems fine.
> 
> Looking at the files included in the hawkey RPMS, one thing that stands out
> is that it's a C library without a pkgconfig file. Would you consider
> including this patch to add one? https://github.com/akozumpl/hawkey/pull/1

Merged to master now, thank you.

This review is against the latest .spec file in hawkey git:

1. Remove python2 from BuildRequires
python2-devel pulls it in anyway.

-BuildRequires: python2 python2-devel
+BuildRequires: python2-devel

2. Remove "rm -rf $RPM_BUILD_ROOT" from %install section

 %install
-rm -rf $RPM_BUILD_ROOT

Only needed if you're targetting EPEL5.

3. Quote _includedir like other RPM directory name macros

-%_includedir/hawkey/
+%{_includedir}/hawkey/

I've uploaded a copy of hawkey.spec with my recommended changes above here: http://scottt.tw/fedora/hawkey.spec
I'll approve this package once the points raised above are addressed.

Ales,
while I have your attention I'll raise a few issue non-packaging related issues that I see in hawkey:

1. You don't tag your releases in git.
Instead you seem to commit a change to hawkey.spec in upstream git with an over detailed changelog entry on release.

I understand you're doing both upstream development and Fedora packaging and don't want to be bogged down with busy work but if you look at the Fedora gnome or yum packages etc, they tag their upstream releases and their RPM changelog just say "New upstream release".

2. You might want to register the name "hawkey" in the Python package index to avoid name conflicts.
See: http://pypi.python.org/pypi?%3Aaction=submit_form

You can always relinquish the name later if the hawkey project prove to be short lived because all its functionality got merged back into libsolv and rpm-libs etc.

(In reply to comment #12)
> This review is against the latest .spec file in hawkey git:
> 
> I've uploaded a copy of hawkey.spec with my recommended changes above here:
> http://scottt.tw/fedora/hawkey.spec

Merged, thanks.


> I'll approve this package once the points raised above are addressed.

Thank you.

(In reply to comment #13)
> Ales,
> while I have your attention I'll raise a few issue non-packaging related
> issues that I see in hawkey:
> 
> 1. You don't tag your releases in git.
> Instead you seem to commit a change to hawkey.spec in upstream git with an
> over detailed changelog entry on release.

Those releases I've done until now were only informal. I will find a more rigorous way once in Fedora. I might even start tagging them if more people find it useful.

> 
> I understand you're doing both upstream development and Fedora packaging and
> don't want to be bogged down with busy work but if you look at the Fedora
> gnome or yum packages etc, they tag their upstream releases and their RPM
> changelog just say "New upstream release".

I am probably not going to adopt this practice, it renders the Fedora changelogs of little value for library client developers.

> 
> 2. You might want to register the name "hawkey" in the Python package index
> to avoid name conflicts.
> See: http://pypi.python.org/pypi?%3Aaction=submit_form
> 

Thanks, will do that.

(In reply to comment #14)
> Merged, thanks.


Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== C/C++ ====
[x]: MUST Header files in -devel subpackage, if present.
[x]: MUST ldconfig called in %post and %postun if required.
[x]: MUST Package does not contain any libtool archives (.la)
[x]: MUST Package does not contain kernel modules.
[x]: MUST Package contains no static executables.
[x]: MUST Rpath absent or only used for internal libs.
[x]: MUST Package is not relocatable.
[x]: MUST Development (unversioned) .so files in -devel subpackage, if
     present.
     Note: /usr/lib/python2.7/site-packages/hawkey/_hawkeymodule.so and
     /usr/lib/python2.7/site-packages/hawkey/test/_hawkey_testmodule.so
     are Python extension modules and don't belong in -devel.


==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Fully versioned dependency in subpackages, if present.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf is only needed if supporting EPEL5
[x]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
[x]: MUST License file installed when any subpackage combination is installed.
[x]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint output is silent.

I've reviewed the rpmlint warnings and think they're fine.

rpmlint hawkey-devel-0.2.4-8.git04ecf00.fc18.i686.rpm

hawkey-devel.i686: W: spelling-error Summary(en_US) libsolv -> absolve
hawkey-devel.i686: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

rpmlint python-hawkey-0.2.4-8.git04ecf00.fc18.i686.rpm

python-hawkey.i686: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


rpmlint hawkey-0.2.4-8.git04ecf00.fc18.i686.rpm

hawkey.i686: W: spelling-error Summary(en_US) libsolv -> absolve
hawkey.i686: W: spelling-error %description -l en_US libsolv -> absolve
hawkey.i686: W: shared-lib-calls-exit /usr/lib/libhawkey.so.0 exit
1 packages and 0 specfiles checked; 0 errors, 3 warnings.


rpmlint hawkey-debuginfo-0.2.4-8.git04ecf00.fc18.i686.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


rpmlint hawkey-0.2.4-8.git04ecf00.fc18.src.rpm

hawkey.src: W: spelling-error Summary(en_US) libsolv -> absolve
hawkey.src: W: spelling-error %description -l en_US libsolv -> absolve
hawkey.src: W: strange-permission hawkey-04ecf00.tar.xz 0600L
hawkey.src:66: W: macro-in-comment %{_libdir}
hawkey.src: W: invalid-url Source0: hawkey-04ecf00.tar.xz
1 packages and 0 specfiles checked; 0 errors, 5 warnings.


[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
Package has no sources or they are generated by developer
[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[x]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD Scriptlets must be sane, if used.
[x]: SHOULD SourceX is a working URL.
[x]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[x]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.


APPROVED.

> 
> APPROVED.

Thank you for the review and the patches.

New Package SCM Request
=======================
Package Name: hawkey
Short Description: Library providing simplified C and Python API to libsolv
Owners: akozumpl
Branches: f17 f18
InitialCC:

(In reply to comment #17)
Best of luck on improving package management in Fedora.

BTW, where is a good place to discuss hawkey and dnf development?
I'd like to know more about the problems you're trying to solve and
potentially helping out.

(In reply to comment #19)
> (In reply to comment #17)
> Best of luck on improving package management in Fedora.
> 
> BTW, where is a good place to discuss hawkey and dnf development?
> I'd like to know more about the problems you're trying to solve and
> potentially helping out.

Yes, for those things I hang out on freenode's #yum+rpm.

What we are trying to solve is those things outlined in the feature: better defined API and unified depsolver. If you checkout yum and poke around you'll see what I'm talking about.

Git done (by process-git-requests).

No need to request f18, devel is automatic.

hawkey-0.2.5-1.git042738b.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/hawkey-0.2.5-1.git042738b.fc17

hawkey-0.2.5-1.git042738b.fc17 has been pushed to the Fedora 17 testing repository.

hawkey-0.2.5-1.git042738b.fc17 has been pushed to the Fedora 17 stable repository.