Bug 472683 (jpcap) - Review Request: jpcap - Packet capturing library for Java
Summary: Review Request: jpcap - Packet capturing library for Java
Keywords:
Status: CLOSED NEXTRELEASE
Alias: jpcap
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-11-23 12:14 UTC by Patrick Dignan
Modified: 2009-06-16 01:59 UTC (History)
4 users (show)

Fixed In Version: 0.7-6.fc9
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-01-26 15:24:28 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Patrick Dignan 2008-11-23 12:14:53 UTC
Spec URL: http://users.wpi.edu/~dignan/jpcap.spec
SRPM URL: http://users.wpi.edu/~dignan/jpcap-0.7-3.fc10.src.rpm
Description: Jpcap is a Java library for capturing and
sending network packets from Java applications.
This Jpcap package requires Java 1.6 or higher 
and libpcap 0.9 or higher.

This is my first package, so I am looking for a sponsor.

Comment 1 Patrick Dignan 2008-11-23 12:24:50 UTC
I'd like to note that this library was written by Keita Fujii (see the URL in the spec file), and I repackaged it to conform with Fedora's standards.

Comment 2 Mamoru TASAKA 2009-01-05 18:25:39 UTC
Well:
- First of all, your srpm doesn't build on F-11:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=1031148

- Please explain why you want to define %jpcap, %jpcap_version

- For license tag:
  - No version of LGPL is specified in the codes
  - However some files are licensed under BSD with advertising
    (and BSD)
  So the license tag should be "LGPLv2+ and BSD with advertising"

- Source must be given by full URL:
  https://fedoraproject.org/wiki/Packaging/SourceURL

- Please remove Vendor item. This is defined automatically when
  rebuilding your srpm on Fedora buildsystem.

- For BuildRequires:
  * Please explicitly write "BuildRequires: jpackage-utils" to
    honor Java packaging guidelines (even if java-devel implicitly
    pulls in jpackage-utils dependency) (same for Requires)

  * "BuildRequires: libpcap" should be removed as libpcap-devel always
    requires it.

- Please explain why you set "Autoreq: 0".

- Remove all pre-compiled binaries (like foo.jar) at %prep
  stage to make it sure that all binaries are compiled from the
  source.

- We now recommend %defattr(-,root,root,-)

- The directory %{_libdir}/%{name} is not owned by any packages (ref:
  https://fedoraproject.org/wiki/Packaging/Guidelines#File_and_Directory_Ownership
  https://fedoraproject.org/wiki/Packaging/UnownedDirectories )

Comment 3 Patrick Dignan 2009-01-06 21:42:49 UTC
Ok, updated the package.  

- I tested it in mock for fedora-devel-i386, and it works fine now, I hadn't set the location for jni.h properly for the libjpcap.so to build.

- I had those set because I saw it in an example, but it seems quite useless in retrospect, since I can just use %name and %version

- Fixed the licensing issue

- Changed to full upstream URL

- Removed the Vendor item

- Added jpackage-utils to both BuildRequires and Requires

- Removed libpcap as a BuildRequire

- I used "Autoreq: 0" because upstream had done so in their RPM spec file (which was nowhere near current Fedora spec)

- Added an rm -f statement to the %prep which removes the jarfiles and .so pre-built in the upstream tarball

- Changed to %defattr(-,root,root,-)

- Now uses install -D which should fix the permissions


I uploaded new versions here:

Spec File: http://users.wpi.edu/~dignan/jpcap.spec
Source RPM: http://users.wpi.edu/~dignan/jpcap-0.7-4.fc10.src.rpm

The originals are now moved to http://users.wpi.edu/~dignan/archive/0.7-3/jpcap.spec and http://users.wpi.edu/~dignan/archive/0.7-3/jpcap-0.7-3.fc10.src.rpm

Thank you for the review, I look forward to your forthcoming comments.

Comment 4 Mamoru TASAKA 2009-01-08 18:29:34 UTC
For 0.7-4:

* SourceURL
  - I recommend to use %{name} and %{version}, especially
    %{version}. With this you probably won't have to change
    the source URL when version is updated:
    https://fedoraproject.org/wiki/Packaging/SourceURL#Using_.25.7Bversion.7D

* Tarball
  - By the way the tarball I could download from the Source URL
    differs from what in your srpm:
--------------------------------------------------------------
716370 2007-06-12 03:27 downloaded/jpcap-0.7.tar.gz
715154 2009-01-06 15:51 jpcap-0.7-4.fc10.src/jpcap-0.7.tar.gz
--------------------------------------------------------------
    (If there is CRLF line terminators difference, please don't
     fix this in advance but fix CRLF terminators at %prep)

* Requires
  - "Requires: libpcap" is not needed.
    rpmbuild detects libraries related dependencies automatically
    (in this case libpcap.so.0.9 on F-11) and adds the dependencies
    to binary rpms.

* JAVA_HOME
  - There is %java_home macro and you can use it
  - By the way you may want to use %ant macro instead
    (please try $ rpm --eval %ant)

* Build failure/optflags
  - Your srpm fails to build on x86_64:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=1038602
    This is because C codes are not compiled with -fPIC.

    Also Fedora specific compilation flags are not correctly
    honored:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags

    For this package
---------------------------------------------------------------
make %{?_smp_mflags} CC="gcc %{optflags} -fPIC"
---------------------------------------------------------------
    seems to work.

* Don't strip binaries
  - Please don't strip binaries. Debug information is required
    to create debuginfo rpm correctly (i.e. rpmbuild properly
    handles this: also please make sure that binaries are compiled
    with -g -- %{optflags} contains this)

* Directory ownership issue
  - The directory %{_libdir}/%{name} itself is not yet owned
    by this package.

Comment 5 Mamoru TASAKA 2009-01-08 18:37:54 UTC
* rpmlint
  - Also fix these:
-------------------------------------------------------------
W: wrong-file-end-of-line-encoding /usr/share/javadoc/jpcap/javadoc/package-list
W: wrong-file-end-of-line-encoding /usr/share/javadoc/jpcap/javadoc/stylesheet.css
-------------------------------------------------------------

Comment 6 Patrick Dignan 2009-01-08 22:54:40 UTC
Alright!  Fixed all of those problems, and tested using koji on F10 and F11 (I didn't know I had the permissions to do so!)

Uploaded new source rpm and new SPEC file to:
http://users.wpi.edu/~dignan/jpcap-0.7-5.fc10.src.rpm and
http://users.wpi.edu/~dignan/jpcap.spec

Moved the old versions to an archive (lost the jpcap spec file to 0.7-3 somehow), and symlinked the source rpm files

jpcap-0.7-3.fc10.src.rpm can be found:
http://users.wpi.edu/~dignan/jpcap-0.7-3.fc10.src.rpm or
http://users.wpi.edu/~dignan/archive/jpcap/0.7-3/jpcap-0.7-3.fc10.src.rpm


jpcap-0.7-4.fc10.src.rpm can be found:
http://users.wpi.edu/~dignan/jpcap-0.7-4.fc10.src.rpm or
http://users.wpi.edu/~dignan/archive/jpcap/0.7-4/jpcap-0.7-4.fc10.src.rpm

jpcap-0.7-4 spec file can be found:
http://users.wpi.edu/~dignan/archive/jpcap/0.7-4/jpcap.spec

Comment 7 Mamoru TASAKA 2009-01-09 16:52:31 UTC
Okay, now:

-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few (or no) work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------

Comment 8 Patrick Dignan 2009-01-10 11:01:26 UTC
I Did an informal review here: https://bugzilla.redhat.com/show_bug.cgi?id=478412

I am now looking into packaging the program Jailkit, which is on the wishlist, and seems to be a pretty useful tool.

Comment 9 Patrick Dignan 2009-01-11 04:07:52 UTC
Jailkit is packaged and up for review here:
https://bugzilla.redhat.com/show_bug.cgi?id=479546

Comment 10 Mamoru TASAKA 2009-01-14 17:32:18 UTC
Okay,
- For your pre-review for bug 478412, about compiler flags:
  * %configure macro sets CFLAGS environment value
    (you can check what %configure does by "$ rpm --eval %configure").
    Usually when using configure/Makefile generated by recent
    autotools this $CFLAGS value is correctly honored.

    However when using some old configure/Makefile, which are
    originally written by the upstream developer sometimes
    these files does not correctly catch $CFLAGS. In such
    cases you may have to use "CC='gcc %{optflags}'", for example.

- For jailkit, packaging itself seems almost good (I wrote some
  comments)

----------------------------------------------------------
    This package (jpcap) is APPROVED by mtasaka
----------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Install the Client Tools (Koji) ".

Now I am sponsoring you.

If you want to import this package into Fedora 8/9, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Removing NEEDSPONSOR.

Comment 11 Patrick Dignan 2009-01-15 03:17:58 UTC
New Package CVS Request
=======================
Package Name: jpcap
Short Description: Packet capturing library for Java
Owners: dignan
Branches: F-9 F-10
InitialCC: dignan mtasaka

Comment 12 Patrick Dignan 2009-01-15 04:26:22 UTC
Thank you for the sponsorship and review Mr. Tasaka.  I understand now when the %{optflags} tag is needed, have made the appropriate changes to Jailkit, and have taken the appropriate next steps for this package.

Thank you very much.

Comment 13 Kevin Fenzi 2009-01-15 20:21:24 UTC
cvs done.

Comment 14 Mamoru TASAKA 2009-01-25 07:53:24 UTC
Please rebuild this package on koji and for F-10/9
submit requests to push the rebuilt packages into repositories.

Comment 15 Patrick Dignan 2009-01-25 08:35:38 UTC
Sorry, experiencing some hardware difficulties with my main computer (which runs Fedora), so I've had some downtime.  I should hopefully be back up tomorrow, as I hadn't had time to repair it over the week and reinstall.  First on my TODO list is getting this built and pushed.

Comment 16 Patrick Dignan 2009-01-26 06:28:20 UTC
Built in Koji for F9, F10, and devel.  Submitted bodhi update requests for F-9 and F-10.

Thanks for all the help!

Comment 17 Mamoru TASAKA 2009-01-26 15:24:28 UTC
Okay, now closing, thank you.

Comment 18 Fedora Update System 2009-06-16 01:27:22 UTC
jpcap-0.7-6.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 19 Fedora Update System 2009-06-16 01:59:09 UTC
jpcap-0.7-6.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.


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