Bug 1018492 (vcsh)

*** Bug 972237 has been marked as a duplicate of this bug. ***

Hi,

I'll review this one :)

Thanks,
Warm regards,
Ankur

Created attachment 813933 [details]
build.log from failed build.

The package doesn't build in mock in a rawhide configuration. Build.log attached.

This is weird, on my f19 machine %doc adds _pkgdocdir and its contents to the file listing. This includes files from the %doc macro, and files installed from make.

If I patch the spec:
 %files
 %defattr(-, root, root, -)
 %doc LICENSE CONTRIBUTORS changelog
+%{_docdir}/%{name}-%{version}
 %{_bindir}/%{name}
 %{_mandir}/man*/%{name}*
 %{_datadir}/zsh/

I get the following warnings:
warning: File listed twice: /usr/share/doc/vcsh-1.20130909
warning: File listed twice: /usr/share/doc/vcsh-1.20130909/README.md
warning: File listed twice: /usr/share/doc/vcsh-1.20130909/hooks

Same results here:
http://koji.fedoraproject.org/koji/getfile?taskID=6078574&name=build.log

It's because of the unversioned docdir change: http://fedoraproject.org/wiki/Changes/UnversionedDocdirs

I'll double check when i get home. On my cell at the moment.

I was aware of this change, but I forgot about it. According to the wiki, it starts with f20 (rawhide is currently f21) so I'll handle that in the spec.

Spec URL: https://bitbucket.org/dridi/fedora_packages/downloads/vcsh.spec
SRPM URL: https://bitbucket.org/dridi/fedora_packages/downloads/vcsh-1.20130909-2.fc19.src.rpm

%changelog
* Sat Oct 19 2013 Dridi Boukelmoune <dridi.boukelmoune> - 1.20130909-2
- Switched to _pkgdocdir
- Removed unnecessary `rm -rf %{buildroot}' in clean and install

Review:

[+] OK
[-] NA
[?] Issue

** Mandatory review guidelines: **
 [+] rpmlint output:
[asinha@ankur-laptop  SRPMS]$ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/*.rpm
vcsh.noarch: W: spelling-error %description -l en_US config -> con fig, con-fig, configure
vcsh.noarch: W: spelling-error %description -l en_US configs -> con figs, con-figs, configure
vcsh.src: W: spelling-error %description -l en_US config -> con fig, con-fig, configure
vcsh.src: W: spelling-error %description -l en_US zsh -> sh, ssh, ash
vcsh.src: W: spelling-error %description -l en_US configs -> con figs, con-figs, configure
vcsh.src:52: W: macro-in-%changelog %{buildroot}
2 packages and 0 specfiles checked; 0 errors, 6 warnings.
[asinha@ankur-laptop  SRPMS]$

 [+] License is acceptable
 [+] License field in spec is correct
 [+] License files included in package %docs if included in source package
 [+] License files installed when any subpackage combination is installed
 [+] Spec written in American English
 [+] Spec is legible
 [+] Sources match upstream unless altered to fix permissibility issues
[asinha@ankur-laptop  SRPMS]$ review-md5check.sh ../SPECS/vcsh.spec
Getting https://github.com/RichiH/vcsh/archive/v1.20130909.tar.gz to /tmp/review/v1.20130909.tar.gz
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   124  100   124    0     0     56      0  0:00:02  0:00:02 --:--:--    56
100 23850  100 23850    0     0   6317      0  0:00:03  0:00:03 --:--:-- 30228
6007dba2d65db1a8fe32d22d28094fd5  /tmp/review/v1.20130909.tar.gz
6007dba2d65db1a8fe32d22d28094fd5 /home/asinha/rpmbuild/SOURCES/v1.20130909.tar.gz
removed ‘/tmp/review/v1.20130909.tar.gz’
removed directory: ‘/tmp/review’
[asinha@ankur-laptop  SRPMS]$

 [+] Build succeeds on at least one primary arch
 [+] Build succeeds on all primary arches or has ExcludeArch + bugs filed
 [+] BuildRequires correct, justified where necessary
 [-] Locales handled with %find_lang, not %_datadir/locale/*
 [-] %post, %postun call ldconfig if package contains shared .so files
 [+] No bundled libs
 [-] Relocatability is justified
 [+] Package owns all directories it creates
 [+] Package requires others for directories it uses but does not own
Note that the package owns the /usr/share/zsh directory which is owned by other packages also. However, since this is for optional functionality, this is OK:
https://fedoraproject.org/wiki/Packaging:Guidelines#The_directory_is_owned_by_a_package_which_is_not_required_for_your_package_to_function

[asinha@ankur-laptop  SRPMS]$ sudo repoquery -l zsh | egrep '^/usr/share/zsh$'
/usr/share/zsh

[asinha@ankur-laptop  SRPMS]$ rpmls /var/lib/mock/fedora-rawhide-x86_64/result/vcsh-1.20130909-2.fc21.noarch.rpm
-rwxr-xr-x  /usr/bin/vcsh
drwxr-xr-x  /usr/share/doc/vcsh
-rw-r--r--  /usr/share/doc/vcsh/CONTRIBUTORS
-rw-r--r--  /usr/share/doc/vcsh/LICENSE
-rw-r--r--  /usr/share/doc/vcsh/README.md
-rw-r--r--  /usr/share/doc/vcsh/changelog
-rw-r--r--  /usr/share/doc/vcsh/hooks
-rw-r--r--  /usr/share/man/man1/vcsh.1.gz
drwxr-xr-x  /usr/share/zsh
drwxr-xr-x  /usr/share/zsh/site-functions
-rw-r--r--  /usr/share/zsh/site-functions/_vcsh
[asinha@ankur-laptop  SRPMS]$

 [+] No duplication in %files unless necessary for license files
 [+] File permissions are sane
 [+] Package contains permissible code or content
 [-] Large docs go in -doc subpackage
 [+] %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
 [-] No .la files
 [-] GUI app uses .desktop file, installs it with desktop-file-install
 [+] File list does not conflict with other packages' without justification
 [+] File names are valid UTF-8

** Optional review guidelines: **
 [-] Query upstream about including license files
 [-] Translations of description, summary
 [+] Builds in mock
 [+] Builds on all arches
 [+] 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
 [+] No file deps outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin
 [+] Include man pages if available

Naming guidelines:
 [+] Package names use only a-zA-Z0-9-._+ subject to restrictions on -._+
 [+] Package names are sane
 [+] No naming conflicts
 [+] Spec file name matches base package name
 [+] Version is sane
 [+] Version does not contain ~
 [+] Release is sane
 [+] %dist tag
 [-] Case used only when necessary
 [-] Renaming handled correctly

Packaging guidelines:
 [+] Useful without external bits
 [+] No kmods
 [-] Pre-built binaries, libs removed in %prep
 [+] Sources contain only redistributable code or content
 [+] Spec format is sane
 [+] Package obeys FHS, except libexecdir, /run, /usr/target
 [+] 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
 [+] No files under /srv, /opt, /usr/local
 [+] Changelog in prescribed format
 [+] No Packager, Vendor, Copyright, PreReq tags
 [+] Summary does not end in a period
 [-] Correct BuildRoot tag on < EL6
 [-] Correct %clean section on < EL6
 [+] Requires correct, justified where necessary
 [+] Summary, description do not use trademarks incorrectly
 [+] All relevant documentation is packaged, appropriately marked with %doc
 [+] 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
 [+] No static executables
 [+] Rpath absent or only used for internal libs
 [-] Config files marked with %config(noreplace) or justified %config
 [+] No config files under /usr
 [-] Third party package manager configs acceptable, in %_docdir
 [-] .desktop files are sane
 [+] Spec uses macros consistently
 [+] Spec uses macros instead of hard-coded names where appropriate
 [+] 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
 [+] No software collections (scl)
 [-] Macro files named /etc/rpm/macros.%name
 [-] 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
 [-] File ops preserve timestamps
 [+] Parallel make
 [+] 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
 [+] One project per package
 [+] No bundled fonts
 [-] Patches have appropriate commentary
 [-] Available test suites executed in %check
 [-] tmpfiles.d used for /run, /run/lock on >= F15


Nitpicks:
- Please comment the patch, and send it upstream too.

- Since you're installing to pkgdocdir, and the %doc macro already takes ownership of it, you don't need to specify it again. From http://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25files_prefixes:
"Usually, "%doc" is used to list documentation files within %{_builddir} that were not copied to %{buildroot}. A README and INSTALL file is usually included. They will be placed in an appropriate directory under /usr/share/doc, whose ownership does not need to be declared. "


Not blockers though. You can fix them up before you commit to SCM. 

+++ APPROVED +++
Thanks,
Warm regards,
Ankur

(In reply to Ankur Sinha (FranciscoD) from comment #9)
> Nitpicks:
> - Please comment the patch, and send it upstream too.

I will, I have another ongoing issue (RFE) with uptream, I will send a pull request before commiting to the SCM (and add a comment).

> - Since you're installing to pkgdocdir, and the %doc macro already takes
> ownership of it, you don't need to specify it again. From
> http://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25files_prefixes:
> "Usually, "%doc" is used to list documentation files within %{_builddir}
> that were not copied to %{buildroot}. A README and INSTALL file is usually
> included. They will be placed in an appropriate directory under
> /usr/share/doc, whose ownership does not need to be declared. "

Some files are installed with the %doc macro (LICENSE CONTRIBUTORS changelog) and the others with `make install'. There shouldn't be any dupe in the %files section.

> Not blockers though. You can fix them up before you commit to SCM. 
> 
> +++ APPROVED +++
> Thanks,
> Warm regards,
> Ankur

Thank you for the review,
Dridi

For your information, I have submitted the patch:
Spec URL: https://bitbucket.org/dridi/fedora_packages/downloads/vcsh.spec
SRPM URL: https://bitbucket.org/dridi/fedora_packages/downloads/vcsh-1.20130909-2.fc19.src.rpm

New Package SCM Request
=======================
Package Name: vcsh
Short Description: Version Control System for $HOME
Owners: dridi
Branches: f18 f19 f20
InitialCC:

Git done (by process-git-requests).

vcsh-1.20130909-3.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/vcsh-1.20130909-3.fc20

vcsh-1.20130909-3.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/vcsh-1.20130909-3.fc19

vcsh-1.20130909-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/vcsh-1.20130909-3.fc18

vcsh-1.20130909-3.fc18 has been pushed to the Fedora 18 testing repository.

vcsh-1.20130909-3.fc19 has been pushed to the Fedora 19 stable repository.

vcsh-1.20130909-3.fc18 has been pushed to the Fedora 18 stable repository.

vcsh-1.20130909-3.fc20 has been pushed to the Fedora 20 stable repository.

Could you please made it for epel7 too?