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: CLOSED INSUFFICIENT_DATA
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: 2019-09-17 15:39 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-09-17 15:39:15 UTC
Type: ---
Embargoed:
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> - 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> - 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.

Comment 6 Zbigniew Jędrzejewski-Szmek 2019-09-17 15:39:15 UTC
Let's close this. Feel free to restart the process at any time.


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