Some initial comments:

- provide full url to source tarball please
- %description and Summary needs some work (people don't know what yum is)
- more explicit file listing would bbe nice
- split buildreq into separate lines
- some kind of documentation would be great :-)
- drop the %dist thing in changelog?
- why is cmake used over setup.py?

New spec and srpm:
http://akozumpl.fedorapeople.org/review/dnf_2/dnf.spec
http://akozumpl.fedorapeople.org/review/dnf_2/dnf-0.2.6-8.gitb4aa5c1.fc17.noarch.rpm

(In reply to comment #1)
> Some initial comments:
> 
> - provide full url to source tarball please
Done.

> - %description and Summary needs some work (people don't know what yum is)
Improved.

> - more explicit file listing would bbe nice
That's what I would like to avoid, the current listing is concise and safe as far as I know. Or does the package have some files it shouldn't? Should I name every python source explicitly?

Updated /usr/bin/* to /usr/bin/dnf.

> - split buildreq into separate lines
Hm, I only have one buildreq..

> - some kind of documentation would be great :-)
Coming up, not just right yet. I plan to ship this into F18 with documentation (a man page for 'dnf' at least).

> - drop the %dist thing in changelog?
Done.

> - why is cmake used over setup.py?
The reason is cmake is used in libsolv, I use it in hawkey (the glue between libsolv and DNF) and here I use it to be consistent accross the stack. And it works pretty well for DNF too (and once i18n comes in I am sure cmake can handle it fine too).

Thanks, some more items.

- seems to mix tab and whitespace
- this is two buildrequires on one line: BuildRequires:	cmake python2 :-)
- file listing is recursive by default and you need to own the dir itself, change 
   %{python_sitelib}/dnf/* to  %{python_sitelib}/dnf/
- with only %config(noreplace) %{confdir}/dnf.conf,  %{confdir} ends up unowned, add
 a %dir  %{confdir} line to file listing.
- use %global not %define
- still %dist in changelogs?

Uploaded next version:

http://akozumpl.fedorapeople.org/review/dnf_3/dnf.spec
http://akozumpl.fedorapeople.org/review/dnf_3/dnf-0.2.6-9.gitb4aa5c1.fc17.src.rpm


(In reply to comment #3)
> Thanks, some more items.
> 
> - seems to mix tab and whitespace

What line? I thought rpmlint catches this but mine is silent about this.

> - this is two buildrequires on one line: BuildRequires:	cmake python2 :-)
Ah, yes, thanks.

> - file listing is recursive by default and you need to own the dir itself,
> change 
>    %{python_sitelib}/dnf/* to  %{python_sitelib}/dnf/
> - with only %config(noreplace) %{confdir}/dnf.conf,  %{confdir} ends up
> unowned, add
>  a %dir  %{confdir} line to file listing.
Fixed, thanks.

> - use %global not %define
Done.

> - still %dist in changelogs?
Sorry about that, I sent the wrong file version, should be fine now.

Would it be possible to run tests in a %check section?

What is the state of the tool?

I have done some initial testing on a F17 system and found some bugs.

a)
# dnf --enablerepo=\*testing\* update
[snip]
File "/usr/lib/python2.7/site-packages/dnf/conf.py", line 69, in _retdir
    return os.path.join(dir, self.suffix)
  File "/usr/lib64/python2.7/posixpath.py", line 66, in join
    if b.startswith('/'):
AttributeError: 'NoneType' object has no attribute 'startswith'


b)
# Does the --exclude options work?

c)
Enabling updates-testing I get a strange message:

# dnf update
Setting up Update Process
Resolving Dependencies
--> Starting dependency resolution
--> Finished dependency resolution
Error: package nfs-utils-1:1.2.6-2.fc17.x86_64 requires kmod, but none of the providers can be installed

# dnf list kmod
Installed Packages
kmod.x86_64    7-2.fc17                  @System

Why ask about kmod, it's installed?

(In reply to comment #5)
> Would it be possible to run tests in a %check section?

It would require pulling in python-hawkey for the build, I might try to get this working later.

> 
> What is the state of the tool?

A technical preview. Many features are broken or missing at this moment including --enablerepo and --exclude.

> I have done some initial testing on a F17 system and found some bugs.
> 
> a)
> # dnf --enablerepo=\*testing\* update
> [snip]
> File "/usr/lib/python2.7/site-packages/dnf/conf.py", line 69, in _retdir
>     return os.path.join(dir, self.suffix)
>   File "/usr/lib64/python2.7/posixpath.py", line 66, in join
>     if b.startswith('/'):
> AttributeError: 'NoneType' object has no attribute 'startswith'
> 
> 
> b)
> # Does the --exclude options work?
> 
> c)
> Enabling updates-testing I get a strange message:
> 
> # dnf update
> Setting up Update Process
> Resolving Dependencies
> --> Starting dependency resolution
> --> Finished dependency resolution
> Error: package nfs-utils-1:1.2.6-2.fc17.x86_64 requires kmod, but none of
> the providers can be installed
> 
> # dnf list kmod
> Installed Packages
> kmod.x86_64    7-2.fc17                  @System
> 
> Why ask about kmod, it's installed?

I'll look into this.

> # dnf update
> Setting up Update Process
> Resolving Dependencies
> --> Starting dependency resolution
> --> Finished dependency resolution
> Error: package nfs-utils-1:1.2.6-2.fc17.x86_64 requires kmod, but none of
> the providers can be installed
> 
> # dnf list kmod
> Installed Packages
> kmod.x86_64    7-2.fc17                  @System
> 
> Why ask about kmod, it's installed?

What this error says is not that it cannot see kmod installed but either that:

a) the transaction is somehow going to end up uninstalling kmod and so this dependency can not be satisfied in the end.

b) (very plauisble) there is another problem happening and we report the wrong one. 

I will try to see how the error reporting can be broken and find a way to force higher level of libsolv logging so I can get more information as I am not able to reproduce it here, but that won't happen soon.

Will you be able to approve the review before this is fixed? Thanks.

Yeah, I think so. I don't see any stoppers here at the moment, however #833462 must be done first.

(In reply to comment #8)
> Yeah, I think so. I don't see any stoppers here at the moment, however
> #833462 must be done first.

Sure, hawkey has just been built and submitted as a f17 update.

I looking at the license status, most files are GPLv2+, 
however I find some other stuff too:

in dnf/ subdir :

yum/sqlutils.py : GPLv2
yum/misc.py : unknown
yum/parser.py : unknown
rpmUtils/oldUtils.py : unknown
rpmUtils/transaction.py : GPL
rpmUtils/arch.py : 
rpmUtils/__init__.py : unknown
rpmUtils/tests/updates-test.py : unknown

bin/dnf : unknown

Could you please have a look at this issue?

(In reply to comment #10)
> I looking at the license status, most files are GPLv2+, 
> however I find some other stuff too:
> 
> in dnf/ subdir :
> 
> yum/sqlutils.py : GPLv2
> yum/misc.py : unknown
> yum/parser.py : unknown
> rpmUtils/oldUtils.py : unknown
> rpmUtils/transaction.py : GPL
> rpmUtils/arch.py : 
> rpmUtils/__init__.py : unknown
> rpmUtils/tests/updates-test.py : unknown
> 
> bin/dnf : unknown
> 
> Could you please have a look at this issue?

With the exception of bin/dnf where I will yet add the license information, these files are all taken over from yum which has been licensed as GPLv2+ since 2008.

Terje, can you please estimate when you'll be able to approve this? Based on comment 8 I thought we were all set. It's ten days later and nothing has happened.. I will look for another reviewer on Monday unless this moves.

Yum has been labeled in the rpm as GPLv2+ - Terje is right - there appears to be some differentiation in the license applied to individual files in yum. For DNF that's going to need to be fixed and/or labeled in the spec file.

I removed some of the useless files in 3544ca1d9c1a18825ba3034cd153bd3571d5dae8 and now I am down to these:

yum/sqlutils.py : GPLv2
yum/misc.py : unknown
yum/parser.py : unknown
rpmUtils/transaction.py : GPL
rpmUtils/arch.py : unknown

At this point DNF unfortunately needs all of those.

Seth, 'git log' in yum reveals that yum/misc.py, rpmUtils/transaction.py and rpmUtils/arch.py were started by you. What was the reason to omit the licensing information there?

Since it would be hard to get an approval of all the people who contributed to the files in the meantime I changed the licensing in the spec to a "Multiple Licensing Scenario" [1].

Here's the new spec and SRPM:
http://akozumpl.fedorapeople.org/review/dnf_4/dnf.spec
http://akozumpl.fedorapeople.org/review/dnf_4/dnf-0.2.6-10.git964faae.fc17.src.rpm

[1] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios

From my POV the licensing is OK now.
Correctly listed all licenses and used Public Domain license for not licensed files.

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

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



==== Generic ====
[ ]: 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.
[ ]: 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
[ ]: MUST Package contains no bundled libraries.
[ ]: 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
[ ]: MUST Sources contain only permissible code or content.
[x]: MUST %config files are marked noreplace or the reason is justified.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[ ]: MUST Macros in Summary, %description expandable at SRPM build time.
[ ]: MUST Package requires other packages for directories it uses.
[ ]: MUST Package uses nothing in %doc for runtime.
[ ]: 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 Spec file lacks Packager, Vendor, PreReq tags.
[!]: 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
[ ]: MUST Large documentation files are in a -doc subpackage, if required.
[!]: 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.
[ ]: MUST License field in the package spec file matches the actual license.
[ ]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package is named according to the Package Naming Guidelines.
[ ]: MUST No %config files under /usr.
[ ]: MUST Package does not generate any conflict.
[ ]: MUST Package obeys FHS, except libexecdir and /usr/target.
[ ]: MUST Package must own all directories that it creates.
[ ]: MUST Package does not own files or directories owned by other packages.
[ ]: MUST Package installs properly.
[ ]: MUST Requires correct, justified where necessary.
[!]: MUST Rpmlint output is silent.

rpmlint dnf-0.2.6-10.git964faae.fc18.src.rpm

dnf.src: W: spelling-error Summary(en_US) libsolv -> absolve
dnf.src: W: spelling-error %description -l en_US libsolv -> absolve
dnf.src: W: invalid-license GPL
dnf.src: W: strange-permission dnf-964faae.tar.xz 0600L
dnf.src: W: strange-permission dnf.spec 0600L
dnf.src: W: invalid-url Source0: http://akozumpl.fedorapeople.org/dnf-964faae.tar.xz HTTP Error 404: Not Found
1 packages and 0 specfiles checked; 0 errors, 6 warnings.


rpmlint dnf-0.2.6-10.git964faae.fc18.noarch.rpm

dnf.noarch: W: spelling-error Summary(en_US) libsolv -> absolve
dnf.noarch: W: spelling-error %description -l en_US libsolv -> absolve
dnf.noarch: W: invalid-license GPL
dnf.noarch: W: no-manual-page-for-binary dnf
1 packages and 0 specfiles checked; 0 errors, 4 warnings.


[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/akozumpl/tmp/review/dnf-964faae.tar.xz :
  MD5SUM this package     : 49086a82dd10cb99010b9b01b1656bc5
  MD5SUM upstream package : d41d8cd98f00b204e9800998ecf8427e

[ ]: 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.
[ ]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[ ]: 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.
[ ]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[ ]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[ ]: SHOULD Package functions as described.
[ ]: SHOULD Latest version is packaged.
[ ]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD SourceX is a working URL.
[ ]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[ ]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[ ]: SHOULD %check is present and all tests pass.
[ ]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Issues:
[!]: 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
See: None
[!]: 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.
See: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text
[!]: MUST Rpmlint output is silent.

rpmlint dnf-0.2.6-10.git964faae.fc18.src.rpm

dnf.src: W: spelling-error Summary(en_US) libsolv -> absolve
dnf.src: W: spelling-error %description -l en_US libsolv -> absolve
dnf.src: W: invalid-license GPL
dnf.src: W: strange-permission dnf-964faae.tar.xz 0600L
dnf.src: W: strange-permission dnf.spec 0600L
dnf.src: W: invalid-url Source0: http://akozumpl.fedorapeople.org/dnf-964faae.tar.xz HTTP Error 404: Not Found
1 packages and 0 specfiles checked; 0 errors, 6 warnings.


rpmlint dnf-0.2.6-10.git964faae.fc18.noarch.rpm

dnf.noarch: W: spelling-error Summary(en_US) libsolv -> absolve
dnf.noarch: W: spelling-error %description -l en_US libsolv -> absolve
dnf.noarch: W: invalid-license GPL
dnf.noarch: W: no-manual-page-for-binary dnf
1 packages and 0 specfiles checked; 0 errors, 4 warnings.


See: http://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint
[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/akozumpl/tmp/review/dnf-964faae.tar.xz :
  MD5SUM this package     : 49086a82dd10cb99010b9b01b1656bc5
  MD5SUM upstream package : d41d8cd98f00b204e9800998ecf8427e

See: http://fedoraproject.org/wiki/Packaging/SourceURL


Generated by fedora-review 0.1.3
External plugins:

Thanks everyone who participated in the review.

New Package SCM Request
=======================
Package Name: dnf
Short Description: Package manager forked from Yum, using libsolv as a dependency resolver
Owners: akozumpl
Branches: f18
InitialCC:

(In reply to comment #15)
> From my POV the licensing is OK now.
> Correctly listed all licenses and used Public Domain license for not
> licensed files.

Maybe I'm misunderstanding what's written there, but please note that it is very, very far from acceptable to assume a file with no included license is public domain.

Indeed, I would say files without license should have license according to COPYING (GPLv2).
I guess that is the reason for the COPYING file in the first place?

The main Yum developers have confirmed the disputed files are licensed under GPLv2 (and definitely do not belong in Public Domain) [1]. I updated spec and PACKAGE-LICENSING accordingly:
http://akozumpl.fedorapeople.org/review/dnf_5/dnf.spec
http://akozumpl.fedorapeople.org/review/dnf_5/dnf-0.2.6-11.gitb1f1c08.fc17.src.rpm

[1] http://lists.baseurl.org/pipermail/yum-devel/2012-July/009376.html

It is too early to request f18 branches.  Otherwise....

Git done (by process-git-requests).

hmm, shouldn't this bug move to modified now when the package is in F18?

No, you should close it by hand.

Fair enough, closing this.