Hi Michele.

Use the macros instead of hard-coded directory names (see http://fedoraproject.org/wiki/Packaging:RPMMacros).

Hi Antonio,

thanks for letting me know. I've uploaded fixed spec and srpm:
Spec URL: http://acksyn.org/files/rpms/captcp/captcp.spec
SRPM URL: http://acksyn.org/files/rpms/captcp/captcp-1.6-2.fc20.src.rpm

I increased the release tag, hope that is okay. Here is the changelog:
* Sat Nov 02 2013 Michele Baldessari <michele> - 1.6-2
- Fixed two hardcoded dirs in install section (Antonio Trande <anto.trande>)
- Added texlive-epstopdf-bin in Requires as socketstatistic requires it
- Added a patch to correct some help messages 

I've now git-ified the patch applying (a bit like in xorg-x11-server) so
rpmlint now needlessly warns about:
captcp.spec: W: patch-not-applied Patch0: 0001-The-option-is-called-statistic-not-statistics.patch

Hope that is alright.

thanks again and regards,
Michele

(In reply to Michele Baldessari from comment #2)
> Hi Antonio,
> 
> thanks for letting me know. I've uploaded fixed spec and srpm:
> Spec URL: http://acksyn.org/files/rpms/captcp/captcp.spec
> SRPM URL: http://acksyn.org/files/rpms/captcp/captcp-1.6-2.fc20.src.rpm
> 
> I increased the release tag, hope that is okay. Here is the changelog:
> * Sat Nov 02 2013 Michele Baldessari <michele> - 1.6-2
> - Fixed two hardcoded dirs in install section (Antonio Trande
> <anto.trande>)

You don't need to indicate my Name/Surname/Mail. Please, don't do that.
Hardcoded directory names must be avoided in the whole file.

> - Added texlive-epstopdf-bin in Requires as socketstatistic requires it
> - Added a patch to correct some help messages 
> 
> I've now git-ified the patch applying (a bit like in xorg-x11-server) so
> rpmlint now needlessly warns about:
> captcp.spec: W: patch-not-applied Patch0:
> 0001-The-option-is-called-statistic-not-statistics.patch

Probably, it means that you have not applied the patch included in your source rpm. You can simply use the %patch command in the %setup section.
See http://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25prep_section:_.25patch_commands and
http://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

Remove the line 'rm -rf $RPM_BUILD_ROOT' in the %install section.

Why have you erased the '%{buildroot}/{_bindir}/captcp' file ?
Leave a comment to explain your reasons. ;)

Also, some slashes are redundant:

%{buildroot}/%{_bindir}/captcp is wrong.
%{buildroot}%{_bindir}/captcp is fine.

%{buildroot}/%{_mandir}/man8/captcp.8 is wrong.
%{buildroot}%{_mandir}/man8/captcp.8 is fine.

And so on.

(In reply to Antonio Trande from comment #3)
> (In reply to Michele Baldessari from comment #2)
> > Hi Antonio,
> > 
> > thanks for letting me know. I've uploaded fixed spec and srpm:
> > Spec URL: http://acksyn.org/files/rpms/captcp/captcp.spec
> > SRPM URL: http://acksyn.org/files/rpms/captcp/captcp-1.6-2.fc20.src.rpm
> > 
> > I increased the release tag, hope that is okay. Here is the changelog:
> > * Sat Nov 02 2013 Michele Baldessari <michele> - 1.6-2
> > - Fixed two hardcoded dirs in install section (Antonio Trande
> > <anto.trande>)
> 
> You don't need to indicate my Name/Surname/Mail. Please, don't do that.
> Hardcoded directory names must be avoided in the whole file.

Ok. I removed your name. The comment was imprecise, I had removed them
from everywhere.
 
> > - Added texlive-epstopdf-bin in Requires as socketstatistic requires it
> > - Added a patch to correct some help messages 
> > 
> > I've now git-ified the patch applying (a bit like in xorg-x11-server) so
> > rpmlint now needlessly warns about:
> > captcp.spec: W: patch-not-applied Patch0:
> > 0001-The-option-is-called-statistic-not-statistics.patch
> 
> Probably, it means that you have not applied the patch included in your
> source rpm. You can simply use the %patch command in the %setup section.
> See
> http://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25prep_section:_.
> 25patch_commands and
> http://fedoraproject.org/wiki/Packaging:
> Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

It's rpmlint that is unfortunately not smart enough, the patch has been
applied. Patch has been sent upstream but has not been applied there yet. 
I tend to avoid %patch because 'git am' fits nicely in my workflow and
brings the same result (like the x11 folks do)
I've added a comment about the patch.

> Remove the line 'rm -rf $RPM_BUILD_ROOT' in the %install section.

Ok, done.

> Why have you erased the '%{buildroot}/{_bindir}/captcp' file ?
> Leave a comment to explain your reasons. ;)

Ok, I've added a comment explaining this in the spec file directly.

Thanks for all your comments. Very appreciated.

regards,
Michele

So I went through an iteration of fedora-review -b 1025977 and fixed
a couple of things. New files:

Spec URL: http://acksyn.org/files/rpms/captcp/captcp.spec
SRPM URL: http://acksyn.org/files/rpms/captcp/captcp-1.6-3.fc20.src.rpm

I am not attaching the output of the review as I'm assuming that has to 
be done by someone else. If that is not the case, just let me know and I'll attach it.

regards,
Michele

(In reply to Michele Baldessari from comment #6)
> So I went through an iteration of fedora-review -b 1025977 and fixed
> a couple of things. New files:
> 
> Spec URL: http://acksyn.org/files/rpms/captcp/captcp.spec
> SRPM URL: http://acksyn.org/files/rpms/captcp/captcp-1.6-3.fc20.src.rpm
> 
> I am not attaching the output of the review as I'm assuming that has to 
> be done by someone else. If that is not the case, just let me know and I'll
> attach it.

It is not the case; when a sponsor will decide to sponsor you (I can't do that), then he/she will make an official review against a rawhide building.
Now, you can just "convince" a sponsor by following the dedicated guidelines (https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group). ;) 

However, you can already test your package in other builds like epel5/6 and especially in rawhide. See https://fedoraproject.org/wiki/Using_Mock_to_test_package_builds?rd=Extras/MockTricks.

(In reply to Michele Baldessari from comment #5)
> It's rpmlint that is unfortunately not smart enough, the patch has been
> applied. Patch has been sent upstream but has not been applied there yet. 
> I tend to avoid %patch because 'git am' fits nicely in my workflow and
> brings the same result (like the x11 folks do)
> I've added a comment about the patch.

I understand that you may prefer "git am", I prefer it too, but that's not how patches are applied in Fedora.  And there is a reason for that. Packages need to follow common practices.  If every package had its own way of doing everything then it would be hard to understand what they are doing and maintain them.  In other words, when submitting a package to Fedora sometimes you need to sacrifice some of your individual preferences in favor of distribution practices.

chmod calls in %install should be avoided, please use %attr macro in %files section instead.

You are packaging version 1.6, why not use this tarball?
http://github.com/hgn/%{name}/archive/v%{version}.tar.gz#/%{name}-%{version}.tar.gz
If you use it then you'll be able to get riod of commit and shortcommit macros as well as reduce %setup macro call to just "%setup -q".

You are mixing some macro styles: %SOURCE2 vs %{_bindir}.  Please either use %{SOURCE2} and %{_bindir} or %SOURCE2 and %_bindir (the first option if preferred).

After having a quick look I think that license tag should be "GPL+" instead of "GPLv3" as there is no explicit GPL version specified ("If the Program does not specify a version number of the GNU General Public License, you may choose any version ever published by the Free Software Foundation.").

There is some bundled MIT-licensed code in documentation.  It should either be removed or its licensing corrected (which means installing license file as documentation and adding "MIT" to license tag).

(In reply to Mikolaj Izdebski from comment #8)
> After having a quick look I think that license tag should be "GPL+" instead
> of "GPLv3" as there is no explicit GPL version specified ("If the Program
> does not specify a version number of the GNU General Public License, you may
> choose any version ever published by the Free Software Foundation.").

After more detailed licensing review it looks like there is GPLv2+ code too, making effective license tag "GPL+ and GPLv2+"

Hi Mikolaj,

(In reply to Mikolaj Izdebski from comment #8)
> I understand that you may prefer "git am", I prefer it too, but that's not
> how patches are applied in Fedora.  And there is a reason for that. Packages
> need to follow common practices.  If every package had its own way of doing
> everything then it would be hard to understand what they are doing and
> maintain them.  In other words, when submitting a package to Fedora
> sometimes you need to sacrifice some of your individual preferences in favor
> of distribution practices.

Makes sense. I've cleaned it up to use the traditional %patch approach.

> chmod calls in %install should be avoided, please use %attr macro in %files
> section instead.

Ack, done. Although without chmod in install I need to be more verbose in the
%files section. (i.e. To avoid a file being listed twice I need to list
files or directories explicitely). Not sure if there is a clever way around 
that?
 
> You are packaging version 1.6, why not use this tarball?
> http://github.com/hgn/%{name}/archive/v%{version}.tar.gz#/%{name}-%{version}.
> tar.gz
> If you use it then you'll be able to get riod of commit and shortcommit
> macros as well as reduce %setup macro call to just "%setup -q".

Ack, I've done so.

> You are mixing some macro styles: %SOURCE2 vs %{_bindir}.  Please either use
> %{SOURCE2} and %{_bindir} or %SOURCE2 and %_bindir (the first option if
> preferred).

Ack, I've changed that.

> After having a quick look I think that license tag should be "GPL+" instead
> of "GPLv3" as there is no explicit GPL version specified ("If the Program
> does not specify a version number of the GNU General Public License, you may
> choose any version ever published by the Free Software Foundation.").
> 
> There is some bundled MIT-licensed code in documentation.  It should either
> be removed or its licensing corrected (which means installing license file
> as documentation and adding "MIT" to license tag).

Let me double-check here with upstream because at least one icon seems in the
public domain as well and while we're at it, I'll ask for clarification about
the whole set of files. 

FWIW (while I sort out the license stuff with upstream):
Spec URL: http://acksyn.org/files/rpms/captcp/captcp.spec
SRPM URL: http://acksyn.org/files/rpms/captcp/captcp-1.6-4.fc20.src.rpm

regards,
Michele

(In reply to Michele Baldessari from comment #10)
> > chmod calls in %install should be avoided, please use %attr macro in %files
> > section instead.
> 
> Ack, done. Although without chmod in install I need to be more verbose in the
> %files section. (i.e. To avoid a file being listed twice I need to list
> files or directories explicitely). Not sure if there is a clever way around 
> that?

There is no harm in listing files more then once.  RPM will print a warning, but it can be ignored in most cases.  (If you really wanted to exclude some files then you could use %exclude macro, but it is not needed in this case.)

But first I would think if "chmod" or "%attr" is really needed and why.  If "make install" does not install files marked as executable then it's probably upstream bug.  If that's the case then you should report it.  Once it's fixed you'll be able to remove %attr macro.

Hi Mikolaj,

(In reply to Mikolaj Izdebski from comment #11)
> (In reply to Michele Baldessari from comment #10)
> > > chmod calls in %install should be avoided, please use %attr macro in %files
> > > section instead.
> > 
> > Ack, done. Although without chmod in install I need to be more verbose in the
> > %files section. (i.e. To avoid a file being listed twice I need to list
> > files or directories explicitely). Not sure if there is a clever way around 
> > that?
> 
> There is no harm in listing files more then once.  RPM will print a warning,
> but it can be ignored in most cases.  (If you really wanted to exclude some
> files then you could use %exclude macro, but it is not needed in this case.)

Ah ok, good to know.

> But first I would think if "chmod" or "%attr" is really needed and why.  If
> "make install" does not install files marked as executable then it's
> probably upstream bug.  If that's the case then you should report it.  Once
> it's fixed you'll be able to remove %attr macro.

Yes, I've chased this option as it makes the spec a bit cleaner and sent the one-liner upstream.

Once I get the feedback about the licensing questions, I'll submit the
latest round of spec+srpm

Thanks and regards,
Michele

(In reply to Michele Baldessari from comment #12)
> (In reply to Mikolaj Izdebski from comment #11)
> > But first I would think if "chmod" or "%attr" is really needed and why.  If
> > "make install" does not install files marked as executable then it's
> > probably upstream bug.  If that's the case then you should report it.  Once
> > it's fixed you'll be able to remove %attr macro.
> 
> Yes, I've chased this option as it makes the spec a bit cleaner and sent the
> one-liner upstream.

Perfect.

> Once I get the feedback about the licensing questions, I'll submit the
> latest round of spec+srpm

OK. Once you do that i'll do a full package review.

Hi Mikolaj,

so I've discussed with upstream a few things and the package should be more or
less ready at this point. Here are a couple of points that are worth noting and/or discussing:
- License Tag: I've put "GPLv2+ and MIT" as it seemed the most concise way to describe it. Even though for the vast majority *is* GPLv3 and there are only
a couple of exceptions:
data/stap-scripts/tcp-trace.stp - GPLv2+
data/connection-animation-data/raphael-min.js - MIT

I've got confirmation from upstream on the correctness of all the licenses and
also had the GPLv3 preamble added to the main python file.

- The two images' license has been clarified/changed:
1. data/connection-animation-data/images/old-computer.png 
CC AS Alike 3.0 Unported - https://en.wikipedia.org/wiki/File:Sperry-univac-uniscope-200-0a.jpg
2. data/connection-animation-data/images/router.png:
Public Domain - http://openclipart.org/detail/1918/router-by-juanjo

- I had to introduce the commit variables in the spec again as we need a commit
to get the properly licensed images.

- I've patched in the full MIT-LICENSE as upstream was not keen in adding it 
to the main git repo

- All the patches I had around locally have been applied

Both files are rpmlint -i -v clean:
Spec URL: http://acksyn.org/files/rpms/captcp/captcp.spec
SRPM URL: http://acksyn.org/files/rpms/captcp/captcp-1.7-1.fc20.src.rpm

regards,
Michele

I am currently on vacation [1] until 2013-12-01.
I'll continue this review when I come back.

[1] http://fedoraproject.org/wiki/Vacation

Updated packages:
Spec URL: http://acksyn.org/files/rpms/captcp/captcp.spec
SRPM URL: http://acksyn.org/files/rpms/captcp/captcp-1.7-2.fc20.src.rpm

- Fixed Licensing tag after Simo's comments
- Removed shell script and installed python executable directly in /usr/bin like
  rpmlint does

Looks good to me!


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

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "*No copyright* GPL (v3 or later)", "Unknown or generated". 2 files have
     unknown license. Detailed output of licensecheck in
     /home/simo/tmp/1025977-captcp/licensecheck.txt
[x]: If the package is under multiple licenses, the licensing breakdown must
     be documented in the spec.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[ ]: Package contains desktop file if it is a GUI application.
[ ]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[ ]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[ ]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 563200 bytes in 23 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: No rpmlint messages.
[x]: 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]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[ ]: 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]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[ ]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[ ]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[ ]: Package should compile and build into binary rpms on all supported
     architectures.
[ ]: %check is present and all tests pass.
[ ]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: No rpmlint messages.
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: captcp-1.7-2.fc21.noarch.rpm
          captcp-1.7-2.fc21.src.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint captcp
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
# echo 'rpmlint-done:'



Requires
--------
captcp (rpmlib, GLIBC filtered):
    /usr/bin/env
    /usr/bin/python
    gnuplot
    numpy
    python-dpkt
    systemtap



Provides
--------
captcp:
    captcp



Source checksums
----------------
https://github.com/hgn/captcp/archive/e73024063fd6c9cb73a325b92f316ee6fbfd0e56/captcp-1.7-e730240.tar.gz :
  CHECKSUM(SHA256) this package     : ea2b864edc59956184885457561c18ff2d34576f8cae29552972d9664c96d7e8
  CHECKSUM(SHA256) upstream package : ea2b864edc59956184885457561c18ff2d34576f8cae29552972d9664c96d7e8


Generated by fedora-review 0.5.0 (920221d) last change: 2013-08-30
Command line :/usr/bin/fedora-review -b 1025977 -m fedora-rawhide-x86_64
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api
Disabled plugins: Java, C/C++, Python, SugarActivity, Perl, R, PHP, Ruby
Disabled flags: EPEL5, EXARCH, DISTTAG

Sorry Mikolaj,
I did not realize a sponsor was needed for this review.
I think all is ok with the latest changes, can you please take back the review and proceed with approving it ?

Simo.

Greetings. 

Simo asked if I could take a look here and sponsor the submitter. 

I'll take a quick look at the package and do so unless I find any issues that need to be addressed first.

I've sponsored you into the packager group. Welcome to the fun!

You can continue the process from: 
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=PackageMaintainers/Join#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner


If you have any questions, please feel free to drop me an email or ask here or catch me on irc (nirik is my nick).

New Package SCM Request
=======================
Package Name: captcp
Short Description: TCP Analyzer for PCAP Files
Owners: mbaldessari
Branches: f20
InitialCC:

Git done (by process-git-requests).

captcp-1.7-2.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/captcp-1.7-2.fc20

captcp-1.7-2.fc20 has been pushed to the Fedora 20 testing repository.

captcp-1.7-2.fc20 has been pushed to the Fedora 20 stable repository.