Bug 1149176 - Review Request: vdsm-jsonrpc-java - jsonrpc communication lib for ovirt
Summary: Review Request: vdsm-jsonrpc-java - jsonrpc communication lib for ovirt
Keywords:
Status: NEW
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-JAVASIG
TreeView+ depends on / blocked
 
Reported: 2014-10-03 12:36 UTC by Piotr Kliczewski
Modified: 2018-08-18 11:26 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed:


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
oVirt gerrit 63673 master NEW fedora package review 2016-09-20 13:18:47 UTC

Description Piotr Kliczewski 2014-10-03 12:36:51 UTC
Spec URL: https://drive.google.com/file/d/0ByDkUDLXsT0YcHYxa1k2cGk0emc/view?usp=sharing
SRPM URL: https://drive.google.com/file/d/0ByDkUDLXsT0YeFJfMWJINWdPdnM/view?usp=sharing
Description: Communication library used by components of oVirt.
Fedora Account System Username: pkliczew

Comment 1 Michael Simacek 2015-12-07 12:36:15 UTC
Hi Piotr, are you still interested in packaging this?

Comment 2 Piotr Kliczewski 2015-12-07 12:52:24 UTC
Yes

Comment 3 Michael Simacek 2015-12-07 14:06:37 UTC
I don't see you in the packager group in FAS, that means you need a sponsor. I'm not a sponsor, therefore I cannot do the review.

Please read:
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers

Few other items:
- the specfile seems to be copied from RHEL, you'll need to clean it up a lot and make sure it conforms to current Fedora packaging guidelines
- please post the specfile/srpm somewhere where it can be downloaded directly. your current links cannot be processed by automated tools, such as fedora-review
- there seems to be more recent upstream version available

Comment 4 Mikolaj Izdebski 2015-12-08 23:19:58 UTC
(In reply to Michael Simacek from comment #3)
> I don't see you in the packager group in FAS, that means you need a sponsor.
> I'm not a sponsor, therefore I cannot do the review.

Michael, you can do the review and I will sponsor Piotr.

Comment 5 Douglas Schilling Landgraf 2015-12-09 07:10:07 UTC
Hi Piotr,

As mentioned in comment#3 it's recommended to provide the spec and sprm in it can be downloaded directly with tools like wget. The Source is pointing to Version 1.0.8 which is not available in http://resources.ovirt.org/pub/ovirt-3.5-snapshot/src/vdsm-jsonrpc-java/. At this point, please update the spec to the lastest ovirt-3.6 bits.

Every change in spec, please update Release and reflect in changelog

Encourage upstream to add a copy of license (COPYING) and use %license macro in the spec.
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines

Probably maven-surefire-provider-junit4 should be replaced by 
maven-surefire-provider-junit

Looks like you will need BuildRequires: gcc
$ rpmbuild -ba vdsm-jsonrpc-java.spec
configure: error: no acceptable C compiler found in $PATH
See `config.log' for more details.
error: Bad exit status from /var/tmp/rpm-tmp.DBtEbl (%build)

A good recommendation is refresh your spec and re-test it using fedora-review tool, a simple example:
$ fedora-review --rpm-spec -n vdsm-jsonrpc-java-1.0.8-1.XX.src.rpm

Comment 6 Douglas Schilling Landgraf 2015-12-14 21:51:22 UTC
Hi Piotr,

I see you updated the package. Let's start with the below, please also consider the previous comments.

- Point the spec and srpm to a different url, so it can easily downloaded by
  wget or similar tool. 

- Source0 is not correct, the last bits seems to be 1.1.6
  http://resources.ovirt.org/pub/ovirt-3.6-snapshot/src/vdsm-jsonrpc-java/vdsm-jsonrpc-java-1.1.5.tar.gz

- Remove non needed BuildRoot tag
  BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
  https://fedoraproject.org/wiki/Java_review_template 
  (Buildroot definition is not present)

- Package does not contain duplicates in %files.
  Note: warning: File listed twice: /usr/share/java/vdsm-jsonrpc-java
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles

- For subpackage that requires the main package, please use: 
  https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

- This one below, is it really needed?
  %if 0%{?_use_repolib:1}
  rm -rf "%{buildroot}"
  %endif

- some rpmlint issues:
  vdsm-jsonrpc-java.src: W: summary-not-capitalized C oVirt JsonRpc java client (vdsm-jsonrpc-java)
  vdsm-jsonrpc-java.src: W: invalid-url Source0: http://resources.ovirt.org/pub/ovirt-3.6-snapshot/src/vdsm-jsonrpc-java/vdsm-jsonrpc-java-1.1.5.tar.gz HTTP Error 404: Not Found

- Please consider run fedora-review tool with fedora-review-plugin-java plugin, it will help during your review.

Comment 7 Douglas Schilling Landgraf 2015-12-14 22:02:12 UTC
Also, please also double check if vdsm-jsonrpc-java spec should keep the EL6 support, looks like the current vdsm only supports EL7 in current days.

Comment 8 Piotr Kliczewski 2015-12-15 10:35:35 UTC
El6 is supported as well and we build it every time there is change in the code within this package.

Comment 9 Piotr Kliczewski 2015-12-16 16:02:44 UTC
(In reply to Douglas Schilling Landgraf from comment #6)
> Hi Piotr,
> 
> I see you updated the package. Let's start with the below, please also
> consider the previous comments.
> 
> - Point the spec and srpm to a different url, so it can easily downloaded by
>   wget or similar tool. 
> 
> - Source0 is not correct, the last bits seems to be 1.1.6
>  
> http://resources.ovirt.org/pub/ovirt-3.6-snapshot/src/vdsm-jsonrpc-java/vdsm-
> jsonrpc-java-1.1.5.tar.gz
> 

Updated

> - Remove non needed BuildRoot tag
>   BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u}
> -n)
>   https://fedoraproject.org/wiki/Java_review_template 
>   (Buildroot definition is not present)
> 

We have BuildRoot in line #52

> - Package does not contain duplicates in %files.
>   Note: warning: File listed twice: /usr/share/java/vdsm-jsonrpc-java
>   See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles
> 

Can you point me to the lines? I do not see it.

> - For subpackage that requires the main package, please use: 
>   https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package
> 

There are 3 packages:
- with code
- with java-doc
- with src

so I think there is no need to declare dependencies

> - This one below, is it really needed?
>   %if 0%{?_use_repolib:1}
>   rm -rf "%{buildroot}"
>   %endif
> 
> - some rpmlint issues:
>   vdsm-jsonrpc-java.src: W: summary-not-capitalized C oVirt JsonRpc java
> client (vdsm-jsonrpc-java)
>   vdsm-jsonrpc-java.src: W: invalid-url Source0:
> http://resources.ovirt.org/pub/ovirt-3.6-snapshot/src/vdsm-jsonrpc-java/vdsm-
> jsonrpc-java-1.1.5.tar.gz HTTP Error 404: Not Found
> 

Fixed

> - Please consider run fedora-review tool with fedora-review-plugin-java
> plugin, it will help during your review.

New version uploaded.

Comment 10 Douglas Schilling Landgraf 2016-09-05 16:07:07 UTC
Piotr, I am reseting the assignee at this moment, if you are still interested in this package please update the spec so other person might continue from here.

Comment 11 Piotr Kliczewski 2016-09-06 07:22:32 UTC
The spec was updated long time ago and still waiting on review.

Comment 12 Mikolaj Izdebski 2016-09-06 09:22:13 UTC
Piotr, I still can't see the updated version. Are you sure that SRPM and spec URLs are up-to-date?

Comment 13 Piotr Kliczewski 2016-09-06 09:46:42 UTC
I updated the spec as you can see on 2015-12-16. The package is under constant development so I would need to check whether there were any changes.

Will check and let you know.

Comment 14 Michael Simacek 2016-09-06 13:53:36 UTC
I only see the comment saying that you updated it, but not the updated specfile.

I'd like to note that while Fedora and RHEL based products use RPM specfiles, their usage and guidelines can differ a lot. I've examined the old specfile linked in the original description and there are many problems. I'd strongly suggest creating a new specfile from scratch using current guidelines and documentation, as that will actually be less work for both you and the reviewer.

Comment 15 Piotr Kliczewski 2016-09-07 08:24:21 UTC
Will do that.

Comment 16 Michael Simacek 2016-09-07 09:47:29 UTC
Great.

Most of the links to relevant documentation are available in https://fedoraproject.org/wiki/Join_the_package_collection_maintainers

Java packaging HowTo is available at https://fedora-java.github.io/howto/latest/index.html

Feel free to contact us with questions on IRC on #fedora-java freenode channel.

Comment 17 Piotr Kliczewski 2016-09-07 15:35:26 UTC
I removed old spec and srpm and uploaded updated schema and newly created srpm:

Spec URL: https://drive.google.com/open?id=0ByDkUDLXsT0YYTZqTGc2YTJnSDQ
To download: wget --no-check-certificate 'https://drive.google.com/open?id=0ByDkUDLXsT0YYTZqTGc2YTJnSDQ' -O vdsm-jsonrpc-java.spec

SRPM URL: https://drive.google.com/open?id=0ByDkUDLXsT0YM05nenNid0REMGM
To download: wget --no-check-certificate 'https://drive.google.com/open?id=0ByDkUDLXsT0YM05nenNid0REMGM' -O vdsm-jsonrpc-java-1.3.1-master.fc23.src.rpm

Comment 18 Michael Simacek 2016-09-08 09:35:07 UTC
Looks much better now.

Issues I found so far:
- There are missing BuildRequires, so the package doesn't build in mock. Most notably "maven-local" package which provides mvn_build. For other maven dependencies, there are some tools which can help you, see: https://fedora-java.github.io/howto/snapshot/index.html#error_missing_dependency
- The Release numbering is not correct - you won't be able to update to new snapshots with this scheme. See https://fedoraproject.org/wiki/Packaging:Versioning - section about snapshot packages.
- _use_maven variable is always true, do you need it at all? You could get rid of it and the many ifs that use it.
- EPEL should contain stable packages that are updated only when there is a compelling reason, similarly to RHEL. I'm not much familiar with EPEL guidelines, but I'm afraid you shouldn't/cannot have snapshot package there. Did you consider packaging stable versions for Fedora and EPEL and using Copr for snapshots?
- The %description should be improved
- When using mvn_build, you shouldn't specify Requires - they are autogenerated.
- Changleog entries should contain release at the end. You can use tools like rpmdev-bumpspec to generate those.

Comment 19 Piotr Kliczewski 2016-09-08 11:26:58 UTC
Michael thank you for your feedback. Will follow your suggestions.


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