Bug 962834 - Review Request: libmetalink - a Metalink C library
Review Request: libmetalink - a Metalink C library
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Michael Schwendt
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-14 10:29 EDT by Alejandro Alvarez
Modified: 2013-06-18 03:38 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-06-18 03:22:06 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
bugs.michael: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Alejandro Alvarez 2013-05-14 10:29:43 EDT
This is a deprecated package of which I would like to take ownership. The previous maintainer stopped supporting it, but upstream is active.

Spec URL: https://aalvarez.web.cern.ch/aalvarez/fedora/libmetalink.spec
SRPM URL: https://aalvarez.web.cern.ch/aalvarez/fedora/libmetalink-0.1.2-1.src.rpm

Description:
------------
libmetalink is a Metalink C library. It adds Metalink functionality such as
parsing Metalink XML files to programs written in C.

Fedora Account System Username: aalvarez

rpmlint output
--------------
libmetalink.src:84: W: macro-in-%changelog %{version}
libmetalink.src:84: W: macro-in-%changelog %{release}
libmetalink.src:87: W: macro-in-%changelog %{_docdir}
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

Build on EPEL6
--------------
http://koji.fedoraproject.org/koji/taskinfo?taskID=5379069

Build on F20
------------
http://koji.fedoraproject.org/koji/taskinfo?taskID=5379039
Comment 1 Michael Schwendt 2013-06-05 02:44:31 EDT
> rpmlint output

Fix the issues found by rpmlint, please. Also run rpmlint on the _built_ rpms, not just the src.rpm.


> Summary:        A Metalink C library

In Anaconda and package tools, which display these summaries, it looks better
(and more concise) when omitting these leading articles:

  Summary: Metalink library written in C

https://fedoraproject.org/wiki/Examples_of_good_package_summaries


> BuildRoot: ...

> %install
> rm -rf $RPM_BUILD_ROOT

> %clean
> rm -rf $RPM_BUILD_ROOT

> %defattr(-,root,root,-)

Nowdays these are not necessary anymore. Unless you want to build with the same spec file for EL5. Therefore it's better to remove what's not needed anymore and add a comment what shall be kept:

https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag
https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean
https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions


> %package       devel
> Summary:       A Metalink C library devel package

More clear (as in many -devel packages):

  Summary: Files needed for developing with %{name}

> Requires:      %{name} = %{version}-%{release}

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

> Requires:      pkgconfig

Not needed. Automatically added for the executable _and_ the .pc file's Provides.


> %package       docs

https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

Probably not worthwhile, but you could see whether to replace this -docs package with a -doc package (dead.package file says: retired on 2012-02-06).

> Group:         Development/Libraries

Rather "Group: Documentation".

> Requires:      %{name} = %{version}-%{release}

Independent documentation -doc packages typically don't require the base
package. It should be possible to install documentation without having to
install a program and all its dependencies.


> %files devel
> %{_includedir}/metalink/
> %{_includedir}/metalink/metalink*.h

Duplicate file entries. The first includes the directory *and* everything in it. The second includes the header files once more. Choose either one:

  %{_includedir}/metalink/

Or:

  %dir %{_includedir}/metalink/
  %{_includedir}/metalink/metalink*.h

As a packaging hint: It can be helpful to spell out the specific file names of header files, so if one file gets renamed during a version upgrade, the build would fail and serve as an early-warning-system.
Comment 2 Alejandro Alvarez 2013-06-10 05:51:43 EDT
Here is the new version.
By the way, I do want to release in EPEL5. I attach this time a build in el5-candidate.

Spec URL: https://aalvarez.web.cern.ch/aalvarez/fedora/libmetalink.spec
SRPM URL: https://aalvarez.web.cern.ch/aalvarez/fedora/libmetalink-0.1.2-2.fc18.src.rpm

rpmlint output
--------------
$rpmlint ./libmetalink-0.1.2-2.fc18.src.rpm 
libmetalink.src: W: spelling-error Summary(en_US) Metalink -> Meta link, Meta-link, Metal ink
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

$ rpmlint ./libmetalink-0.1.2-2.fc18.x86_64.rpm 
libmetalink.x86_64: W: spelling-error Summary(en_US) Metalink -> Meta link, Meta-link, Metal ink

Build on F20
------------
http://koji.fedoraproject.org/koji/taskinfo?taskID=5486463

Build on EPEL6
--------------
http://koji.fedoraproject.org/koji/taskinfo?taskID=5486469

Build on EPEL5
--------------
http://koji.fedoraproject.org/koji/taskinfo?taskID=5486487
Comment 3 Michael Schwendt 2013-06-13 04:52:23 EDT
* The changes are fine, except that I just had a final look at the new -doc package and noticed it contains only 15 manual pages with a total package size of 12092. As such I think it's not justified to create a separate -doc package. 

I recommend that you include the manual pages in the -devel package where they belong (manuals section 3). No Obsoletes are needed, since the -doc package hasn't existed before. The old packages included the man pages in the base package (wrongly): http://koji.fedoraproject.org/koji/packageinfo?packageID=8400

https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
| 
| MUST: Large documentation files must go in a -doc subpackage.
| (The definition of large is left up to the packager's best judgement,
| but is not restricted to size. Large can refer to either size or quantity).

The %changelog doesn't tell _why_ you've moved the man pages into a subpackage. In case a future version will include "large" documentation, it would still be possible to introduce a -doc package.


* EPEL5: %defattr is not needed there either, because RPM is new enough.


* You're free to handle the doc changes within pkg git.

APPROVED
Comment 4 Michael Schwendt 2013-06-13 04:55:10 EDT
Btw, "yum search cunit" is successful meanwhile, and the configure script searches for it. Consider looking into it, too.
Comment 5 Michael Schwendt 2013-06-13 04:57:37 EDT
BuildRequires: CUnit-devel

%check
make check
Comment 6 Michael Schwendt 2013-06-13 04:58:45 EDT
    CUnit - A unit testing framework for C - Version 2.1-2
     http://cunit.sourceforge.net/


Suite: libmetalink_TestSuite
  Test: test of metalink_list ...passed
  Test: test of metalink_pctrl_file_transaction ...passed
  Test: test of metalink_pctrl_resource_transaction ...passed
  Test: test of metalink_pctrl_checksum_transaction ...passed
  Test: test of metalink_pctrl_piece_hash_transaction ...passed
  Test: test of metalink_pctrl_chunk_checksum_transaction ...passed
  Test: test of metalink_parse_file ...passed
  Test: test of metalink_parse_fp ...passed
  Test: test of metalink_parse_fd ...passed
  Test: test of metalink_parse_memory ...passed
  Test: test of metalink_parse_update ...passed
  Test: test of metalink_parser_update_fail ...passed
  Test: test of metalink_check_safe_path ...passed
  Test: test of metalink_get_version ...passed
  Test: test of metalink_parse_file_v4 ...passed

Run Summary:    Type  Total    Ran Passed Failed Inactive
              suites      1      1    n/a      0        0
               tests     15     15     15      0        0
             asserts    489    489    489      0      n/a

Elapsed time =    0.000 seconds
PASS: metalinktest
=============
1 test passed
=============
Comment 7 Alejandro Alvarez 2013-06-13 05:09:37 EDT
I will have a look at those, thanks!

Package Change Request
======================
Package Name: libmetalink
New Branches: f17 f18 f19 el5 el6
Owners: aalvarez
Comment 8 Gwyn Ciesla 2013-06-13 08:09:18 EDT
Git done (by process-git-requests).
Comment 9 Alejandro Alvarez 2013-06-13 08:45:56 EDT
Sorry. I assumed master would be adopted too, but it hasn't been.

Package Change Request
======================
Package Name: libmetalink
New Branches: master
Owners: aalvarez
Comment 10 Michael Schwendt 2013-06-13 09:10:02 EDT
I think you need to file an unblock request at Fedora Release Engineering and point at this re-review: https://fedorahosted.org/rel-eng/
Comment 11 Alejandro Alvarez 2013-06-13 09:24:23 EDT
I have done that too, but I was surprised I can push to all branches except master.

Well, and in the database devel is the only one still marked as orphan and deprecated.
https://admin.fedoraproject.org/pkgdb/acls/name/libmetalink

For the record: https://fedorahosted.org/rel-eng/ticket/5635
Comment 12 Gwyn Ciesla 2013-06-13 09:53:06 EDT
You need to take ownership of it in pkgdb.
Comment 13 Michael Schwendt 2013-06-13 10:00:24 EDT
In pkgdb mortals cannot take ownership of "deprecated" packages. An admin needs to assist.

$ koji list-pkgs --package libmetalink --show-blocked
Package                 Tag                     Extra Arches     Owner          
----------------------- ----------------------- ---------------- ---------------
libmetalink             dist-f9                                  ant            
libmetalink             dist-5E-epel                             aalvarez       
libmetalink             f12-alpha                                ant            
libmetalink             f9-cutoff                                ant            
libmetalink             f9-build-cutoff                          ant            
libmetalink             f12-beta                                 ant            
libmetalink             f12-final                                ant            
libmetalink             dist-6E-epel                             aalvarez       
libmetalink             dist-f15                                 orphan         
libmetalink             f16                                      orphan         
libmetalink             f17                                      aalvarez        [BLOCKED]
libmetalink             f17-final                                orphan          [BLOCKED]
libmetalink             f18-Alpha                                orphan          [BLOCKED]
libmetalink             dist-f15-eol                             orphan         
libmetalink             f18-final                                orphan          [BLOCKED]
Comment 14 Michael Schwendt 2013-06-17 12:12:49 EDT
Alejandro, you can "Take ownership" in pkgdb now thanks to Kevin Fenzi.
Comment 15 Gwyn Ciesla 2013-06-17 12:16:03 EDT
Alejandro, you now own the devel branch.
Comment 16 Alejandro Alvarez 2013-06-18 03:21:24 EDT
Great, thanks!

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