Bug 835275
| Summary: | Review Request: shflags - Simple handling of command-line flags in Bourne based Unix scripts | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Ralph Bean <rbean> |
| Component: | Package Review | Assignee: | Garrett Holmstrom <gholms> |
| Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | gholms, mspaulding06, notting, package-review |
| Target Milestone: | --- | Flags: | gholms:
fedora-review+
gwync: fedora-cvs+ |
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | Bug Fix | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2012-07-07 18:46:41 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: | 835657 | ||
|
Description
Ralph Bean
2012-06-25 20:27:37 UTC
This is an unofficial practice review. Rpmlint Output: shflags.noarch: W: spelling-error %description -l en_US ksh -> ks, sh, ssh shflags.noarch: W: spelling-error %description -l en_US zsh -> sh, ssh, ash shflags.noarch: W: spelling-error %description -l en_US getopt -> get opt, get-opt, treetop shflags.noarch: W: spelling-error %description -l en_US google -> Google, goggle, googly shflags.noarch: W: spelling-error %description -l en_US gflags -> flags, gulags, g flags shflags.noarch: E: incorrect-fsf-address /usr/share/doc/shflags-1.0.3/doc/LICENSE.shunit2 shflags.noarch: E: incorrect-fsf-address /usr/share/doc/shflags-1.0.3/doc/LGPL-2.1 1 packages and 0 specfiles checked; 2 errors, 5 warnings. Package Review: - package meets package naming guidelines - legible and in American English - license LGPLv2 is slightly ambiguous (see below) - license file included in %doc section - MD5 sum on tarball matches that of the upstream tarball - no missing BuildRequires - util-linux BuildRequires is unnecessary (see below) - no locales included - not a relocatable package - does not own all directories it creates (see below) - no duplicate files in %files section and all file names are valid utf-8 - needs to preserve timestamps on file copy (see below) - consistent macro use - might want to have a -docs subpackage, though not required - example scripts in %doc should not be executable (see below) - no bundled system libraries - no need for .desktop file - builds on x86 successfully in mock - verified that shflags works as expected using examples included in the tarball - included tests are not being run (see below) Fixes: - I believe util-linux is unnecessary in BuildRequires section and should be removed. The package exception guidelines mention util-linux-ng which is essentially the same package - contact upstream to fix incorrect fsf address (which you have commented you are doing) - contact upstream regardling license type. It's not clear if this is LGPLv2 or LGPLv2+ - no man pages, maybe work with upstream to get these added - make example scripts not executable with "chmod -x" - when copying files in %install section use "cp -p" to preserve timestamps - need to own /usr/share/doc/shflags directory in %files section - tests included in tarball should be run in the %check section Matt's review looks pretty close to me. Here are the issues I found:
The sources don't indicate a LGPL version, but the source tree includes LGPL-2.1. This likely indicates LGPLv2+, but it might be worth double-checking with upstream to ensure it isn't LGPLv2.
BuildRequires: util-linux is superfluous because it appears in the BuildRequires exception list [1].
The package doesn't own %{_datadir}/%{name}.
The %doc files under the "examples" directory are executable, causing it to drag in /bin/sh unnecessarily. While that's a trifling issue, it's probably worth proactively fixing it to prevent future updates from accidentally pulling in other things.
The test programs under the "src" directory aren't executed during %check.
The packaging guidelines suggest using cp's -p option to preserve file timestamps. This one isn't required; just best practice.
Just fix those and you should be good to go! An exhaustive review follows.
[1] https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2
== Review of shflags-1.0.3-1 ==
Mandatory review guidelines:
ok - rpmlint output:
shflags.noarch: W: spelling-error %description -l en_US ksh -> ks, sh, ssh
shflags.noarch: W: spelling-error %description -l en_US zsh -> sh, ssh, ash
shflags.noarch: W: spelling-error %description -l en_US getopt -> get opt, get-opt, treetop
shflags.noarch: W: spelling-error %description -l en_US google -> Google, goggle, googly
shflags.noarch: W: spelling-error %description -l en_US gflags -> flags, gulags, g flags
shflags.noarch: E: incorrect-fsf-address /usr/share/doc/shflags-1.0.3/doc/LICENSE.shunit2
shflags.noarch: E: incorrect-fsf-address /usr/share/doc/shflags-1.0.3/doc/LGPL-2.1
shflags.src: W: spelling-error %description -l en_US ksh -> ks, sh, ssh
shflags.src: W: spelling-error %description -l en_US zsh -> sh, ssh, ash
shflags.src: W: spelling-error %description -l en_US getopt -> get opt, get-opt, treetop
shflags.src: W: spelling-error %description -l en_US google -> Google, goggle, googly
shflags.src: W: spelling-error %description -l en_US gflags -> flags, gulags, g flags
2 packages and 0 specfiles checked; 2 errors, 10 warnings.
You've already addressed the FSF address issue. The rest look harmless.
ok - License is acceptable (LGPLv2)
?? - License field in spec is correct
The sources don't indicate a LGPL version. Is upstream okay with
LGPLv2+ or is it only LGPLv2?
ok - License files included in package %docs if included in source package
ok - License files installed when any subpackage combination is installed
ok - Spec written in American English
ok - Spec is legible
ok - Sources match upstream unless altered to fix permissibility issues
Upstream MD5: b4d7133696ec05b71b27d8df5e278f0f shflags-1.0.3.tgz
Your MD5: b4d7133696ec05b71b27d8df5e278f0f shflags-1.0.3.tgz
ok - Build succeeds on at least one primary arch
ok - Build succeeds on all primary arches or has ExcludeArch + bugs filed
NO - BuildRequires correct
util-linux is part of the BuildRequires exception list.
-- - Locales handled with %find_lang, not %_datadir/locale/*
-- - %post, %postun call ldconfig if package contains shared .so files
ok - No bundled libs
-- - Relocatability is justified
NO - Package owns all directories it creates
Missing %{_datadir}/%{name}
-- - Package requires others for directories it uses but does not own
ok - No duplication in %files unless necessary for license files
NO - File permissions are sane
-rwxr-xr-x root root /usr/share/doc/shflags-1.0.3/examples/debug_output.sh
-rwxr-xr-x root root /usr/share/doc/shflags-1.0.3/examples/hello_world.sh
-rwxr-xr-x root root /usr/share/doc/shflags-1.0.3/examples/write_date.sh
ok - Package contains permissible code or content
-- - Large docs go in -doc subpackage
ok - %doc files not required at runtime
-- - Static libs go in -static package/virtual Provides
-- - Development files go in -devel package
-- - -devel packages Require base with fully-versioned dependency, %_isa
ok - No .la files
-- - GUI app uses .desktop file, installs it with desktop-file-install
ok - File list does not conflict with other packages' without justification
ok - File names are valid UTF-8
Optional review guidelines:
-- - Query upstream about including license files
no - Translations of description, summary
ok - Builds in mock
ok - Builds on all arches
ok - Functions as described (e.g. no crashes)
-- - Scriptlets are sane
-- - Subpackages require base with fully-versioned dependency if sensible
-- - .pc file subpackage placement is sensible
ok - No file deps outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin
-- - Include man pages if available
Naming guidelines:
ok - Package names use only a-zA-Z0-9-._+ subject to restrictions on -._+
ok - Package names are sane
ok - No naming conflicts
ok - Spec file name matches base package name
ok - Version is sane
ok - Version does not contain ~
ok - Release is sane
ok - %dist tag
ok - Case used only when necessary
-- - Renaming handled correctly
Packaging guidelines:
ok - Useful without external bits
ok - No kmods
ok - Pre-built binaries, libs removed in %prep
ok - Sources contain only redistributable code or content
ok - Spec format is sane
ok - Package obeys FHS, except libexecdir, /run, /usr/target
ok - No files in /bin, /sbin, /lib* on >= F17
-- - Programs run before FS mounting use /run instead of /var/run
-- - Binaries in /bin, /sbin do not depend on files in /usr on < F17
ok - No files under /srv, /opt, /usr/local
ok - Changelog in prescribed format
ok - No Packager, Vendor, Copyright, PreReq tags
ok - Summary does not end in a period
-- - Correct BuildRoot tag on < EL6
-- - Correct %clean section on < EL6
ok - Requires correct, justified where necessary
ok - Summary, description do not use trademarks incorrectly
ok - All relevant documentation is packaged, appropriately marked with %doc
NO - Doc files do not drag in extra dependencies (e.g. due to +x)
Executables in "examples" dir drag in /bin/sh. This is rather
trifling, but chmod'ing them now can help prevent updates' causing
issues in the future.
-- - Code compilable with gcc is compiled with gcc
-- - Build honors applicable compiler flags or justifies otherwise
-- - PIE used for long-running/root daemons, setuid/filecap programs
-- - Useful -debuginfo package or disabled and justified
-- - Package with .pc files Requires pkgconfig on < EL6
ok - No static executables
-- - Rpath absent or only used for internal libs
-- - Config files marked with %config(noreplace) or justified %config
ok - No config files under /usr
-- - Third party package manager configs acceptable, in %_docdir
-- - .desktop files are sane
ok - Spec uses macros consistently
ok - Spec uses macros instead of hard-coded names where appropriate
ok - Spec uses macros for executables only when configurability is needed
-- - %makeinstall used only when alternatives don't work
-- - Macros in Summary, description are expandable at srpm build time
-- - Spec uses %{SOURCE#} instead of $RPM_SOURCE_DIR and %sourcedir
ok - No software collections (scl)
ok - Build uses only python/perl/shell+coreutils/lua/BuildRequired langs
-- - %global, not %define
-- - Package translating with gettext BuildRequires it
-- - Package translating with Linguist BuildRequires qt-devel
no - File ops preserve timestamps
Though optional, the packaging guidelines recommend cp's -p option.
-- - Parallel make
ok - No Requires(pre,post) notation
-- - User, group creation handled correctly (See Packaging:UsersAndGroups)
-- - Web apps go in /usr/share/%name, not /var/www
-- - Conflicts are justified
ok - One project per package
ok - No bundled fonts
-- - Patches have appropriate commentary
NO - Available test suites executed in %check
If running the tests isn't practical then please drop the util-linux
build dep.
-- - tmpfiles.d used for /run, /run/lock on >= F15
Thanks guys. Here's a new release with:
- Updated license field to LGPLv2+
- Removed BuildRequires util-linux. Superfluous!
- Ownership taken for %%{_datadir}/%%{name}
- Copying with '-p' to preserve
- Made examples non-exectuable.
- Added %%check section.
Spec URL: http://threebean.org/rpm/shflags.spec
SRPM URL: http://threebean.org/rpm/shflags-1.0.3-2.fc17.src.rpm
I just emailed upstream directly about the license ambiguity. I changed the license tag in the spec file to LGPLv2+ in the meantime at Garrett's suggestion.
Its tests don't seem to pass on F16; I hope that's okay. Everything else looks fine to me. Just don't forget to fix the License field if it turns out to be LGPLv2 after all.
Enjoy!
== Review of shflags-1.0.3-2 ==
Mandatory review guidelines:
ok - rpmlint output:
shflags.noarch: W: spelling-error %description -l en_US ksh -> ks, sh, ssh
shflags.noarch: W: spelling-error %description -l en_US zsh -> sh, ssh, ash
shflags.noarch: W: spelling-error %description -l en_US getopt -> get opt, get-opt, treetop
shflags.noarch: W: spelling-error %description -l en_US google -> Google, goggle, googly
shflags.noarch: W: spelling-error %description -l en_US gflags -> flags, gulags, g flags
shflags.noarch: E: incorrect-fsf-address /usr/share/doc/shflags-1.0.3/doc/LICENSE.shunit2
shflags.noarch: E: incorrect-fsf-address /usr/share/doc/shflags-1.0.3/doc/LGPL-2.1
shflags.src: W: spelling-error %description -l en_US ksh -> ks, sh, ssh
shflags.src: W: spelling-error %description -l en_US zsh -> sh, ssh, ash
shflags.src: W: spelling-error %description -l en_US getopt -> get opt, get-opt, treetop
shflags.src: W: spelling-error %description -l en_US google -> Google, goggle, googly
shflags.src: W: spelling-error %description -l en_US gflags -> flags, gulags, g flags
shflags.src: W: invalid-url Source0: http://shflags.googlecode.com/files/shflags-1.0.3.tgz HTTP Error 404: Not Found
2 packages and 0 specfiles checked; 2 errors, 11 warnings.
--
You've already addressed the FSF address issue. The 404 is due to
Google's badly-behaved web servers. The rest are harmless.
ok - License is acceptable (LGPLv2+)
ok - License field in spec is correct
Please make sure to fix the License field if upstream disagrees
with the LGPLv2+ assessment.
ok - License files included in package %docs if included in source package
ok - License files installed when any subpackage combination is installed
ok - Spec written in American English
ok - Spec is legible
ok - Sources match upstream unless altered to fix permissibility issues
Upstream MD5: b4d7133696ec05b71b27d8df5e278f0f shflags-1.0.3.tgz
Your MD5: b4d7133696ec05b71b27d8df5e278f0f shflags-1.0.3.tgz
ok - Build succeeds on at least one primary arch
ok - Build succeeds on all primary arches or has ExcludeArch + bugs filed
ok - BuildRequires correct
-- - Locales handled with %find_lang, not %_datadir/locale/*
-- - %post, %postun call ldconfig if package contains shared .so files
ok - No bundled libs
-- - Relocatability is justified
ok - Package owns all directories it creates
-- - Package requires others for directories it uses but does not own
ok - No duplication in %files unless necessary for license files
ok - File permissions are sane
ok - Package contains permissible code or content
-- - Large docs go in -doc subpackage
ok - %doc files not required at runtime
-- - Static libs go in -static package/virtual Provides
-- - Development files go in -devel package
-- - -devel packages Require base with fully-versioned dependency, %_isa
ok - No .la files
-- - GUI app uses .desktop file, installs it with desktop-file-install
ok - File list does not conflict with other packages' without justification
ok - File names are valid UTF-8
Optional review guidelines:
-- - Query upstream about including license files
no - Translations of description, summary
ok - Builds in mock
ok - Builds on all arches
ok - Functions as described (e.g. no crashes)
-- - Scriptlets are sane
-- - Subpackages require base with fully-versioned dependency if sensible
-- - .pc file subpackage placement is sensible
ok - No file deps outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin
-- - Include man pages if available
Naming guidelines:
ok - Package names use only a-zA-Z0-9-._+ subject to restrictions on -._+
ok - Package names are sane
ok - No naming conflicts
ok - Spec file name matches base package name
ok - Version is sane
ok - Version does not contain ~
ok - Release is sane
ok - %dist tag
ok - Case used only when necessary
-- - Renaming handled correctly
Packaging guidelines:
ok - Useful without external bits
ok - No kmods
ok - Pre-built binaries, libs removed in %prep
ok - Sources contain only redistributable code or content
ok - Spec format is sane
ok - Package obeys FHS, except libexecdir, /run, /usr/target
ok - No files in /bin, /sbin, /lib* on >= F17
-- - Programs run before FS mounting use /run instead of /var/run
-- - Binaries in /bin, /sbin do not depend on files in /usr on < F17
ok - No files under /srv, /opt, /usr/local
ok - Changelog in prescribed format
ok - No Packager, Vendor, Copyright, PreReq tags
ok - Summary does not end in a period
-- - Correct BuildRoot tag on < EL6
-- - Correct %clean section on < EL6
ok - Requires correct, justified where necessary
ok - Summary, description do not use trademarks incorrectly
ok - All relevant documentation is packaged, appropriately marked with %doc
ok - Doc files do not drag in extra dependencies (e.g. due to +x)
-- - Code compilable with gcc is compiled with gcc
-- - Build honors applicable compiler flags or justifies otherwise
-- - PIE used for long-running/root daemons, setuid/filecap programs
-- - Useful -debuginfo package or disabled and justified
-- - Package with .pc files Requires pkgconfig on < EL6
ok - No static executables
-- - Rpath absent or only used for internal libs
-- - Config files marked with %config(noreplace) or justified %config
ok - No config files under /usr
-- - Third party package manager configs acceptable, in %_docdir
-- - .desktop files are sane
ok - Spec uses macros consistently
ok - Spec uses macros instead of hard-coded names where appropriate
ok - Spec uses macros for executables only when configurability is needed
-- - %makeinstall used only when alternatives don't work
-- - Macros in Summary, description are expandable at srpm build time
-- - Spec uses %{SOURCE#} instead of $RPM_SOURCE_DIR and %sourcedir
ok - No software collections (scl)
ok - Build uses only python/perl/shell+coreutils/lua/BuildRequired langs
-- - %global, not %define
-- - Package translating with gettext BuildRequires it
-- - Package translating with Linguist BuildRequires qt-devel
ok - File ops preserve timestamps
-- - Parallel make
ok - No Requires(pre,post) notation
-- - User, group creation handled correctly (See Packaging:UsersAndGroups)
-- - Web apps go in /usr/share/%name, not /var/www
-- - Conflicts are justified
ok - One project per package
ok - No bundled fonts
-- - Patches have appropriate commentary
ok - Available test suites executed in %check
-- - tmpfiles.d used for /run, /run/lock on >= F15
Got it, Garrett. And thanks again (to Matt, too)! New Package SCM Request ======================= Package Name: shflags Short Description: Simple handling of command-line flags in Bourne based Unix scripts Owners: ralph Branches: f17 el6 InitialCC: Git done (by process-git-requests). |