Bug 691027 (n2n)

rpmlint result:
$ rpmlint /home/rpmbuild/SRPMS/n2n-2.1.0-1.el6.src.rpm /home/rpmbuild/RPMS/i686/n2n-2.1.0-1.el6.i686.rpm SPECS/n2n.spec 
n2n.src: I: enchant-dictionary-not-found en_US
n2n.src: W: invalid-url Source0: n2n-2.1.0.tar.gz
SPECS/n2n.spec: W: invalid-url Source0: n2n-2.1.0.tar.gz
2 packages and 1 specfiles checked; 0 errors, 2 warnings.

The "invalid-url" is the case decribed in https://fedoraproject.org/wiki/Packaging/SourceURL, Using Revision Control.
"enchant-dictionary-not-found" is because I dont have spell check libs installed on my local system.

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

Package: 

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec
 [x] Package meets the Packaging Guidelines
 [x] Package successfully compiles and builds into binary RPMs on at least one
supported architecture
     Tested on: F14/x86_64
 [x] Rpmlint output:
     Source RPM:
     [fab@laptop023 SRPMS]$ rpmlint n2n-2.1.0-1.el6.src.rpm
     n2n.src: W: spelling-error %description -l en_US https -> HTTP, hatpins, hotpots
     n2n.src: W: invalid-url Source0: n2n-2.1.0.tar.gz
     1 packages and 0 specfiles checked; 0 errors, 2 warnings.
     Binary RPM(s):
     [fab@laptop023 x86_64]$ rpmlint n2n*
     n2n.x86_64: W: spelling-error %description -l en_US https -> HTTP, hatpins, hotpots
     2 packages and 0 specfiles checked; 0 errors, 1 warnings.
 [x] Package is not relocatable
 [x] Buildroot is correct (if it's still used)
     master   : %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
     spec file: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 [x] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
 [!] License field in the package spec file matches the actual license
     License type: GPLv3+
 [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] Spec file is legible and written in American English
 [?] Sources used to build the package matches the upstream source, as provided
in the spec URL
     Upstream source: 6da99a853dd128ee3526139dde57e535  n2n-2.1.0.tar.gz
     Build source:    6f05cbc045978f5978ea5b8eaa129084  n2n-2.1.0.tar.gz
 [x] Package is not known to require ExcludeArch
 [-] Architecture independent packages have: BuildArch: noarch
 [-] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.  %find_lang used for locales
 [!] %{optflags} or RPM_OPT_FLAGS are honoured
 [-] ldconfig called in %post and %postun if required
 [x] %install starts with rm -rf %{buildroot} or $RPM_BUILD_ROOT (if it's still used)
 [-] Package must own all directories that it creates
 [-] Package requires other packages for directories it uses
 [x] Package does not own files or directories owned by other packages
 [x] Package does not contain duplicates in %files
 [x] Permissions on files are set properly. %defattr(-,root,root,-) is in every %files section
 [x] Package uses nothing in %doc for runtime
 [x] Package has a %clean section, which contains rm -rf %{buildroot} or $RPM_BUILD_ROOT (if it's still used)
 [-] Included tests passed successfully 
 [x] Package consistently uses macros
 [x] Package contains code, or permissable content
 [x] Included filenames are in UTF-8

 [-] Large documentation files are in a -doc subpackage, if required
 [-] Header files (.h) in -devel subpackage, if present
 [-] Fully versioned dependency in subpackage, if present
 [-] Static libraries (.a) in -static subpackage, if present
 [-] Package requires pkgconfig, if .pc files are present
 [-] Development .so files in -devel subpackage, if present
 [x] Package does not contain any libtool archives (.la)
 [x] -debuginfo subpackage is present and looks complete
 [x] No pre-built binaries (.a, .so*, executable)
 
 [-] Package contains a properly installed .desktop file if it is a GUI application
 [-] Follows desktop entry spec
 [-] Valid .desktop Name
 [-] Valid .desktop GenericName
 [-] Valid .desktop Categories
 [-] Valid .desktop StartupNotify
 [-] .desktop file installed with desktop-file-install in %install

=== SUGGESTED ITEMS ===
 [!] Timestamps preserved with cp and install
 [x] Uses parallel make (%{?_smp_mflags})
 [x] Latest version is packaged
 [x] Package does not include license text files separate from upstream
 [x] Description and summary sections in the package spec file contains
translations for supported Non-English languages, if available
 [x] Package should compile and build into binary RPMs on all supported
architectures.
     Tested:  http://koji.fedoraproject.org/koji/taskinfo?taskID=2948473
 [x] Package functions as described
 [-] Scriptlets must be sane, if used
 [-] The placement of pkgconfig(.pc) files is correct
 [-] File based requires are sane
 [x] Changelog in allowed format

- The source header claims that the license is GPLv3+.
- Please preserve the timestamps https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps
- Why not combine the man pages in the %files section? In the future you do not need to take care about new man pages.

Please follow http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

Thanks Fabian, will fix and repost.

Hi Fabian,
I fixed license tag and source tallbar, please review, thanks!

SPEC URL: http://dl.dropbox.com/u/24432462/n2n.spec
SRPM URL: http://dl.dropbox.com/u/24432462/n2n-2.1.0-2.el6.src.rpm

scratch build:

https://koji.fedoraproject.org/koji/taskinfo?taskID=2956057

Hi, I just do a informal review:
https://bugzilla.redhat.com/show_bug.cgi?id=691153#c2
Are you willing to be my sponsor?

Thanks,
Hushan

Fabian, are you doing a formal review and sponsoring? Just checking, because the review flag is still not set. Thanks.

Well, I'm not a sponsor. So the review is informal.

Thanks; taking it then

Hi Michel, I did an informal review for libopkele review request:

https://bugzilla.redhat.com/show_bug.cgi?id=691597#c5


Thanks,
Hushan

Hi Hushan,

Looking at the package now. Some questions; I'll continue reviewing afterwards. Thanks for doing the informal reviews -- it helps packagers when they do the full review. That package looks neat, so I'll actually review it and check on your informal review at the same time.

I'll continue this review after the issues below are addressed. Thanks!

=== Source ===

- any reason you're tracking SVN trunk? The n2n-2.0.1 tag might be a better bet:
  https://svn.ntop.org/svn/ntop/tags/n2n-2.0.1/

- rather than checking out using Subversion and then using a patched version of
  upstream's script for packaging, I'd suggest checking out using git's svn
  adapter. I find `svn export` frustrating as it exports a directory, and thus
  the file ordering is unspecified if you then directly use tar. The files are
  added in the order that the filesystem provides the directory listing, and even
  for the same SVN revision this is not the same!

  (note that even with trunk, the last revision for n2n is 4444, not 4541 -- the
   later revisions do not touch n2n at all)

    git svn clone --no-minimize-url https://svn.ntop.org/svn/ntop/tags/n2n-2.0.1
    (cd n2n-2.0.1 && git archive HEAD --format=tar --prefix=n2n-2.0.1/) | bzip2 - > n2n-2.0.1.tar.bz2
    

  In an ideal world, upstream has this layout:

    /svn/n2n/{trunk,branches,tags}

  that way one can just do this:

    git svn clone --no-minimize-url -s https://svn.ntop.org/svn/n2n

  and then keep track of upstream developments by pulling in later SVN
  revisions:
    git svn rebase

  rather than checking out a tag whenever it is created. Their current SVN
  layout is rather messy.

  Could you perhaps ask them to also release n2n tarballs? This does not have
  to delay this review, but would make maintaining this package easier.

=== Optimization flags ===
Fabian raised this issue, which you have not addressed. Since this package does
not use the standard %configure, you'd have to override CFLAGS manually when building. e.g.

make %{?_smp_mflags} CFLAGS=%{optflags}

Hi Michel,
Thanks a lot for the review, a question about source archive is that there is quite a lot update in the trunk version already, so should we package the 2.0.1 tag version, and then pull the changes into it using patchset, or put the changes into the source rpm before the initial packaging?

Thanks,
Hushan

Hi Hushan,

I am not personally familiar with n2n, and I assume you are, so it's up to you. If you want to track the trunk branch, though, since it's not tagged 2.1.0 yet, you have to follow the guidelines for pre-release versions for the release tag:

  http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages

  Release Tag for Pre-Release Packages: 0.%{X}.%{alphatag}

in this case it would be something like

  n2n-2.1.0-0.1.some-svn-tag here

The packaging guideline recommends putting the date there, but I never see it stated as an explicit requirement. It probably dates back to CVS days where each file tracked in CVS has is own version number!

I'd recommend also adding the SVN revision to this. See e.g. the nouveau driver:

  $ rpm -q xorg-x11-drv-nouveau
  xorg-x11-drv-nouveau-0.0.16-24.20110324git8378443.fc15.x86_64
                                 ^ date ^   ^ commit ID ^

I'd recommend using the SVN revision number instead, so:

  git svn clone --no-minimize-url https://svn.ntop.org/svn/ntop/trunk/n2n/n2n_v2
  git log -1 # show last commit -> Jan 31, revision 4444
             # wow, that's not very auspicious in some culture's numerology!
  thus the N-V-R will be

    n2n-2.1.0-0.1.20110131svn4444%{?dist}

by tracking trunk, you can just do a 'git svn rebase' to pull in later changes.

When 2.1.0 is tagged, you can either do another 'git svn clone' for that tag, as described in the previous commit, or just, from within the Git clone, checkout the revision corresponding to the tag:

  git checkout $(git svn find-rev rXXXX)

and then do the `git archive` as described before. The release tag for a released version will be of the normal form -- %{X}%{?dist} -- no need
to include the SVN revision (but if, say, upstream has a branch for bugfixes that you're tracking, and release patches without bumping the release number, then it's like the pre-releases but without the '0.' prefix.

A bit complicated, but whenever I see a Scala package on Maven, I shudder at their horrible versioning practice -- in Fedora we try to make sure a package's NVR is always monotonically increasing, allowing smooth upgrades (when that failed for some reason -- upstream renaming their release scheme, for instance, that's when you see packages with 'Epoch' numbers. Those are super-versions)

Hi Michel, Thanks, I will package 2.0.1 tagged version instead of trunk, I will communication with n2n upstream to see when will 2.1.0 released.

Hi Michel,

SPEC url:
http://dl.dropbox.com/u/24432462/n2n.spec
SRPM url:
http://dl.dropbox.com/u/24432462/n2n-2.0.1-1.el6.src.rpm

Please review, thanks.

koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2971179

Hi Hushan,

Almost done! Several small niggling issues:
- description is not properly indented. If you use Emacs, adding an empty line
  between %description and the first line of the text, and then putting your
  cursor anywhere on the text and pressing Esc-q should fix it; then remove the
  extra line

- rename https to HTTPS in the description

- also, are you planning to build for RHEL? If not, there are some items that
  can be removed:
  - BuildRoot declaration
  - %clean section
  - the cleaning of $RPM_BUILD_ROOT at the beginning of %install

(see review below for details) 
Everything else checks out; post an updated build and I'll approve it and sponsor you. Do you already have a Fedora Account? Let me know your username.


#+TODO: TODO(t) WAIT(w@/!) FAIL(f@) | DONE(d) N/A(n)

* TODO Review [90%]
  - [X] Names [2/2]
    - [X] Package name
    - [X] Spec name
  - [X] Package version [2/2]
	http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Package_Versioning
      - [X] Version number
	    http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Version_Tag
      - [X] Release tag
	    http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Release_Tag
	    http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Pre-Release_packages
  - [X] Meets [[http://fedoraproject.org/wiki/Packaging/Guidelines][guidelines]]
  - [X] Source files match upstream
	$ sha1sum n2n-2.0.1.tar.bz2 ~/rpmbuild/SOURCES/n2n-2.0.1.tar.bz2 
	1a7f87c6f7434220fa60a700818a4c3c9220c276  n2n-2.0.1.tar.bz2
	1a7f87c6f7434220fa60a700818a4c3c9220c276  /home/michel/rpmbuild/SOURCES/n2n-2.0.1.tar.bz2
  - [X] [[http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries][No bundled libraries]]
  - [X] License [4/4]
    - [X] License is Fedora-approved
    - [X] No licensing conflict
    - [X] License field accurate
    - [X] License included iff packaged by upstream
  - [X] rpmlint [2/2]
    - [X] on src.rpm
	  n2n.src: W: spelling-error %description -l en_US https -> HTTP
	  => I'd suggest spelling it "HTTPS"
          n2n.src: W: invalid-url Source0: n2n-2.1.0.tar.bz2
	  => ignore this; SVN checkout
          1 packages and 0 specfiles checked; 0 errors, 2 warnings.

    - [X] on x86_64.rpm
	  n2n.x86_64: W: spelling-error %description -l en_US https -> HTTP
	  2 packages and 0 specfiles checked; 0 errors, 1 warnings.
  - [-] Language & locale [2/3]
    - [X] Spec in US English
    - [-] Spec legible
    - [X] Use %find_lang to handle locale files
	  N/A
  - [X] Build [3/3]
    - [X] Koji results
    - [X] BRs complete
    - [X] Directory ownership
  - [X] Spec inspection [8/8]
    - [X] No duplicate files
    - [X] File permissions
    - [X] Filenames must be UTF-8
    - [X] no BuildRoot ([[https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag][except if targeting EPEL5]])
    - [X] No %clean section
	  (except RHEL:
	  https://fedoraproject.org/wiki/Packaging/Guidelines#.25clean)
    - [X] RHEL: %buildroot cleaned on %install
    - [X] Macro usage consistent
    - [X] Documentation [1/1]
      - [X] %doc files are non-essential

please package it for RHEL too (EL-5 and EL-6)

Hi Michel,
Thanks for review and sponsor, I would like to build for epel as well.
My fedora account is hushan.

I have updated the indentation and spelling of description, the update spec and source rpm are:
http://dl.dropbox.com/u/24432462/n2n.spec
http://dl.dropbox.com/u/24432462/n2n-2.0.1-2.el6.src.rpm
http://dl.dropbox.com/u/24432462/n2n-2.0.1-2.el5.src.rpm (for el5)

koji scratch builds:
F15
http://koji.fedoraproject.org/koji/taskinfo?taskID=2974287
epel6
http://koji.fedoraproject.org/koji/taskinfo?taskID=2974296
epel5
http://koji.fedoraproject.org/koji/taskinfo?taskID=2974304

Thanks,
Hushan

Splendid. APPROVED and SPONSORED -- oh, I forgot to mention this during the review: you want to avoid explicit Requires: as much as possible.

In this case, if you do `rpm -qp --requires <the-binary-rpm>` you'll see that it already depends on libcrypto, which is provided by openssl, so you don't need to manually Requires: it (odd that rpmlint didn't complain).

Sometimes you do need it -- e.g. you want to tie down a particular version of the library you depend on, normally when upstream notoriously break ABI without bumping the soname (Mozilla's xulrunner is a particularly bad example).

No need to show me the fix though, just make it when you commit. Feel free to ask me to review your next couple of packages, and please Cc: me on this and the next couple of packages you maintain, just in case I can be of assistance.

New Package SCM Request
=======================
Package Name: n2n
Short Description: n2n is a layer-two peer-to-peer virtual private network (VPN) which allows users to exploit features typical of P2P applications at network instead of application level.
Owners: hushan
Branches: f15 el5 el6
InitialCC: michel+fdr

New Package SCM Request
=======================
Package Name: n2n
Short Description: n2n is a layer-two peer-to-peer virtual private network
(VPN) which allows users to exploit features typical of P2P applications at
network instead of application level.
Owners: hushan
Branches: f15 el5 el6
InitialCC: salimma

Hi sorry, it should be fas name in InitialCC field, I just copied it wrongly :(

Git done (by process-git-requests).

Thanks Fabian for review, thanks Michel for review and sponsor, thanks Jason for process. :)

n2n-2.0.1-3.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/n2n-2.0.1-3.fc15

n2n-2.0.1-3.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/n2n-2.0.1-3.el6

n2n-2.0.1-3.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/n2n-2.0.1-3.el5

Hi Itamar, 
The packages are now in el5 and el6, you can test them, also please add a feedback if it works, so that we can make it into stable release.

Thanks,
Hushan

n2n-2.0.1-3.el6 has been pushed to the Fedora EPEL 6 testing repository.

n2n-2.0.1-3.fc15 has been pushed to the Fedora 15 stable repository.

Package Change Request
======================
Package Name: n2n
New Branches: f13 f14
Owners: hushan

Git done (by process-git-requests).

n2n-2.0.1-3.el6 has been pushed to the Fedora EPEL 6 stable repository.

n2n-2.0.1-3.el5 has been pushed to the Fedora EPEL 5 stable repository.

n2n-2.0.1-3.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/n2n-2.0.1-3.fc14

n2n-2.0.1-3.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/n2n-2.0.1-3.fc13

n2n-2.0.1-3.fc14 has been pushed to the Fedora 14 stable repository.

n2n-2.0.1-3.fc13 has been pushed to the Fedora 13 stable repository.