[Bug 1392950] New: Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

Bug ID: 1392950
           Summary: Review Request: qclib - Provides a C API for
                    extraction of system information for Linux on z
                    Systems
           Product: Fedora
           Version: rawhide
         Component: Package Review
          Severity: medium
          Priority: medium
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]

Spec URL: http://people.redhat.com/~rdossant/qclib.spec
SRPM URL: http://people.redhat.com/~rdossant/qclib-1.2.0-1.fc24.src.rpm
Description: qclib provides a C API for extraction of system information for
Linux on z
Systems.
For instance, it will provide the number of CPUs 
 * on the machine (CEC, Central Electronic Complex) layer
 * on the PR/SM (Processor Resource/Systems Manager) layer, i.e. visible to
   LPARs, including LPAR groups in z/VM hosts, guests and CPU pools
 * in KVM hosts and guests

This allows calculating the upper limit of CPU resources a highest level guest
can use. For example: If an LPAR on a z13 provides 4 CPUs to a z/VM
hyper-visor,
and the hyper-visor provides 8 virtual CPUs to a guest, qclib can be used to
retrieve all of these numbers, and it can be concluded that not more capacity
than 4 CPUs can be used by the software running in the guest.

Fedora Account System Username: rdossant

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

Thomas Andrejak  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #1 from Thomas Andrejak  ---
Hello

I'm not a packager yet, hence the review is unofficial. Also, I do not have
s390 or s390x so I can't build it but I have some comments.

- Summary:
I propose to remove "Provides a "

- Source0:
At the end, you should use %{name}-%{version}.tgz

- Patch0:
Also, use %{version}

- In %package, do not specify the group

- In %package, I think you have to add the %{?_isa} to requires

- %package static
Provides:    foo-static = %{version}-%{release} <== Typo for "foo" ?

- %prep
I think you can use %autosetup

- %files doc
Is there a reason you are using a different license file than "%files"

Regards

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

Dan Horák  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]
             Blocks|                            |1306280
           Assignee|[e-mail]    |[e-mail]
              Flags|                            |fedora-review?

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

Dan Horák  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

--- Comment #2 from Dan Horák  ---
(In reply to Thomas Andrejak from comment #1)
> Hello
>
> I'm not a packager yet, hence the review is unofficial. Also, I do not have
> s390 or s390x so I can't build it but I have some comments.

you should be able to run "s390-koji build --scratch f26 your.src.rpm", there
are also "arm-koji" and "ppc-koji" for F < 26

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

Hanns-Joachim Uhl  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Hardware|All                         |s390x
             Blocks|                            |467765 (ZedoraTracker)

Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=467765
[Bug 467765] Fedora for System z (s390): Bug Tracker

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

--- Comment #3 from Dan Horák  ---
formal review is here, see the notes explaining OK* and BAD statuses below:

OK      source files match upstream:
            e0a0c4c73a63c6a8c5281bab9508dc634f39925a  qclib-1.2.0.tgz
OK      package meets naming and versioning guidelines.
OK*     specfile is properly named, is cleanly written and uses macros
consistently.
OK      dist tag is present.
OK      license field matches the actual license.
OK      license is open source-compatible (BSD). License text included in
package.
OK      latest version is being packaged.
OK      BuildRequires are proper.
BAD     compiler flags are appropriate.
OK      package builds in mock (Rawhide/s390x).
OK      debuginfo package looks complete.
OK*     rpmlint is silent.
BAD     final provides and requires look sane.
OK      %check is present and all tests pass.
OK      shared libraries are added to the regular linker search paths.
BAD     owns the directories it creates.
OK      doesn't own any directories it shouldn't.
OK      no duplicates in %files.
OK      file permissions are appropriate.
BAD     correct scriptlets present.
OK      code, not content.
OK*     documentation is small, so no -docs subpackage is necessary.
OK      %docs are not necessary for the proper functioning of the package.
OK      headers in devel
OK      no pkgconfig files.
OK      no libtool .la droppings.
OK      not a GUI app.

- you can drop the Group tag, it isn't used
- Source0 should be in %{name}-%{version}.tgz form
- Summary could be simplified a bit to "Library for extraction of system
information for Linux on z Systems"
- the build is "silent" (no visible compile commands), but seems that upstream
not distro CFLAGS are used
- rpmlint complains a bit
qclib-static.s390x: W: no-documentation
qclib.s390x: W: no-documentation
qclib.src:23: W: macro-in-comment %{name}
qclib.src:23: W: macro-in-comment %{version}
qclib.src:23: W: macro-in-comment %{release}
qclib.src:14: W: mixed-use-of-spaces-and-tabs (spaces: line 14, tab: line 1)
qclib-devel.s390x: W: only-non-binary-in-usr-lib
qclib-devel.s390x: W: no-documentation
6 packages and 0 specfiles checked; 0 errors, 8 warnings.
-> drop the commented out line 23 and fix the mixed tabs and spaces
- foo-static is Provided in the static subpackage, should be %{name}-static
- the devel package should Require the base package with %{_isa}
(https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#...)
- /usr/share/doc/packages/qclib/ is created, but not owned by the doc
subpackage
- you can drop the scriptlets for the devel subpackage, ldconfig makes sense
only for the runtime lib
- my personal opinion is that the separate doc subpackage is not needed if the
rpm sizes are in low 100kB and can be merged with the devel subpackage
- I would add the README as documentation into the base package
- the path in the doc package shouldn't contain the "packages" part (debianism
in upstream Makefile?)

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

Hanns-Joachim Uhl  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]
    External Bug ID|                            |IBM Linux Technology Center
                   |                            |149296

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

--- Comment #4 from Rafael Fonseca  ---
Update addressing mentioned points:

Spec URL: http://people.redhat.com/~rdossant/qclib.spec
SRPM URL: http://people.redhat.com/~rdossant/qclib-1.2.0-1.fc25.src.rpm

Thank you for the review.

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

--- Comment #5 from Dan Horák  ---
All looks good now except the %doc files, specifically the docs dir, which is
not owned in the final rpm. I think we need the docs dir to be configurable in
the makefile.

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

--- Comment #6 from Rafael Fonseca  ---
Making doc dir configurable:

Spec URL: http://people.redhat.com/~rdossant/qclib.spec
SRPM URL: http://people.redhat.com/~rdossant/qclib-1.2.0-1.fc25.src.rpm

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

--- Comment #7 from Rafael Fonseca  ---
Just update the spec file to show full gcc command line and fixed owning of
%{_docdir}/%{name}

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

Dan Horák  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|fedora-review?              |fedora-review+

--- Comment #8 from Dan Horák  ---
All issues were fixed, the package is APPROVED.

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

--- Comment #9 from Jon Ciesla  ---
Package request has been approved:
https://admin.fedoraproject.org/pkgdb/package/rpms/qclib

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

Rafael Fonseca  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|---                         |RAWHIDE
        Last Closed|                            |2017-01-10 09:06:28

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

--- Comment #10 from IBM Bug Proxy  ---
------- Comment From [e-mail] 2017-09-13 09:55 EDT-------
Dan,
I guess this can be closed "done" as it is part of Fedora 25 already - isn't it
?

[Bug 1392950] Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems

https://bugzilla.redhat.com/show_bug.cgi?id=1392950

--- Comment #11 from Dan Horák  ---
(In reply to IBM Bug Proxy from comment #10)
> ------- Comment From [e-mail] 2017-09-13 09:55 EDT-------
> Dan,
> I guess this can be closed "done" as it is part of Fedora 25 already - isn't
> it ?

correct, it's included in F-27 and Rawhide, so all is good