Bug 1575601 - Review Request: beakerlib-libraries - Beakerlib Libraries
Summary: Review Request: beakerlib-libraries - Beakerlib Libraries
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Tim Flink
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-05-07 12:15 UTC by Andrei Stepanov
Modified: 2020-04-24 14:26 UTC (History)
10 users (show)

Fixed In Version: beakerlib-libraries-0.2-1.fc27
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-04-24 14:26:46 UTC
Type: ---
tflink: fedora-review+


Attachments (Terms of Use)
review checklist for beaker-libraries (11.92 KB, text/plain)
2018-06-14 16:45 UTC, Tim Flink
no flags Details

Description Andrei Stepanov 2018-05-07 12:15:18 UTC
Spec URL: https://pagure.io/beakerlib-libraries/blob/master/f/beakerlib-libraries.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/mvadkert/beakerlib-libraries/epel-7-x86_64/00742549-beakerlib-libraries/beakerlib-libraries-0.1-39.g764645d.src.rpm
Description: Beakerlib Libraries are used by beakerlib tests to encapsulate common complex tasks such as configuring and starting a particular daemon in a single function.
Fedora Account System Username: astepano

Comment 1 Artur Frenszek-Iwicki 2018-05-07 13:18:42 UTC
>Group: Development/Libraries
Drop this.

>BuildRoot: %{_tmppath}/beaker-libraries-buildroot
Drop this too.

>%clean
>rm -rf $RPM_BUILD_ROOT
And this.
https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections

>Source0: beakerlib-libraries.tar.gz
Ideally, this should be a downloadable URL. If that's not possible, include a #comment describing how to recreate the tarball.
https://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control

>%define libraries_path /usr/share/beakerlib-libraries
Use %{_datadir} instead of /usr/share.
https://fedoraproject.org/wiki/Packaging:RPMMacros

Comment 2 Andrei Stepanov 2018-05-07 15:13:01 UTC
Hi, thanks for the comments!
I sent PR: https://pagure.io/beakerlib-libraries/pull-request/3#request_diff
Spec file will be updated with this PR.

Comment 3 Andrei Stepanov 2018-05-07 15:34:40 UTC
Hi, SPEC file was updated.

https://pagure.io/beakerlib-libraries/blob/master/f/beakerlib-libraries.spec

Comment 4 Artur Frenszek-Iwicki 2018-05-11 16:21:21 UTC
I'm not sure why you have three %defines at the top for the name, version and release. If you put them in the tags (like "Version: 0.1"), you can use the macros (e.g. "%{version}") later on without any issues.

>Release: 39.g764645d
This should end with the dist tag (e.g. "f27") - use "%{?dist}".

>%setup -n beakerlib-libraries
It's recommended to use a "quiet setup" - use the -q option.

>%install
>rm -rf %{buildroot}
Do not remove the buildroot at start of %install. [1]

>%clean
>rm -rf $RPM_BUILD_ROOT
Drop the %clean section completely. [1]

>rm -rf %{buildroot}
>rm -rf $RPM_BUILD_ROOT
Do not mix usages of %{buildroot} and $RPM_BUILD_ROOT. Pick one and stick to it. [2]

>cp -rf * %{buildroot}%{libraries_path}
Use the -p option to preserve timestamps. [3]

There are some scripts (.sh) installed which do not have the executable bit set: [4]
/usr/share/beakerlib-libraries/authconfig/Library/basic/lib.sh
/usr/share/beakerlib-libraries/bind/Library/bind-setup/lib.sh
/usr/share/beakerlib-libraries/distribution/Library/Cleanup/lib.sh
/usr/share/beakerlib-libraries/distribution/Library/Log/lib.sh
/usr/share/beakerlib-libraries/distribution/Library/RpmSnapshot/lib.sh
/usr/share/beakerlib-libraries/distribution/Library/dpcommon/lib.sh
/usr/share/beakerlib-libraries/distribution/Library/dump/lib.sh
/usr/share/beakerlib-libraries/distribution/Library/epel/lib.sh
/usr/share/beakerlib-libraries/distribution/Library/opts/lib.sh
/usr/share/beakerlib-libraries/distribution/Library/sundry/lib.sh
/usr/share/beakerlib-libraries/kernel/Library/base/lib-compat.sh
/usr/share/beakerlib-libraries/kernel/Library/base/lib.sh
/usr/share/beakerlib-libraries/nginx/Library/nginx/lib.sh
/usr/share/beakerlib-libraries/openssl/Library/certgen/lib.sh
/usr/share/beakerlib-libraries/openssl/Library/certgen/runtest.sh
/usr/share/beakerlib-libraries/perl/Library/subpackage/lib.sh
/usr/share/beakerlib-libraries/php/Library/utils/lib.sh
/usr/share/beakerlib-libraries/samba/Library/samba-bits/lib.sh
/usr/share/beakerlib-libraries/sos/Library/utils/lib.sh
/usr/share/beakerlib-libraries/squid/Library/squid/lib.sh
/usr/share/beakerlib-libraries/tuned/Library/basic/lib.sh
/usr/share/beakerlib-libraries/yum/Library/common-functions/lib.sh

There's also a single .c file with the executable bit set: [4]
/usr/share/beakerlib-libraries/kernel/Library/base/src/expendfdrun/expendfdrun.c

You should also add %changelog entries when making changes.

[1] https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections
[2] https://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS
[3] https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps
[4] https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

Comment 5 Andrei Stepanov 2018-05-14 10:27:51 UTC
Artur Iwicki Hi!
Thank you for your suggestions. 
I made requested changes:

Spec file is : https://pagure.io/beakerlib-libraries/blob/master/f/beakerlib-libraries.spec

Changes are:

https://pagure.io/beakerlib-libraries/c/9e83077b04221e2e21438548415dba7fd319f7f1?branch=master

I kept /usr/share/beakerlib-libraries/*/*.sh files without executable bit. There is reason for this: These files supposed only be included in other scripts. By no means they should be called directly.

Could you please check again?

Thank you!

Comment 6 Pierre-YvesChibon 2018-05-15 12:09:46 UTC
Note that the packaging guidelines prefer %global over %define: https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define

Comment 7 Andrei Stepanov 2018-05-15 13:25:11 UTC
Pierre-YvesChibon, Artur Iwicki

I updated .spec file as you requested:

https://pagure.io/beakerlib-libraries/blob/master/f/beakerlib-libraries.spec

SRPM and builds for distros are with with updates spec:

https://copr.fedorainfracloud.org/coprs/g/osci/beakerlib-libraries/build/754413/

Could you please review? Thank you!

Comment 8 Tim Flink 2018-06-14 16:44:54 UTC
Overall, it looks pretty good but I found a few small concerns.

1. The license declaration in the spec file does not match the code packaged. Some of the packaged code is GPLv2, some of it is GPLv2+. If the declaration is changed to "GPLv2 and GPLv2+", that would be an acceptable solution. There is also no LICENSE file included upstream which would be preferred instead of just having license headers in most of the individual files.

2. I'm a little unclear on how these libraries are called, should the shell files be executable?

3. Requires issues? I'm going back and forth on this one - the individiual libraries are not usable without unlisted dependencies (mongodb, httpd, openssl etc.) but including all of those as requires seems a bit heavyweight and given the use case of these libraries, I'm not sure how critical it is to have all of those dependencies made explicit. One option would be to make all of the individual libraries into subpackages (which would also solve the licensing issue) but 

4. Mariadb55 has (unstated) requirements which cannot be resolved in any current Fedora release.

5. The source URL is not valid, I get a 404 when I try to download beaker-libraries-0.2

I'll attach the review checklist to this bug but 1-4 will need to be resolved through fixing/discussion and 5 is a SHOULD have that I suspect is not intentional.

Comment 9 Tim Flink 2018-06-14 16:45:53 UTC
Created attachment 1451464 [details]
review checklist for beaker-libraries

Comment 10 Andrei Stepanov 2018-06-15 14:19:59 UTC
2. I'm a little unclear on how these libraries are called, should the shell files be executable?

This libraries are included within `runtest.sh` file.
https://github.com/beakerlib/beakerlib/wiki/man#rlimport

  rlRun "rlImport usability/httpd-custom-dir" || rlDie "Can't HTTP export logs"

They should not be executable.

Comment 11 Andrei Stepanov 2018-06-15 14:22:05 UTC
2. More about import mechanism https://www.mankier.com/1/beakerlib-libraries
It shows that files should not be executable.

Comment 13 Andrei Stepanov 2018-06-15 14:30:36 UTC
Right, #5 is not intentional. I will tag with release + upload .tar.gz file as this BZ is approved.

Comment 14 Andrei Stepanov 2018-06-15 15:09:04 UTC
3. I think there should not be issues with requirements. This is test-framework.
For example library for squid there is:

./squid/squid/Makefile:56:	@echo "Requires:        squid httpd" >> $(METADATA)

beakerlib takes responsibility for installing necessary package.

Comment 15 Andrei Stepanov 2018-06-15 15:11:18 UTC
About subsection 4. Mariadb55 has (unstated) requirements which cannot be resolved in any current Fedora release.

We run tests with mvadkert@. It turned out that Mariadb55 is library name. It has nothing with package dependency. The test which uses the library works, it uses mariadb.

Comment 16 Miroslav Vadkerti 2018-06-15 15:17:12 UTC
> We run tests with mvadkert@. It turned out that Mariadb55 is library name. It has nothing with package dependency. The test which uses the library works, it uses mariadb.

Correction: the variable PACKAGE in mariadb55/basic/runtest.sh is not used anywhere, please ignore that. I even tried to remove it and no change. All tests pass.

Comment 17 Tim Flink 2018-06-15 15:23:26 UTC
That takes care of all my concerns, package accepted

Comment 18 Gwyn Ciesla 2018-06-15 15:33:14 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/beakerlib-libraries


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