Bug 706432

licensecheck finds the license to be ISC.

What does "aggregate(1)" mean in the ios description?

You don't need to include the license file and history in the sub-package, as it requires the main package.

Better use an asterix instead of "gz" in the files section.

The manpages should be installed with -p.

(In reply to comment #1)
Thanks for the review.

> licensecheck finds the license to be ISC.

Obviously BSD type licenses confuse me. Didn't know about licensecheck.

> What does "aggregate(1)" mean in the ios description?

I guess it means aggregate in man section 1, as most commands are usually referred in documentation. But I guess not necessary here. I've removed it.

> You don't need to include the license file and history in the sub-package, as
> it requires the main package.

Removed.
 
> Better use an asterix instead of "gz" in the files section.

Changed to asterisk.

> The manpages should be installed with -p.

Added a -p.



1.6-2
- Some changes suggested by review comment #1

Spec URL: http://declera.com/~yaneti/aggregate/aggregate.spec
SRPM URL: http://declera.com/~yaneti/aggregate/aggregate-1.6-2.fc16.src.rpm

If you understand what the code is supposed to do, please try to correct:

aggregate.c:257:9: warning: suggest parentheses around '&&' within '||' [-Wparentheses]

Please write a comment on the patch in the spec file. Something like: "Adjust installation to use DESTDIR and preserve manpage timestamps"

You could also change the name of the patch to something more self-explanatory.

The sub-package's require should look like this: http://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

If you don't have a good reason not to, use the name macro in the Source0 and Patch0 line. Also use it in the files section.

(You can delete the trailing slash on the URL line, if you want.)

Yes, asteriSK, of course! :)

- I'd rather not get into code at the time of review

- I think "-fedora" is pretty much self explanatory for both the patch and the changes it does. Basically changes specific to fedora that don't alter the main functionality of the program or fix specific upstream bugs. No need to go into details.

- I don't think the requires is all that specific. What the perl script wants is to run a binary called aggregate.

- The %{name} macro use is just pedantry with little practical value ihmo.

Thanks

I am triaging old review tickets.  I can't promise a review if you reply, but by closing out the stale tickets we can devote extra attention to the ones which aren't stale.

srpm link above is not valid.

sorry about that, here it is:

Spec URL: http://declera.com/~yaneti/aggregate/aggregate.spec
SRPM URL: http://declera.com/~yaneti/aggregate/aggregate-1.6-2.fc17.src.rpm

This one probably should have been marked trivial.  Packages don't get much simpler.  I guess I could ask why the -ios subpackage isn't made noarch, since it's just a script and a manpage, but I doubt it's worth it for 5K.  Otherwise there's nothing else I can really say.

APPROVED

* source files match upstream.  sha256sum:
  166503005cd8722c730e530cc90652ddfa198a25624914c65dffc3eb87ba5482
   aggregate-1.6.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper (none needed).
* compiler flags are appropriate.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks complete.
* rpmlint is silent (except for pointless spelling complaints).
* final provides and requires are sane:
  aggregate-1.6-2.fc20.x86_64.rpm
   aggregate = 1.6-2.fc20
   aggregate(x86-64) = 1.6-2.fc20
  =
   (none)

  aggregate-ios-1.6-2.fc20.x86_64.rpm
   aggregate-ios = 1.6-2.fc20
   aggregate-ios(x86-64) = 1.6-2.fc20
  =
   /usr/bin/perl  
   aggregate  
   perl(IPC::Open2)  
   perl(strict)  

* no bundled libraries.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files.
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no static libraries.
* no libtool .la files.

Thanks. I'll take your suggestion for the noarch -ios subpackage.

New Package SCM Request
=======================
Package Name: aggregate
Short Description: IPv4 CIDR prefix aggregator
Owners: yaneti
Branches: f19
InitialCC:

New Package SCM Request
=======================
Package Name: aggregate
Short Description: IPv4 CIDR prefix aggregator
Owners: yaneti
Branches: f19 el6
InitialCC:

Git done (by process-git-requests).

Imported. Built. Updates submitted.
Thanks again.