>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

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.

Hi, SPEC file was updated.

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

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

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!

Note that the packaging guidelines prefer %global over %define: https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define

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!

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.

Created attachment 1451464 [details]
review checklist for beaker-libraries

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.

2. More about import mechanism https://www.mankier.com/1/beakerlib-libraries
It shows that files should not be executable.

1. Commit https://pagure.io/beakerlib-libraries/c/6698e44b0ee7a5ed2101fef09692bf3fd680e827?branch=master fixes issue #1.

Right, #5 is not intentional. I will tag with release + upload .tar.gz file as this BZ is approved.

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.

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.

> 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.

That takes care of all my concerns, package accepted

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/beakerlib-libraries