Bug 835275 - Review Request: shflags - Simple handling of command-line flags in Bourne based Unix scripts
Review Request: shflags - Simple handling of command-line flags in Bourne bas...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Garrett Holmstrom
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 835657
  Show dependency treegraph
 
Reported: 2012-06-25 16:27 EDT by Ralph Bean
Modified: 2012-07-07 14:46 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-07-07 14:46:41 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
gholms: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Ralph Bean 2012-06-25 16:27:37 EDT
Spec URL: http://threebean.org/rpm/shflags.spec
SRPM URL: http://threebean.org/rpm/shflags-1.0.3-1.fc17.src.rpm
Description:
Shell Flags (shFlags) is a library written to greatly simplify the handling of
command-line flags in Bourne based Unix shell scripts (bash, dash, ksh, sh, zsh)
on many Unix OSes (Linux, Solaris, Mac OS X, etc.).

Most shell scripts use getopt for flags processing, but the different versions
of getopt on various OSes make writing portable shell scripts difficult. shFlags
instead provides an API that doesn't change across shell and OS versions so the
script writer can be confident that the script will work.

shFlags is a port of the google-gflags C++/Python library.

Fedora Account System Username: ralph



rpmlint
-------

--- ~/rpmbuild » rpmlint {SPECS,SRPMS}/shflags*
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
1 packages and 1 specfiles checked; 0 errors, 5 warnings.
--- ~/rpmbuild » rpmlint /var/lib/mock/fedora-17-x86_64/result/*.rpm                                   
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.



I'll file an issue upstream to update the FSF license shortly.
Comment 1 Matt Spaulding 2012-07-05 18:55:26 EDT
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
Comment 2 Garrett Holmstrom 2012-07-06 14:40:37 EDT
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
Comment 3 Ralph Bean 2012-07-06 16:40:16 EDT
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.
Comment 4 Garrett Holmstrom 2012-07-06 19:34:07 EDT
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
Comment 5 Ralph Bean 2012-07-07 09:30:41 EDT
Got it, Garrett.  And thanks again (to Matt, too)!
Comment 6 Ralph Bean 2012-07-07 09:31:31 EDT
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:
Comment 7 Gwyn Ciesla 2012-07-07 09:34:36 EDT
Git done (by process-git-requests).
Comment 8 Ralph Bean 2012-07-07 14:46:24 EDT
Updates -> https://admin.fedoraproject.org/updates/shflags

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