Bug 561470
Summary: | Review Request: beakerlib - shell-level integration testing library | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Petr Muller <pmuller> | ||||
Component: | Package Review | Assignee: | Kevin Fenzi <kevin> | ||||
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | rawhide | CC: | fedora-package-review, jlaska, kevin, notting, ohudlick, psplicha, tcallawa, terje.rosten | ||||
Target Milestone: | --- | Flags: | kevin:
fedora-review+
gwync: fedora-cvs+ |
||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | beakerlib-1.3-1.fc13 | Doc Type: | Bug Fix | ||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2010-06-01 18:16:12 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: | |||||||
Attachments: |
|
Description
Petr Muller
2010-02-03 18:00:31 UTC
More info: https://fedorahosted.org/beakerlib/ (we are also "upstream") rpmlint: $ rpmlint /home/afri/qa/beakerlib/rpm-build/noarch/beakerlib-1.0-1.noarch.rpm beakerlib.noarch: W: self-obsoletion beaker-lib obsoletes beaker-lib beakerlib.noarch: W: self-obsoletion rhtslib obsoletes rhtslib These are the "old" names for the same stuff. Both names are in both "Provides" and "Obsoletes" field, which I believed was the right way. beakerlib.noarch: W: only-non-binary-in-usr-lib We believe that because all of the scripts are meant to be just sourced and used as a library, /usr/lib is the right place for them. I can do the review, but I'm not able to sponsor you. The review must be done by the sponsor, at least according to Fedora policy. Jason, thanks ... it wasn't obvious to me from https://fedoraproject.org/wiki/Package_Review_Process that a sponsor could only do the review. I knew I wouldn't be able to complete the review, but I'd like to provide my review feedback anyway. According to https://fedoraproject.org/wiki/Packaging:ReviewGuidelines = General = Can the .spec summary be made more specific? Presently, it says "An operating system integration testing harness". Is that the correct summary for beakerlib? = MUST = * MUST: rpmlint must be run on every package. The output should be posted in the review. FAIL - I've attached a small spec patch to address several issues, but you will need to adjust further to address the issues identified below. > # rpmlint beakerlib-1.0-1.src.rpm > beakerlib.src:11: W: unversioned-explicit-obsoletes rhtslib > beakerlib.src:11: W: unversioned-explicit-obsoletes beaker-lib > beakerlib.src:12: W: unversioned-explicit-provides rhtslib > beakerlib.src:12: W: unversioned-explicit-provides beaker-lib Unsure ... may need to get a second opinion on this. > beakerlib.src:30: E: files-attr-not-set Corrected in attached patch > beakerlib.src:31: E: hardcoded-library-path in /usr/lib/beakerlib/*.sh I don't think bash source files should live in /usr/lib/beakerlib. According to the FHS (http://www.pathname.com/fhs/pub/fhs-2.3.html#USRLIBLIBRARIESFORPROGRAMMINGANDPA) - "/usr/lib includes object files, libraries, and internal binaries that are not intended to be executed directly by users or shell scripts. [22] Applications may use a single subdirectory under /usr/lib. If an application uses a subdirectory, all architecture-dependent data exclusively used by the application must be placed within that subdirectory." beakerlib does not contain architecture dependent files and does not include object files, libraries or internal binaries. I'd recommend moving bash content into a system-wide location such as /etc/profile.d/ > beakerlib.src:31: E: files-attr-not-set > beakerlib.src:32: E: files-attr-not-set > beakerlib.src:33: E: hardcoded-library-path in /usr/lib/beakerlib/virtualX.sh Same as above > beakerlib.src:33: E: files-attr-not-set > beakerlib.src:34: E: hardcoded-library-path in /usr/lib/beakerlib/python/rlMemAvg.py* > beakerlib.src:34: E: files-attr-not-set > beakerlib.src:35: E: hardcoded-library-path in /usr/lib/beakerlib/python/rlMemPeak.py* > beakerlib.src:35: E: files-attr-not-set > beakerlib.src:36: E: hardcoded-library-path in /usr/lib/beakerlib/python/journalling.py* > beakerlib.src:36: E: files-attr-not-set > beakerlib.src:37: E: hardcoded-library-path in /usr/lib/beakerlib/python/journal-compare.py* > beakerlib.src:37: E: files-attr-not-set Python content should be moved into %{python_sitelib}. See https://fedoraproject.org/wiki/Packaging:Python > beakerlib.src:38: E: hardcoded-library-path in /usr/lib/beakerlib/perl/deja-summarize Same, can this content be moved into the system perl directory? > beakerlib.src:38: E: files-attr-not-set > beakerlib.src:39: E: files-attr-not-set > beakerlib.src: W: no-cleaning-of-buildroot %install You have such a call in the .spec, but it is wrapped around a conditional check to ensure BUILD_ROOT != '/'. I've updated the .spec in in the attached patch to address this > beakerlib.src: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 3) Mixed use of tabs and spaces, I've converted to using spaces in the attached patch. > 1 packages and 0 specfiles checked; 17 errors, 6 warnings. > # rpmlint beakerlib-1.0-1.noarch.rpm > beakerlib.noarch: W: self-obsoletion beaker-lib obsoletes beaker-lib > beakerlib.noarch: W: self-obsoletion rhtslib obsoletes rhtslib > beakerlib.noarch: W: only-non-binary-in-usr-lib I believe this will be addressed by moving bash, python and perl content into standard system-wide locations. > 1 packages and 0 specfiles checked; 0 errors, 3 warnings. > # rpmlint beakerlib.spec > beakerlib.spec:11: W: unversioned-explicit-obsoletes rhtslib > beakerlib.spec:11: W: unversioned-explicit-obsoletes beaker-lib > beakerlib.spec:12: W: unversioned-explicit-provides rhtslib > beakerlib.spec:12: W: unversioned-explicit-provides beaker-lib > beakerlib.spec:33: E: hardcoded-library-path in /usr/lib/beakerlib/*.sh > beakerlib.spec:35: E: hardcoded-library-path in /usr/lib/beakerlib/virtualX.sh > beakerlib.spec:36: E: hardcoded-library-path in /usr/lib/beakerlib/python/rlMemAvg.py* > beakerlib.spec:37: E: hardcoded-library-path in /usr/lib/beakerlib/python/rlMemPeak.py* > beakerlib.spec:38: E: hardcoded-library-path in /usr/lib/beakerlib/python/journalling.py* > beakerlib.spec:39: E: hardcoded-library-path in /usr/lib/beakerlib/python/journal-compare.py* > beakerlib.spec:40: E: hardcoded-library-path in /usr/lib/beakerlib/perl/deja-summarize Addressed earlier > beakerlib.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 3) Addressed earlier > 0 packages and 1 specfiles checked; 7 errors, 5 warnings. * MUST: The package must be named according to the [[Packaging/NamingGuidelines| Package Naming Guidelines]] . PASS * MUST: The spec file name must match the base package <code>%{name}</code>, in the format <code>%{name}.spec</code> unless your package has an exemption. . PASS * MUST: The package must meet the [[Packaging/Guidelines| Packaging Guidelines]] . PASS * MUST: The package must be licensed with a Fedora approved license and meet the [[Packaging/LicensingGuidelines| Licensing Guidelines]] . PASS * MUST: The License field in the package spec file must match the actual license. FAIL - * I can't tell by looking at the code what the license is. You may wish to include a LICENSE file. * The Makefile lists '# License: GPL v2 or later', but the package is listed as GPLv2. if this is the case, you may wish to change the .spec file License: GPLv2+ * src/staf-rhts/BEAKERLIB.pm shows "Eclipse Public License (EPL) V1.0" which is not compatible with the GPLv2 (see http://fedoraproject.org/wiki/Licensing#SoftwareLicenses). * MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in <code>%doc</code>. No LICENSE file included, so not an issue. * MUST: The spec file must be written in American English. PASS * MUST: The spec file for the package MUST be legible. PASS * MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the [[Packaging/SourceURL| Source URL Guidelines]] for how to deal with this. FAIL - Please correct according to https://fedoraproject.org/wiki/Packaging/SourceURL * MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. WARN - while the package builds successfully, it won't build properly once the %files are adjusted to suitable system-wide locations. The build process will need to be adjusted. * MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in <code>ExcludeArch</code>. Each architecture listed in <code>ExcludeArch</code> MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding <code>ExcludeArch</code> line. * MUST: All build dependencies must be listed in <code>BuildRequires</code>, except for any that are listed in the [[Packaging/Guidelines#Exceptions_2|exceptions section of the Packaging Guidelines]] ; inclusion of those as <code>BuildRequires</code> is optional. Apply common sense. * MUST: The spec file MUST handle locales properly. This is done by using the <code>%find_lang</code> macro. Using <code>%{_datadir}/locale/*</code> is strictly forbidden. N/A * MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in <code>%post</code> and <code>%postun</code>. N/A * MUST: Packages must NOT bundle copies of system libraries.<ref name="no_bundle">[[Packaging:Guidelines#Duplication_of_system_libraries|Packaging Guidelines: Duplication of System Libraries]]</ref> PASS * MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. N/A * MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. FAIL - please see attached patch * MUST: A Fedora package must not list a file more than once in the spec file's %files listings. FAIL - please see attached patch * MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every <code>%files</code> section must include a <code>%defattr(...)</code> line. FAIL - please see attached patch * MUST: Each package must have a %clean section, which contains <code>rm -rf %{buildroot}</code> ([[Packaging/Guidelines#UsingBuildRootOptFlags|or $RPM_BUILD_ROOT]]). WARN - noted earlier in rpmlint output * MUST: Each package must consistently use macros. PASS * MUST: The package must contain code, or permissable content. PASS * 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). <ref name="docs">[[Packaging/Guidelines#PackageDocumentation|Packaging Guidelines: Package Documentation]]</ref> N/A * MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. <ref name="docs">[[Packaging/Guidelines#PackageDocumentation|Packaging Guidelines: Package Documentation]]</ref> N/A * MUST: Header files must be in a -devel package. <ref name="devel">[[Packaging/Guidelines#DevelPackages|Packaging Guidelines: Devel Packages]]</ref> N/A * MUST: Static libraries must be in a -static package. <ref name="static">[[Packaging/Guidelines#StaticLibraries|Packaging Guidelines: Packaging Static Libraries]]</ref> N/A * MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). <ref name="pkgconfig">[[Packaging/Guidelines#PkgconfigFiles|Packaging Guidelines: Pkgconfig Files]]</ref> N/A * MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. <ref name="devel">[[Packaging/Guidelines#DevelPackages|Packaging Guidelines: Devel Packages]]</ref> N/A * MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: <code>Requires: %{name} = %{version}-%{release} </code> <ref name="requirebase">[[Packaging/Guidelines#RequiringBasePackage|Packaging Guidelines: Requiring Base Package]]</ref> N/A * MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.<ref name="static">[[Packaging/Guidelines#StaticLibraries|Packaging Guidelines: Packaging Static Libraries]]</ref> N/A * MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. N/A * MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the <code>filesystem</code> or <code>man</code> package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. N/A * MUST: At the beginning of <code>%install</code>, each package MUST run <code>rm -rf %{buildroot}</code> ([[Packaging/Guidelines#UsingBuildRootOptFlags|or $RPM_BUILD_ROOT]]). FAIL - please see attached patch * MUST: All filenames in rpm packages must be valid UTF-8. PASS = SHOULD = * SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. Pmuller is upstream in this case, I've recommended including a LICENSE file * SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. N/A * SHOULD: The reviewer should test that the package builds in mock. PASS - tested using a koji scratch build. But additional spec changes will be needed http://koji.fedoraproject.org/koji/taskinfo?taskID=1965521 * SHOULD: The package should compile and build into binary rpms on all supported architectures. PASS * SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example. WARN - will wait for packaging to settle down first * SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. N/A * SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. <ref name="requirebase">[[Packaging/Guidelines#RequiringBasePackage|Packaging Guidelines: Requiring Base Package]]</ref> N/A * SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. <ref name="pkgconfig">[[Packaging/Guidelines#PkgconfigFiles|Packaging Guidelines: Pkgconfig Files]]</ref> N/A * SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. N/A * SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.<ref name="manpages">[[Packaging/Guidelines#Man_pages|Packaging Guidelines: Man Pages]]</ref> PASS (kudos!) (In reply to comment #4) > Jason, thanks ... it wasn't obvious to me from > https://fedoraproject.org/wiki/Package_Review_Process that a sponsor could only > do the review. I see now "The Reviewer can be any Fedora account holder, who is a member of the packager group. There is one exception: If it is the first package of a Contributor, the Reviewer must be a Sponsor.". Hopefully my review feedback is helpful. Created attachment 389172 [details]
spec file patch
> Can the .spec summary be made more specific? Presently, it says > "An operating system integration testing harness". Is that the > correct summary for beakerlib? What about using the summary from the wiki? Should be quite up-to-date and more specific: Summary: A shell-level integration testing library Description: BeakerLib is a shell-level integration testing library, providing convenience functions which simplify writing, running and analysis of integration and blackbox tests. The essential features are: * Journal - uniform logging mechanism (saved in XML format) * Phases - clear separation of setup / test / cleanup * Asserts - common checks (exit codes, check file existence & content) * Common operations - managing services, backup & restore > * MUST: The License field in the package spec file must match the actual > license. > > FAIL - > * I can't tell by looking at the code what the license is. You may wish to > include a LICENSE file. I'll add the LICENSE file as part of the documentation update patch. > * The Makefile lists '# License: GPL v2 or later', but the package is listed > as GPLv2. if this is the case, you may wish to change the .spec file License: > GPLv2+ > * src/staf-rhts/BEAKERLIB.pm shows "Eclipse Public License (EPL) V1.0" which > is not compatible with the GPLv2 (see > http://fedoraproject.org/wiki/Licensing#SoftwareLicenses). The staf-rhts directory should be removed altogether I guess. This is Beaker-Staff integration stuff which has not been used AFAIK. > BuildArch: noarch > %{_libdir}/%{name}/python/rlMemAvg.py* Bzzz! Noarch plus %_libdir won't fly, since the value of %_libdir would change with the build host. > Source0: %{name}-%{version}.tar.gz This is no valid URL where to download the tarball. If the tarball cannot be downloaded anywhere, please add a comment on how to construct it from a scm repository. > https://fedorahosted.org/git/beakerlib.git That redirects to: https://fedorahosted.org/web/410 No such project. The requested project does not exist on Fedora Hosted. > Obsoletes: rhtslib beaker-lib > Provides: rhtslib beaker-lib Have you tried "rpmlint -i ..." on the rpms? It's common practise to specify a max. version in Obsoletes tags in order to not occupy a namespace completely. And to specify an explicit EVR in Provides. Example: Obsoletes: rhtslib < 1.0 Provides: rhtslib = %{version}-%{release} If not doing that, this pair of Obsoletes/Provides becomes much more questionable. Look! On Fedora 12: $ repoquery --whatprovides rhtslib $ repoquery --whatprovides beaker-lib $ Nothing provides these so far. Even if it may be "old names": IMO, it's being frowned upon to provide alternative virtual package names just for fun. My suggestion: keep the Obsoletes with proper max. version, but don't add the Provides. It's silly to run with multiple alternative/competing packages names, whether virtual or not. > %makeinstall DESTDIR=$RPM_BUILD_ROOT Please prefer "make DESTDIR=$RPM_BUILD_ROOT install" over %makeinstall unless the normal "make install ..." doesn't work. The macro redefines many values, which bears a risk. > /usr/lib/beakerlib/python/journal-compare.py* Which packages owns /usr/lib/beakerlib/python/? > /usr/lib/beakerlib/perl/deja-summarize Which packages owns /usr/lib/beakerlib/perl/? http://fedoraproject.org/wiki/Packaging:UnownedDirectories > %{_mandir}/man1/beakerlib*1.gz Since man pages typically are compressed transparently by rpmbuild, prefer a wildcard over a file extension such as .gz: %{_mandir}/man1/beakerlib*1* Thanks you for all the feedback! I'll fix everything and let know... Spec URL: http://www.afri.cz/files/beakerlib.spec SRPM URL: http://www.afri.cz/files/beakerlib-1.0-2.src.rpm (In reply to comment #8) > Bzzz! Noarch plus %_libdir won't fly, since the value of %_libdir would change > with the build host. No libdir now. All scripts meant for sourcing are now in /usr/share/beakerlib/, all files meant for executing are in /usr/bin and prefixed 'beakerlib-'. > > Source0: %{name}-%{version}.tar.gz > This is no valid URL where to download the tarball. If the tarball cannot be > downloaded anywhere, please add a comment on how to construct it from a scm > repository. I placed the tarball at the website and provided a link to it. It's a link to Trac attachment. > > https://fedorahosted.org/git/beakerlib.git> > That redirects to: > https://fedorahosted.org/web/410 > No such project. > The requested project does not exist on Fedora Hosted. Sorry, the right URL is http://git.fedorahosted.org/git/beakerlib.git. Anyways, Source now point directly to the tarball attachment, not osurce repo. > > Obsoletes: rhtslib beaker-lib > > Provides: rhtslib beaker-lib > Nothing provides these so far. Even if it may be "old names": IMO, it's being > frowned upon to provide alternative virtual package names just for fun. My > suggestion: keep the Obsoletes with proper max. version, but don't add the > Provides. It's silly to run with multiple alternative/competing packages names, > whether virtual or not. We've decided to remove these entirely, as the names are not present in Fedora. > > %makeinstall DESTDIR=$RPM_BUILD_ROOT > > Please prefer "make DESTDIR=$RPM_BUILD_ROOT install" over %makeinstall unless > the normal "make install ..." doesn't work. The macro redefines many values, > which bears a risk. Fixed > > /usr/lib/beakerlib/python/journal-compare.py* > Which packages owns /usr/lib/beakerlib/python/? > > /usr/lib/beakerlib/perl/deja-summarize > Which packages owns /usr/lib/beakerlib/perl/? Not applicable now. Package now needs /usr/share/beakerlib and /usr/share/doc/beakerlib, both are owned by it. > > %{_mandir}/man1/beakerlib*1.gz > > Since man pages typically are compressed transparently by rpmbuild, prefer a > wildcard over a file extension such as .gz: > %{_mandir}/man1/beakerlib*1* Fixed rpmlint is now silent on the package (In reply to comment #4) > Can the .spec summary be made more specific? Presently, it says "An operating > system integration testing harness". Is that the correct summary for > beakerlib? I've put there what Petr Splichal proposed in comment 5 > * MUST: rpmlint must be run on every package. The output should be posted in > the review. > > FAIL - I've attached a small spec patch to address several issues, but you will > need to adjust further to address the issues identified below. rpmlint is now silent on the package > FAIL - > * I can't tell by looking at the code what the license is. You may wish to > include a LICENSE file. > * The Makefile lists '# License: GPL v2 or later', but the package is listed > as GPLv2. if this is the case, you may wish to change the .spec file License: > GPLv2+ > * src/staf-rhts/BEAKERLIB.pm shows "Eclipse Public License (EPL) V1.0" which > is not compatible with the GPLv2 (see > http://fedoraproject.org/wiki/Licensing#SoftwareLicenses). staf-rhts was removed. The correct license is GPLv2 only and all files should have standardized license infirmation. > * MUST: If (and only if) the source package includes the text of the license(s) > in its own file, then that file, containing the text of the license(s) for the > package must be included in <code>%doc</code>. > > No LICENSE file included, so not an issue. LICENSE now included and tagged by %doc > FAIL - Please correct according to > https://fedoraproject.org/wiki/Packaging/SourceURL Fixed, see previous comments > * MUST: The package MUST successfully compile and build into binary rpms on at > least one primary architecture. > > WARN - while the package builds successfully, it won't build properly once the > %files are adjusted to suitable system-wide locations. The build process will > need to be adjusted. Paths were fixed, see previous comments. > * MUST: A package must own all directories that it creates. If it does not > create a directory that it uses, then it should require a package which does > create that directory. > > FAIL - please see attached patch Fixed, see previous comment > * MUST: A Fedora package must not list a file more than once in the spec file's > %files listings. > > FAIL - please see attached patch Fixed > * MUST: Permissions on files must be set properly. Executables should be set > with executable permissions, for example. Every <code>%files</code> section > must include a <code>%defattr(...)</code> line. > > FAIL - please see attached patch I believe all files have right permissions. > * MUST: Each package must have a %clean section, which contains <code>rm -rf > %{buildroot}</code> ([[Packaging/Guidelines#UsingBuildRootOptFlags|or > $RPM_BUILD_ROOT]]). > > WARN - noted earlier in rpmlint output Fixed. > * MUST: At the beginning of <code>%install</code>, each package MUST run > <code>rm -rf %{buildroot}</code> > ([[Packaging/Guidelines#UsingBuildRootOptFlags|or $RPM_BUILD_ROOT]]). > > FAIL - please see attached patch Fixed. > * SHOULD: If the source package does not include license text(s) as a separate > file from upstream, the packager SHOULD query upstream to include it. > > Pmuller is upstream in this case, I've recommended including a LICENSE file Included. > Spec URL: http://www.afri.cz/files/beakerlib.spec > SRPM URL: http://www.afri.cz/files/beakerlib-1.0-2.src.rpm The src.rpm URL does not work, I've instead used http://afri.fedorapeople.org/beakerlib/beakerlib-1.0-2.fc12.src.rpm [ FAIL ] MUST: rpmlint must be run on every package beakerlib.src: W: invalid-url Source0: beakerlib-1.0.tar.gz beakerlib.spec: W: invalid-url Source0: beakerlib-1.0.tar.gz I believe you mixed the definitions of URL and Source0. The following patch resolves the problem accordingly. --- /tmp/jlaska-rpm/SPECS/beakerlib.spec 2010-02-12 11:00:20.000000000 -0500 +++ beakerlib.spec 2010-02-22 19:07:48.534865389 -0500 @@ -8 +8 @@ -Source0: %{name}-%{version}.tar.gz +Source0: https://fedorahosted.org/%{name}/attachment/wiki/tarballs/%{name}-%{version}.tar.gz @@ -10 +10 @@ -URL: https://fedorahosted.org/beakerlib/attachment/wiki/tarballs/beakerlib-1.0.tar.gz +URL: https://fedorahosted.org/%{name} [ OK ] MUST: The package must be named according to the Package Naming Guidelines [ OK ] MUST: The spec file name must match the base package %{name} [...] [ OK ] MUST: The package must meet the Packaging Guidelines [ OK ] MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines [ FAIL ] MUST: The License field in the package spec file must match the actual license beakerlib.spec noted GPLv2, however the following files included in beakerlib show 'GPLv2 or later': git_rules.mk:1:# License: GPL v2 or later py_rules.mk:1:# License: GPL v2 or later rpmspec_rules.mk:1:# License: GPL v2 or later upload_rules.mk:1:# License: GPL v2 or later scm_rules.mk:1:# License: GPL v2 or later I think beakerlib.spec should note "GPLv2+" or the *.mk files should be adjusted [ OK ] MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc [ OK ] MUST: The spec file must be written in American English. [ WARN ] MUST: The spec file for the package MUST be legible. It's common practice to include two newlines prior to each %section in the spec file. I'd recommend the same here for readability. Please see the tool rpmdev-newspec (provided by rpmdevtools) or the spec template included in vim now. [ FAIL ] MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this. The Source0 line must be a URL to the location of the source upstream. Something like (note, the URL will depend on the location you post this, but I'd recommend anchored somewhere from your fedorahosted project space) ... Source0: http://fedorahosted.org/releases/b/e/beakerlib/beakerlib-%{version}.tar.bz2 See http://fedoraproject.org/wiki/Packaging/SourceURL for more info [ OK ] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture koji scratch build - http://koji.fedoraproject.org/koji/taskinfo?taskID=2007048 [ N/A ] MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line [ OK ] MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense. [ N/A ] MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden [ N/A ] MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. [ N/A ] MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. [ OK ] MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. [ OK ] MUST: A package must not contain any duplicate files in the %files listing. [ OK ] MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. [ OK ] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [ OK ] MUST: Each package must consistently use macros. [ OK ] MUST: The package must contain code, or permissable content. [ N/A ] 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). [ OK ] MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. [ N/A ] MUST: Header files must be in a -devel package. [ N/A ] MUST: Static libraries must be in a -static package. [ N/A ] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). [ N/A ] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. [ N/A ] MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} [ N/A ] MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built. [ N/A ] MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. [ OK ] MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. [ OK ] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [ OK ] MUST: All filenames in rpm packages must be valid UTF-8. [ N/A ] SHOULD: If the source package does not include license text(s) as a separate file from upstream, should query upstream to include it. [ N/A ] SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. [ OK ] SHOULD: The reviewer should test that the package builds in mock. koji scratch build - http://koji.fedoraproject.org/koji/taskinfo?taskID=2007048 [ OK ] SHOULD: The package should compile and build into binary rpms on all supported architectures. See above [ OK ] SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example. Being used in the AutoQA project https://fedorahosted.org/pipermail/autoqa-devel/2010-February/000217.html [ N/A ] SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. [ N/A ] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. [ N/A ] SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. [ OK ] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. [ OK ] SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.[34] /usr/share/man/man1/beakerlib-analyze.1.gz /usr/share/man/man1/beakerlib-beakerlib.1.gz /usr/share/man/man1/beakerlib-infrastructure.1.gz /usr/share/man/man1/beakerlib-journal.1.gz /usr/share/man/man1/beakerlib-logging.1.gz /usr/share/man/man1/beakerlib-performance.1.gz /usr/share/man/man1/beakerlib-rpms.1.gz /usr/share/man/man1/beakerlib-testing.1.gz /usr/share/man/man1/beakerlib-virtualX.1.gz /usr/share/man/man1/beakerlib.1.gz (In reply to comment #12) > > Spec URL: http://www.afri.cz/files/beakerlib.spec > > SRPM URL: http://www.afri.cz/files/beakerlib-1.0-2.src.rpm > The src.rpm URL does not work, I've instead used > http://afri.fedorapeople.org/beakerlib/beakerlib-1.0-2.fc12.src.rpm Sorry for that, I b0rked the filename. New ones: http://www.afri.cz/files/beakerlib-1.1-0.fc12.src.rpm http://www.afri.cz/files/beakerlib.spec > > [ FAIL ] MUST: rpmlint must be run on every package > beakerlib.src: W: invalid-url Source0: beakerlib-1.0.tar.gz > beakerlib.spec: W: invalid-url Source0: beakerlib-1.0.tar.gz > > I believe you mixed the definitions of URL and Source0. The following > patch resolves the problem accordingly. Thanks for the patch - it should be fixed now. Source0: https://fedorahosted.org/released/%{name}/%{name}-%{version}.tar.gz URL: https://fedorahosted.org/%{name} $ rpmlint beakerlib-1.1-0.fc12.src.rpm beakerlib-1.1-0.fc12.noarch.rpm 2 packages and 0 specfiles checked; 0 errors, 0 warnings. > [ FAIL ] MUST: The License field in the package spec file must match the > actual license > > beakerlib.spec noted GPLv2, however the following files included in > beakerlib show 'GPLv2 or later': > git_rules.mk:1:# License: GPL v2 or later > py_rules.mk:1:# License: GPL v2 or later > rpmspec_rules.mk:1:# License: GPL v2 or later > upload_rules.mk:1:# License: GPL v2 or later > scm_rules.mk:1:# License: GPL v2 or later > > I think beakerlib.spec should note "GPLv2+" or the *.mk files should > be adjusted Fixed to GPLv2, one file was removed. > [ WARN ] MUST: The spec file for the package MUST be legible. > > It's common practice to include two newlines prior to each %section in > the spec file. I'd recommend the same here for readability. Please > see the tool rpmdev-newspec (provided by rpmdevtools) or the spec > template included in vim now. Thanks, I didn't know that. I've added empty lines between sections. > [ FAIL ] MUST: The sources used to build the package must match the upstream > source, as provided in the spec URL. Reviewers should use md5sum for > this task. If no upstream URL can be specified for this package, > please see the Source URL Guidelines for how to deal with this. > > The Source0 line must be a URL to the location of the source upstream. > Something like (note, the URL will depend on the location you post > this, but I'd recommend anchored somewhere from your fedorahosted > project space) ... I've moved the tarball to the fedorahosted release infrastructure, the tarballs will be here now: https://fedorahosted.org/released/beakerlib/ Thanks for the updates Petr. Those changes look good. While re-reviewing the licensing, I noticed that the file 'src/test/shunit2' lists LGPL in the header. There's the issue of lining up the licensing information of shunit2. Before handling lining up with the license in the header of 'src/test/shunit2', I think we need to clarify whether this constitutes bundling a library. Is this code upstream somewhere? Perhaps one of these projects? * shunit - http://shunit.sourceforge.net/ * shunit2 - http://code.google.com/p/shunit2/ Can you explain how shunit2 is used? The 'src/test/README' file doesn't seem to have correct links to the project. Either way, I suspect we might not be allowed to include another project inside beakerlib. I have a suspicion shunit2 would need to be packaged separately in Fedora. Yeah, you really do want to package shunit2 independently. For now, I have removed the offending file entirely - we will replace it with a simple minilibrary which we will write. Or perhaps package shunit2, I don't know now, but I don't want this to be blocker of beakerlib in Fedor a. New links: http://www.afri.cz/files/beakerlib-1.2-0.fc12.src.rpm http://www.afri.cz/files/beakerlib.spec Looks good Petr, thanks for the updates. I can confirm that shunit2 is no longer packaged with beakerlib, and from what I can tell, all license information appears kosher. # rpmlint beakerlib-1.2-0.fc13.noarch.rpm beakerlib-1.2-0.fc12.src.rpm beakerlib.spec 2 packages and 1 specfiles checked; 0 errors, 0 warnings. tcallawa: Any other packaging concerns you can think of? Nope. :) Some pedantic stuff: o why start release with 0 and not 1? o remove trailing ; here: %clean rm -rf $RPM_BUILD_ROOT; o add a comment why %build is empty o use INSTALL='install -p' in make install to preserve timestamps o it's very common to have documentation in %{_docdir}/%{name}-%{version}, any reason not the follow that? Petr: Can you address the questions in comment #19? I can look at reviewing this formally and sponsoring you after that... Thanks for your points! (In reply to comment #19) > Some pedantic stuff: > > o why start release with 0 and not 1? Well, I tend to start everything from zero :) I will keep that in mind for the next time > o remove trailing ; here: > %clean > rm -rf $RPM_BUILD_ROOT; done > o add a comment why %build is empty added > o use INSTALL='install -p' in make install to preserve timestamps fix'd > o it's very common to have documentation in %{_docdir}/%{name}-%{version}, > any reason not the follow that? I've changed Makefiles and specfile to follow that. (In reply to comment #20) > Petr: Can you address the questions in comment #19? > > I can look at reviewing this formally and sponsoring you after that... Sorry for the delay, I was travelling and I avoided computer stuff for a while :) New links: http://www.afri.cz/files/beakerlib-1.2-1.fc12.src.rpm http://www.afri.cz/files/beakerlib.spec Look for a full review in a bit. OK - Package meets naming and packaging guidelines OK - Spec file matches base package name. OK - Spec has consistant macro usage. OK - Meets Packaging Guidelines. OK - License (GPLv2) OK - License field in spec matches OK - License file included in package OK - Spec in American English OK - Spec is legible. See below - Sources match upstream md5sum: 5e5ee854add958ce30746a15b9d7e713 beakerlib-1.2.tar.gz ae18ea068c48e82196ff6cd381e663d9 beakerlib-1.2.tar.gz.orig OK - BuildRequires correct See below - Package has %defattr and permissions on files is good. OK - Package has a correct %clean section. OK - Package has correct buildroot OK - Package is code or permissible content. OK - Packages %doc files don't affect runtime. OK - Package has rm -rf RPM_BUILD_ROOT at top of %install OK - Package compiles and builds on at least one arch. OK - Package has no duplicate files in %files. OK - Package doesn't own any directories other packages own. OK - Package owns all the directories it creates. OK - Package obey's FHS standard (except for 2 exceptions) See below - No rpmlint output. See below - final provides and requires are sane. SHOULD Items: OK - Should build in mock. OK - Should build on all supported archs OK - Should have dist tag OK - Should package latest version OK - Should not use file requires outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin Issues: 1. Source doesn't match from upstream: 5e5ee854add958ce30746a15b9d7e713 beakerlib-1.2.tar.gz ae18ea068c48e82196ff6cd381e663d9 beakerlib-1.2.tar.gz.orig 2. rpmlint says: Probibly all these should be chmod 644 or have a #!/bin/sh added if they should be called stand alone: beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/analyze.sh beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/beakerlib.sh beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/dictionary.vim beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/infrastructure.sh beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/journal.sh beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/logging.sh beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/performance.sh beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/rpms.sh beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/testing.sh beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/virtualX.sh This can be ignored, but it would be nice to have man pages for these: beakerlib.noarch: W: no-manual-page-for-binary beakerlib-deja-summarize beakerlib.noarch: W: no-manual-page-for-binary beakerlib-journalcmp beakerlib.noarch: W: no-manual-page-for-binary beakerlib-journalling beakerlib.noarch: W: no-manual-page-for-binary beakerlib-rlMemAvg beakerlib.noarch: W: no-manual-page-for-binary beakerlib-rlMemPeak All these should be mode 644: beakerlib.noarch: W: spurious-executable-perm /usr/share/doc/beakerlib-1.2/LICENSE beakerlib.noarch: W: spurious-executable-perm /usr/share/man/man1/beakerlib.1.gz beakerlib.noarch: W: spurious-executable-perm /usr/share/man/man1/beakerlib-analyze.1.gz beakerlib.noarch: W: spurious-executable-perm /usr/share/man/man1/beakerlib-beakerlib.1.gz beakerlib.noarch: W: spurious-executable-perm /usr/share/man/man1/beakerlib-infrastructure.1.gz beakerlib.noarch: W: spurious-executable-perm /usr/share/man/man1/beakerlib-journal.1.gz beakerlib.noarch: W: spurious-executable-perm /usr/share/man/man1/beakerlib-logging.1.gz beakerlib.noarch: W: spurious-executable-perm /usr/share/man/man1/beakerlib-performance.1.gz beakerlib.noarch: W: spurious-executable-perm /usr/share/man/man1/beakerlib-rpms.1.gz beakerlib.noarch: W: spurious-executable-perm /usr/share/man/man1/beakerlib-testing.1.gz beakerlib.noarch: W: spurious-executable-perm /usr/share/man/man1/beakerlib-virtualX.1.gz 3. Shouldn't this have a 'Requires: python2' ? 4. Should you add the examples as %doc files? Kevin, first of all, thanks for reviewing! New links: http://www.afri.cz/files/beakerlib-1.3-1.fc12.src.rpm http://www.afri.cz/files/beakerlib.spec (In reply to comment #24) > Issues: > > 1. Source doesn't match from upstream: > 5e5ee854add958ce30746a15b9d7e713 beakerlib-1.2.tar.gz > ae18ea068c48e82196ff6cd381e663d9 beakerlib-1.2.tar.gz.orig3 It should match now. The fact that we are also upstream (and we track the specfile in repo, and it then gets in the tarball) makes this a bit complicated to maintain, because the tarball changes everytime I change even the specfile. > > 2. rpmlint says: > > Probibly all these should be chmod 644 or have a #!/bin/sh added if they should > be called stand alone: > > beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/analyze.sh > beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/beakerlib.sh > beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/dictionary.vim > beakerlib.noarch: E: script-without-shebang > /usr/share/beakerlib/infrastructure.sh > beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/journal.sh > beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/logging.sh > beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/performance.sh > beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/rpms.sh > beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/testing.sh > beakerlib.noarch: E: script-without-shebang /usr/share/beakerlib/virtualX.sh Not intended for standalone calling, I've fixed the perms. > This can be ignored, but it would be nice to have man pages for these: > > beakerlib.noarch: W: no-manual-page-for-binary beakerlib-deja-summarize > beakerlib.noarch: W: no-manual-page-for-binary beakerlib-journalcmp > beakerlib.noarch: W: no-manual-page-for-binary beakerlib-journalling > beakerlib.noarch: W: no-manual-page-for-binary beakerlib-rlMemAvg > beakerlib.noarch: W: no-manual-page-for-binary beakerlib-rlMemPeak Good suggestion. We'll try to write something for the ones which are supposed to be executed directly, not from inside Beakerlib > All these should be mode 644: > > beakerlib.noarch: W: spurious-executable-perm > /usr/share/doc/beakerlib-1.2/LICENSE > beakerlib.noarch: W: spurious-executable-perm > /usr/share/man/man1/beakerlib.1.gz > beakerlib.noarch: W: spurious-executable-perm > /usr/share/man/man1/beakerlib-analyze.1.gz > beakerlib.noarch: W: spurious-executable-perm > /usr/share/man/man1/beakerlib-beakerlib.1.gz > beakerlib.noarch: W: spurious-executable-perm > /usr/share/man/man1/beakerlib-infrastructure.1.gz > beakerlib.noarch: W: spurious-executable-perm > /usr/share/man/man1/beakerlib-journal.1.gz > beakerlib.noarch: W: spurious-executable-perm > /usr/share/man/man1/beakerlib-logging.1.gz > beakerlib.noarch: W: spurious-executable-perm > /usr/share/man/man1/beakerlib-performance.1.gz > beakerlib.noarch: W: spurious-executable-perm > /usr/share/man/man1/beakerlib-rpms.1.gz > beakerlib.noarch: W: spurious-executable-perm > /usr/share/man/man1/beakerlib-testing.1.gz > beakerlib.noarch: W: spurious-executable-perm > /usr/share/man/man1/beakerlib-virtualX.1.gz Perms fixed > 3. Shouldn't this have a 'Requires: python2' ? Seems so. Added. > 4. Should you add the examples as %doc files? Good idea, thanks! Added. 1. Looks good. 13df10e1b94c8a1abee91e9ef111dd29 beakerlib-1.3.tar.gz 13df10e1b94c8a1abee91e9ef111dd29 beakerlib-1.3.tar.gz.orig 2. Looks good. rpmlint now says: beakerlib.noarch: W: no-manual-page-for-binary beakerlib-rlMemAvg beakerlib.noarch: W: no-manual-page-for-binary beakerlib-rlMemPeak beakerlib.noarch: W: no-manual-page-for-binary beakerlib-journalling beakerlib.noarch: W: no-manual-page-for-binary beakerlib-journalcmp beakerlib.noarch: W: no-manual-page-for-binary beakerlib-deja-summarize 3. Looks good. 4. Looks good. I don't see any further blockers here, so this package was APPROVED. I will go ahead and sponsor you. What is your Fedora Account system account? Once you are sponsored, you can continue the process at: https://fedoraproject.org/wiki/PackageMaintainers/Join#Add_Package_to_CVS_and_Set_Owner Kevin, Thanks for sponsoring. My FAS account name is 'afri'. Sorry for the delay, I was off traveling. ;( I have sponsored you now. Please continue from https://fedoraproject.org/wiki/PackageMaintainers/Join#Add_Package_to_CVS_and_Set_Owner If you have any questions at all, please feel free to email me or catch me on irc (my nick is 'nirik'). New Package CVS Request ======================= Package Name: beakerlib Short Description: A shell-level integration testing library Owners: afri Branches: F-13 InitialCC: CVS Done beakerlib-1.3-1.fc13 has been submitted as an update for Fedora 13. http://admin.fedoraproject.org/updates/beakerlib-1.3-1.fc13 beakerlib-1.3-1.fc13 has been pushed to the Fedora 13 stable repository. If problems still persist, please make note of it in this bug report. Package Change Request ====================== Package Name: beakerlib New Branches: el5 el6 epel7 Owners: afri Just want to maintain this package in EPEL too Git done (by process-git-requests). |