Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1358959 - Review Request: nsntrace - Perform network trace of a program using network namespaces
Summary: Review Request: nsntrace - Perform network trace of a program using network n...
Keywords:
Status: ASSIGNED
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-NEEDSPONSOR
TreeView+ depends on / blocked
 
Reported: 2016-07-21 22:41 UTC by Jonas Danielsson
Modified: 2016-11-16 08:25 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
zbyszek: fedora-review?


Attachments (Terms of Use)

Description Jonas Danielsson 2016-07-21 22:41:16 UTC
Spec URL: http://threetimestwo.org/nsntrace/nsntrace.spec
SRPM URL: http://threetimestwo.org/nsntrace/nsntrace-1-1.fc23.src.rpm

Description: The nsntrace program uses Linux network namespaces to perform network traces of a single application. The traces are saved as pcap files. And can later be analyzed by for instance Wireshark.
Fedora Account System Username: jonasdn

This is my first package and I need a sponsor. I am the upstream author of this package. I am also the co-maintainer of GNOME Maps.

I did the koji thing:
http://koji.fedoraproject.org/koji/taskinfo?taskID=14971890

Comment 1 Igor Gnatenko 2016-07-22 06:40:33 UTC
> Source0:        https://github.com/jonasdn/nsntrace/releases/download/v1/nsntrace-1.tar.gz
Source0:        %{url}/releases/download/v%{version}/%{name}-%{version}.tar.gz

> make %{?_smp_flags}
> %make_build
those are same, use only one (second is better)

> %setup -q -n %{name}-%{version}
%setup and %autosetup automatically does "-n %{name}-%{version}", so just replace with "%autosetup -p0" and drop "%patch0 -p0".

> rm -rf $RPM_BUILD_ROOT
not needed since EL5(?)

> BuildRequires:  automake
> BuildRequires:  autoconf
you don't do autoreconf nor something like this, so you don't need those BRs

> %{_bindir}/*
Personally I don't like such things, it's too soft. Do something like %{_bindir}/%{name}

* Missing BuildRequires: gcc
* Check if dependencies used by pkg-config from autotools, if yes, replace dependencies with pkgconfig(MODULE)


This is just some short review.

Comment 2 Igor Gnatenko 2016-07-22 06:50:49 UTC
Suggest you to do something like this:
--- nsntrace.spec.orig	2016-07-22 08:42:06.024075138 +0200
+++ nsntrace.spec	2016-07-22 08:49:33.931017162 +0200
@@ -4,19 +4,17 @@
 Summary:        Perform network trace of a program by using network namespaces
 
 License:        GPLv2+
-URL:            https://github.com/jonasdn/nsntrace/
-Source0:        https://github.com/jonasdn/nsntrace/releases/download/v1/nsntrace-1.tar.gz
+URL:            https://github.com/jonasdn/nsntrace
+Source0:        %{url}/archive/v%{version}/%{name}-%{version}.tar.gz
 
-# Committed upstream as 67b1715f0c01b1cd97c9e9eab1d4d5586ec8bfed
-Patch1:         nsntrace-xsltproc-nonet.patch
+Patch1:         %{url}/commit/67b1715f0c01b1cd97c9e9eab1d4d5586ec8bfed.patch
 
-BuildRequires:  automake
-BuildRequires:  autoconf
+BuildRequires:  gcc
+BuildRequires:  automake autoconf libtool
 BuildRequires:  libpcap-devel
-BuildRequires:  libnl3-devel
-BuildRequires:  iptables
-BuildRequires:  libxslt
-BuildRequires:  docbook-style-xsl
+BuildRequires:  pkgconfig(libnl-route-3.0)
+BuildRequires:  libxslt docbook-style-xsl
+BuildRequires:  /usr/sbin/iptables
 
 %description
 The nsntrace program uses Linux network namespaces to perform network traces
@@ -24,25 +22,20 @@
 analyzed by for instance Wireshark.
 
 %prep
-%setup -q -n %{name}-%{version}
-
-%patch1 -p0
+%autosetup -p1
 
 %build
+autoreconf -vfi
 %configure
-make %{?_smp_flags}
 %make_build
 
-
 %install
-rm -rf $RPM_BUILD_ROOT
 %make_install
 
 %files
-%{_bindir}/*
-%{_mandir}/man1/*
 %license LICENSE
-
+%{_bindir}/%{name}
+%{_mandir}/man1/%{name}.1*
 
 %changelog
 * Thu Jul 21 2016 jonas <jonas@threetimestwo.org> - 1-1

Comment 3 Jonas Danielsson 2016-07-22 07:15:02 UTC
(In reply to Igor Gnatenko from comment #2)
> Suggest you to do something like this:
> --- nsntrace.spec.orig	2016-07-22 08:42:06.024075138 +0200
> +++ nsntrace.spec	2016-07-22 08:49:33.931017162 +0200
> @@ -4,19 +4,17 @@
>  Summary:        Perform network trace of a program by using network
> namespaces
>  
>  License:        GPLv2+
> -URL:            https://github.com/jonasdn/nsntrace/
> -Source0:       
> https://github.com/jonasdn/nsntrace/releases/download/v1/nsntrace-1.tar.gz
> +URL:            https://github.com/jonasdn/nsntrace
> +Source0:        %{url}/archive/v%{version}/%{name}-%{version}.tar.gz
>  
> -# Committed upstream as 67b1715f0c01b1cd97c9e9eab1d4d5586ec8bfed
> -Patch1:         nsntrace-xsltproc-nonet.patch
> +Patch1:         %{url}/commit/67b1715f0c01b1cd97c9e9eab1d4d5586ec8bfed.patch
>  
> -BuildRequires:  automake
> -BuildRequires:  autoconf
> +BuildRequires:  gcc
> +BuildRequires:  automake autoconf libtool
>  BuildRequires:  libpcap-devel
> -BuildRequires:  libnl3-devel
> -BuildRequires:  iptables
> -BuildRequires:  libxslt
> -BuildRequires:  docbook-style-xsl
> +BuildRequires:  pkgconfig(libnl-route-3.0)
> +BuildRequires:  libxslt docbook-style-xsl
> +BuildRequires:  /usr/sbin/iptables
>  
>  %description
>  The nsntrace program uses Linux network namespaces to perform network traces
> @@ -24,25 +22,20 @@
>  analyzed by for instance Wireshark.
>  
>  %prep
> -%setup -q -n %{name}-%{version}
> -
> -%patch1 -p0
> +%autosetup -p1
>  
>  %build
> +autoreconf -vfi
>  %configure
> -make %{?_smp_flags}
>  %make_build
>  
> -
>  %install
> -rm -rf $RPM_BUILD_ROOT
>  %make_install
>  
>  %files
> -%{_bindir}/*
> -%{_mandir}/man1/*
>  %license LICENSE
> -
> +%{_bindir}/%{name}
> +%{_mandir}/man1/%{name}.1*
>  
>  %changelog
>  * Thu Jul 21 2016 jonas <jonas@threetimestwo.org> - 1-1

Thank you so much for the review! Really appreciate it!
I will run this through mock and koji when I get home, and update the spec-file.

Jonas

Comment 4 Jonas Danielsson 2016-07-22 13:32:56 UTC
I have updated the SPEC file. And I also released a v2 of the application.

So the new version is 2, and the new SRPM is: https://threetimestwo.org/nsntrace/nsntrace-2-1.fc23.src.rpm

The spec file is at same uri: http://threetimestwo.org/nsntrace/nsntrace.spec


Koji run here:
http://koji.fedoraproject.org/koji/taskinfo?taskID=14979582


Thanks!

Comment 5 Zbigniew Jędrzejewski-Szmek 2016-11-13 01:46:22 UTC
SRPM link gives 404.

nsntrace.x86_64: E: incorrect-fsf-address /usr/share/licenses/nsntrace/LICENSE

It's nicer to put each BuildRequires on a separate line (better legibility and diffability).

The package is nice and simple, really pleasant to review.
+ latest version
+ license is acceptable (GPLv2+)
+ license is specified correctl
+ builds and installs OK
+ provides/requires look correct

I'll approve the package once you are added to the packagers group.

--


I can sponsor you into the packagers group. In addition to the package that you are submitting, I require two-three reviews of other packages (see http://fedoraproject.org/PackageReviewStatus/NEW.html). There's plenty of packages awaiting review, so you should be able to find something interesting without any trouble. In your review, please indicate that you are not a packager yet, hence the review is unofficial. If nobody beats you to it, you'll be able to finalize those reviews after you become a packager (hopefully soon ;)). Building in mock and/or running fedora-review and carefully looking at the output is a good start.


Note You need to log in before you can comment on or make changes to this bug.