Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1392950
Summary: | Review Request: qclib - Provides a C API for extraction of system information for Linux on z Systems | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Rafael Fonseca <rdossant> |
Component: | Package Review | Assignee: | Dan Horák <dan> |
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | bugproxy, dan, hannsj_uhl, package-review, thomas.andrejak |
Target Milestone: | --- | Flags: | dan:
fedora-review+
|
Target Release: | --- | ||
Hardware: | s390x | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | If docs needed, set a value | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2017-01-10 14:06:28 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 467765, 1654309 |
Description
Rafael Fonseca
2016-11-08 14:13:32 UTC
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 (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 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#Requiring_Base_Package) - /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?) 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. 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. 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 Just update the spec file to show full gcc command line and fixed owning of %{_docdir}/%{name} All issues were fixed, the package is APPROVED. Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/qclib ------- Comment From mgrf.com 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 ? (In reply to IBM Bug Proxy from comment #10) > ------- Comment From mgrf.com 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 |