Bug 670451 - Review Request: async-http-client - Asynchronous Http Client for Java
Summary: Review Request: async-http-client - Asynchronous Http Client for Java
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: David Nalley
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 670164
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-01-18 10:47 UTC by Stanislav Ochotnicky
Modified: 2011-01-31 11:54 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2011-01-31 11:52:09 UTC
Type: ---
Embargoed:
david: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Stanislav Ochotnicky 2011-01-18 10:47:47 UTC
Spec URL: http://sochotni.fedorapeople.org/packages/async-http-client.spec
SRPM URL: http://sochotni.fedorapeople.org/packages/async-http-client-1.4.1-1.fc14.src.rpm


Description: 
Async Http Client library purpose is to allow Java applications to
easily execute HTTP requests and asynchronously process the HTTP
responses. The Async HTTP Client library is simple to use.

Comment 1 David Nalley 2011-01-23 19:07:56 UTC
Stanislav: 

I don't proclaim to be anything other than a nascent java package reviewer (though I am trying to fix that by spending more time reviewing java packages) 

However, when I grabbed this to start working on it I run into the following problem: 

[ke4qqq@L1012001 SPECS]$ rpmbuild -ba async-http-client.spec 
error: Failed build dependencies:
        maven is needed by async-http-client-1.4.1-1.fc14.noarch
        maven-surefire-plugin is needed by async-http-client-1.4.1-1.fc14.noarch
        sonatype-oss-parent is needed by async-http-client-1.4.1-1.fc14.noarch
        netty is needed by async-http-client-1.4.1-1.fc14.noarch


So I think maven should really be maven2 
and maven-surefire-plugin should really be maven-surefire-maven-plugin 
sonatype-oss-parent has been approved, but not built (add dependency in the ticket on that) 
netty I can't seem to find a review request for or a package in the repos. 

thanks

Comment 2 Alexander Kurtakov 2011-01-24 05:14:20 UTC
(In reply to comment #1)
> Stanislav: 
> 
> I don't proclaim to be anything other than a nascent java package reviewer
> (though I am trying to fix that by spending more time reviewing java packages) 
> 
> However, when I grabbed this to start working on it I run into the following
> problem: 
> 
> [ke4qqq@L1012001 SPECS]$ rpmbuild -ba async-http-client.spec 
> error: Failed build dependencies:
>         maven is needed by async-http-client-1.4.1-1.fc14.noarch
>         maven-surefire-plugin is needed by
> async-http-client-1.4.1-1.fc14.noarch
>         sonatype-oss-parent is needed by async-http-client-1.4.1-1.fc14.noarch
>         netty is needed by async-http-client-1.4.1-1.fc14.noarch
> 
> 
> So I think maven should really be maven2 
> and maven-surefire-plugin should really be maven-surefire-maven-plugin 
> sonatype-oss-parent has been approved, but not built (add dependency in the
> ticket on that) 
> netty I can't seem to find a review request for or a package in the repos. 
> 
Hi David,
Stanislav's version fully shows his intentions - maven package is maven version 3.x and it is available in Rawhide only, maven-surefire-plugin is the new name of the plugin in rawhide which properly obsoletes the current one and fixes a long standing crazy name :). 
Here is the netty build http://koji.fedoraproject.org/koji/buildinfo?buildID=214608 and netty review request https://bugzilla.redhat.com/show_bug.cgi?id=669394 . 

This package is not intended to build on anything but rawhide so feel free to continue your review :). Btw, you're more than welcome helping us with the reviews - drop me a mail or ask on irc if you have some questions about our java packages.

Regards, 
Alex

> thanks

Comment 3 Stanislav Ochotnicky 2011-01-25 12:13:33 UTC
David,

as Alexander wrote, package is intended for rawhide only. Therefore if you want to test it you'll have to either have rawhide installation somewhere (virtual machine) or use mock. 

Sonatype review is done, there is already built in rawhide (should be in buildroot shortly), so this package should build without problems for you. You can start the review then. FYI we need this package to update maven 3 to latest version.

Comment 4 David Nalley 2011-01-26 12:52:36 UTC
Sorry it's taken me so long to circle back around to this. review follows. 


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

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

=== REQUIRED ITEMS ===
[ ]  Rpmlint output:
[ke4qqq@thulsadoom SPECS]$ rpmlint ../SRPMS/async-http-client-1.4.1-1.fc15.src.rpm ../RPMS/noarch/async-http-client-
async-http-client-1.4.1-1.fc15.noarch.rpm          async-http-client-javadoc-1.4.1-1.fc15.noarch.rpm  
[ke4qqq@thulsadoom SPECS]$ rpmlint ../SRPMS/async-http-client-1.4.1-1.fc15.src.rpm ../RPMS/noarch/async-http-client-* ./async-http-client.spec 
async-http-client.src: W: invalid-url Source0: AsyncHttpClient-async-http-client-async-http-client-1.4.1-0-g5ce359e.tar.gz
async-http-client.noarch: W: non-conffile-in-etc /etc/maven/fragments/async-http-client
./async-http-client.spec: W: invalid-url Source0: AsyncHttpClient-async-http-client-async-http-client-1.4.1-0-g5ce359e.tar.gz
3 packages and 1 specfiles checked; 0 errors, 3 warnings.
[X]  Package is named according to the Package Naming Guidelines[1].
[X]  Spec file name must match the base package name, in the format %{name}.spec.
[X]  Package meets the Packaging Guidelines[2].
[!]  Package successfully compiles and builds into binary rpms.
Missing a BR of maven-shade-plugin
[X]  Buildroot definition is not present
[X]  Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines[3,4].
[X]  License field in the package spec file matches the actual license.
License type:
[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]  All independent sub-packages have license of their own
[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. - Due to github generating this dynamically - impossible to really test. 
MD5SUM this package    :
MD5SUM upstream package:
[!]  All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines[5].
See above note. 
[X]  Package must own all directories that it creates.
[-]  Package requires other packages for directories it uses.
[X]  Package does not contain duplicates in %files.
[X]  Permissions on files are set properly.
[X]  Package does NOT have a %clean section which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). (not needed anymore)
[X]  Package consistently uses macros (no %{buildroot} and $RPM_BUILD_ROOT mixing)
[X]  Package contains code, or permissable content.
[!]  Fully versioned dependency in subpackages, if present.
Javadoc subpackage should explicitly require the same version of parent package
[-]  Package contains a properly installed %{name}.desktop file if it is a GUI application.
[X]  Package does not own files or directories owned by other packages.
[X]  Javadoc documentation files are generated and included in -javadoc subpackage
[X]  Javadocs are placed in %{_javadocdir}/%{name} (no -%{version} symlinks)
[X]  Packages have proper BuildRequires/Requires on jpackage-utils
[X]  Javadoc subpackages have Require: jpackage-utils
[X]  Package uses %global not %define
[X]  If package uses tarball from VCS include comment how to re-create that tarball (svn export URL, git clone URL, ...)
[X]  If source tarball includes bundled jar/class files these need to be removed prior to building
[X]  All filenames in rpm packages must be valid UTF-8.
[X]  Jar files are installed to %{_javadir}/%{name}.jar (see [6] for details)
[X]  If package contains pom.xml files install it (including depmaps) even when building with ant
[X]  pom files has correct add_to_maven_depmap call which resolves to the pom file (use "JPP." and "JPP-" correctly)

=== Maven ===
[X]  Use %{_mavenpomdir} macro for placing pom files instead of %{_datadir}/maven2/poms
[X]  If package uses "-Dmaven.test.skip=true" explain why it was needed in a comment
[-]  If package uses custom depmap "-Dmaven2.jpp.depmap.file=*" explain why it's needed in a comment
[X]  Package uses %update_maven_depmap in %post/%postun
[X]  Packages have Requires(post) and Requires(postun) on jpackage-utils (for %update_maven_depmap macro)

=== Other suggestions ===
[ ]  If possible use upstream build method (maven/ant/javac)
[ ]  Avoid having BuildRequires on exact NVR unless necessary
[X]  Package has BuildArch: noarch (if possible)
[X]  Latest version is packaged.
[!]  Reviewer should test that the package builds in mock.
Not buildable in mock at the moment because of the missing BR. 


=== Issues ===
1. Missing BR of maven-shade-plugin
2. javadoc doesn't have a fully versioned dependency on the parent package

Comment 5 Stanislav Ochotnicky 2011-01-26 18:06:21 UTC
* Wed Jan 26 2011 Stanislav Ochotnicky <sochotnicky> - 1.4.1-2
- Add maven-shade-plugin to BR

Re: javadoc. Javadoc doesn't depend on main package at all, so that is why there is no Requires. "if present" in "Fully versioned dependency in subpackages, if present." means that IF there is dependency in subpackage on main package, that dependency must be exact (e.g. name = epoch:version-release}. I'll try to make the review template a bit less confusing.


Spec URL: http://sochotni.fedorapeople.org/packages/async-http-client.spec
SRPM URL: http://sochotni.fedorapeople.org/packages/async-http-client-1.4.1-2.fc14.src.rpm


Thanks for reviewing this BTW, it's great to have help :-)

Comment 6 David Nalley 2011-01-26 18:30:19 UTC
So I'll take your word on the javadoc requirement - I assume that you know the package a bit better than I do. 
Reading the below suggests that sub-packages should require the base package - and then of course, if it does require the base package then it should be fully versioned. I'll admit I don't grok why there wouldn't be a Requires there, but it's a 'should' not a must, so isn't a blocker. 
http://fedoraproject.org/wiki/Packaging/Guidelines#RequiringBasePackage


The added BR takes care of building. (Of course the sonatype-oss-parent doesn't appear to be in rawhide, so a mock build will fail if it hasn't hit yet)

Aside from that though, it should be ready to go  - APPROVED

Comment 7 Stanislav Ochotnicky 2011-01-27 08:02:01 UTC
Thanks for the review. I'll just explain the javadoc subpackage for future reference :-) Normally subpackages need to require main package. Javadoc subpackages are a special case (similar to -manual). They contain (more-less) only html files (API documentation) so there is no binary requirement on main package. Some javadoc subpackages do have requires on main package, we don't enforce it one way or the other (but my opinion is that people should be able to install documentation even without main package installed).

btw I see sonatype-oss-parent in rawhide (I actually built with it). I am guessing you were too fast and something didn't happen to be in buildroot yet. Anyway..I tested in mock so all should be OK.

New Package SCM Request
=======================
Package Name: async-http-client
Short Description:  Asynchronous Http Client for Java
Owners: sochotni
Branches: f14
InitialCC: java-sig

(f14 branch just to workaround fedpkg bug when only 1 branch is present)

Comment 8 Jason Tibbitts 2011-01-27 15:05:49 UTC
Git done (by process-git-requests).

There's a simpler workaround than making a branch you don't need, though.

Comment 9 Alexander Kurtakov 2011-01-31 11:52:09 UTC
Done in rawhide. http://koji.fedoraproject.org/koji/buildinfo?buildID=215944


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