Bug 1428693 - Review Request: netpgp - Freely licensed PGP implementation
Summary: Review Request: netpgp - Freely licensed PGP implementation
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard: NotReady
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-03-03 07:36 UTC by jeffrey.lau
Modified: 2017-04-26 03:20 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-04-26 03:20:56 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)

Description jeffrey.lau 2017-03-03 07:36:35 UTC
Spec URL: https://github.com/riboseinc/netpgp/raw/master/packaging/redhat/m4/rpm.spec
SRPM URL: https://github.com/riboseinc/yum/raw/master/SRPMS/netpgp-3.99.17-1.src.rpm
Description: Freely licensed PGP implementation
Fedora Account System Username: jeffreylauribose

This is my first package review request, so I would greatly appreciate any sponsorship.  If there's anything needing correction, I'm all ears.

Github link:
[1] https://github.com/riboseinc/netpgp

Follow the README on the Github link for build instructions.

One thing of note is that the rpm.spec needs preprocessing with m4, which is all automated in the build script detailed in [1].

Cheers!

Comment 1 Michael Schwendt 2017-03-04 11:39:29 UTC
Tons of strange things in the spec file. This will need _a lot_ of work to bring the package into shape. I wonder whether and when you've had a look at other Fedora [or Red Hat] packages for the last time?

As a quickstart, point the fedora-review tool at this ticket:

    fedora-review -b 1428693

It will evaluate the "Spec URL:" and "SRPM URL:" lines, download the latest files, do local builds in Mock and then perform many checks on the packages. That tool should be of interest to you, since it is not specific to reviewers, and all packagers should use it.

Comment 2 jeffrey.lau 2017-03-08 08:04:02 UTC
Thanks Michael for pointing me to the fedora-review tool.

Over the last few days I've just made a few changes --- specifically, the Spec URL and SRPM URL are now different.  Should I be making a new review request with the updated details and then close this bug, or post the details on this same thread?

Comment 3 Michael Schwendt 2017-03-08 09:30:46 UTC
Just post a comment with a pair of "Spec URL:" and "SRPM URL:" pointing at your update package as per the process description:

https://fedoraproject.org/wiki/Package_Review_Process#Contributor

Comment 5 jeffrey.lau 2017-03-08 09:42:27 UTC
Thanks Michael, I'll remove the NotReady from the Whiteboard once it's ready for further review.

Comment 6 Parag AN(पराग) 2017-03-09 09:18:16 UTC
Very Very Preliminary review:

1) Package failed to build in mock. It failed as spec is using autoreconf but that binary is not available in build environment. Fix this as
BuildRequires: autoconf

2) SPEC file must not contain version number. Change netpgp-3.99.17.spec to netpgp.spec

3) Packager and Vendor tags are not used in Fedora packaging.

4) Source0: should provide direct downloadable URL
I suggest check with upstream if they can release any tarball otherwise we need to document the steps on how you created source archive as comments just before Source0: line.

5) Any BuildRequires or Requires for any package should not restrict versions. In this package I see 
Requires: netpgpverify = %{version}
which should be
Requires: netpgpverify

6) We don't need these %defines. Remove them.
%define _unpackaged_files_terminate_build 0
%define _prefix /usr

We already define _prefix to /usr. See /usr/lib/rpm/macros file.

7) For %prep section, Fedora packaging guidelines now recommends use of 
%autosetup

8) for %build
we have those macros already defined in /usr/lib/rpm/macros no need to specify them. I think its better if upstream will provide some release where configure will already be available. Then you will not need to use autoreconf. 

Any C library providing package generally have these lines in spec
%configure --disable-static
make %{?_smp_mflags}


9) for %install any general library providing package should have lines
make install DESTDIR=$RPM_BUILD_ROOT
find $RPM_BUILD_ROOT -name '*.la' -exec rm -f {} ';'


9) We no longer need every %files section to write
%defattr(-,root,root)
and then for every listed files to start with %attr(0755,root,root)

10) for library package we only need
%post -p /sbin/ldconfig

%postun -p /sbin/ldconfig

and not other %pre, %preun. See more on scriptlet here https://fedoraproject.org/wiki/Packaging:Scriptlets#Shared_libraries

11) We no loner need to write %clean

Hope this will at least help you to start moving this package updates. Please remember whenever someone suggests any changes you need to increase release tag by 1 and add appropriate changelog entry e.g. "Review suggestions fixes".
This is the way package submitter need to provide updates. 

Also every time you need to specify both the links that is Spec URL and SRPM URL.
If not provided reviewer cannot use fedora-review tool to review your package and thus you will not get any inputs here.

Comment 7 jeffrey.lau 2017-03-09 09:23:56 UTC
This is very helpful for a newbie like me, thanks Parag!  Will be working through this list.

Comment 8 Parag AN(पराग) 2017-03-09 10:38:34 UTC
(In reply to Parag AN(पराग) from comment #6)
> Very Very Preliminary review:
> 5) Any BuildRequires or Requires for any package should not restrict
> versions. In this package I see 
> Requires: netpgpverify = %{version}
> which should be
> Requires: netpgpverify
> 

ah please forget this point. Its for specific version requirements not for the same version. I was going in SPEC one by one line and later at the end found this package also provides netpgpverify. Hence, you can keep it as "Requires: netpgpverify = %{version}-%{release}"


Also, 
To get sponsor for your package in packager group, please follow these things.

Make sure you have followed steps given on https://fedoraproject.org/wiki/Join_the_package_collection_maintainers
till "Get Sponsored" section.

We have this process, 
http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group to 
get sponsored into the packager group. Can you either submit few more packages 
and/or some full detailed package reviews? This is needed to make sure package 
submitter understands the rpm packaging well and follows the fedora packaging 
guidelines.

Please go through the following links
1) http://fedoraproject.org/wiki/Package_Review_Process

2) https://fedoraproject.org/wiki/PackagingGuidelines

3) To find the packages already submitted for review,
   check http://fedoraproject.org/PackageReviewStatus/

4) http://fedoraproject.org/wiki/Packaging:ReviewGuidelines and
   http://fedoraproject.org/wiki/Package_Review_Process#Reviewer is useful 
   while doing package reviews.

5) https://fedorahosted.org/FedoraReview/ this is fedora-review tool to help
   review packages in fedora. You need to use this and do un-official package 
   reviews of packages submitted by other contributors. While doing so mention 
   "This is un-official review of the package." at top of your review comment.

Good to review packages listed in http://fedoraproject.org/PackageReviewStatus/NEW.html

When you do full package review of some packages, provide that review comment 
link here so that I can look how you have reviewed those packages. An example
command to run fedora-review on any package review bugzilla is

fedora-review -b <bugid> -m fedora-rawhide-x86_64

If you got any questions please ask here or on Freenode IRC join #fedora-devel :)

Comment 9 Parag AN(पराग) 2017-04-17 07:25:26 UTC
Any updates here?

Comment 10 jeffrey.lau 2017-04-26 02:25:55 UTC
Thanks Parag for the thorough guidance! :)

We're actually going to rename this package (+ a whole host of other changes including new dependencies introduced).  But the dependencies haven't been resolved yet, so we don't have any new Spec / SRPM URLs to post.

Regarding the rename of the package, I've read the Rename Process [1], but I'm still not sure if it's sufficient just to edit this request's Summary, or if a new review request is required, as this package doesn't replace any old packages.

[1] https://fedoraproject.org/wiki/Package_Renaming_Process#Re-review_required

Comment 11 Parag AN(पराग) 2017-04-26 02:57:43 UTC
I think better close this as NOTABUG and open a new review request bug. If you can't close this just ask here and I will close this bug.

Comment 12 jeffrey.lau 2017-04-26 03:20:56 UTC
OK.  Closing as NOTABUG as a package rename is expected.


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