Bug 1018492 - (vcsh) Review Request: vcsh - Version Control System for $HOME
Review Request: vcsh - Version Control System for $HOME
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ankur Sinha (FranciscoD)
Fedora Extras Quality Assurance
:
: 972237 (view as bug list)
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2013-10-12 12:02 EDT by Dridi Boukelmoune
Modified: 2015-03-29 14:03 EDT (History)
5 users (show)

See Also:
Fixed In Version: vcsh-1.20130909-3.fc20
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-11-03 00:33:30 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
sanjay.ankur: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
build.log from failed build. (6.52 KB, text/x-log)
2013-10-19 03:18 EDT, Ankur Sinha (FranciscoD)
no flags Details

  None (edit)
Description Dridi Boukelmoune 2013-10-12 12:02:14 EDT
Spec URL: https://bitbucket.org/dridi/fedora_packages/downloads/vcsh.spec
SRPM URL: https://bitbucket.org/dridi/fedora_packages/downloads/vcsh-1.20130909-1.fc19.src.rpm
Description:
vcsh allows you to have several git repositories, all maintaining their working
trees in $HOME without clobbering each other. That, in turn, means you can have
one repository per config set (zsh, vim, ssh, etc), picking and choosing which
configs you want to use on which machine.

Fedora Account System Username: dridi
Comment 1 Dridi Boukelmoune 2013-10-12 12:05:25 EDT
*** Bug 972237 has been marked as a duplicate of this bug. ***
Comment 2 Ankur Sinha (FranciscoD) 2013-10-16 18:43:38 EDT
Hi,

I'll review this one :)

Thanks,
Warm regards,
Ankur
Comment 3 Ankur Sinha (FranciscoD) 2013-10-19 03:18:59 EDT
Created attachment 813933 [details]
build.log from failed build.

The package doesn't build in mock in a rawhide configuration. Build.log attached.
Comment 4 Dridi Boukelmoune 2013-10-19 03:40:02 EDT
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
Comment 5 Dridi Boukelmoune 2013-10-19 03:51:27 EDT
Same results here:
http://koji.fedoraproject.org/koji/getfile?taskID=6078574&name=build.log
Comment 6 Ankur Sinha (FranciscoD) 2013-10-19 05:17:55 EDT
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.
Comment 7 Dridi Boukelmoune 2013-10-19 07:07:16 EDT
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.
Comment 8 Dridi Boukelmoune 2013-10-19 07:48:24 EDT
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@gmail.com> - 1.20130909-2
- Switched to _pkgdocdir
- Removed unnecessary `rm -rf %{buildroot}' in clean and install
Comment 9 Ankur Sinha (FranciscoD) 2013-10-20 08:14:10 EDT
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
Comment 10 Dridi Boukelmoune 2013-10-20 14:03:51 EDT
(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
Comment 11 Dridi Boukelmoune 2013-10-22 15:52:45 EDT
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
Comment 12 Dridi Boukelmoune 2013-10-22 15:57:32 EDT
New Package SCM Request
=======================
Package Name: vcsh
Short Description: Version Control System for $HOME
Owners: dridi
Branches: f18 f19 f20
InitialCC:
Comment 13 Jon Ciesla 2013-10-23 08:44:53 EDT
Git done (by process-git-requests).
Comment 14 Fedora Update System 2013-10-23 12:33:47 EDT
vcsh-1.20130909-3.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/vcsh-1.20130909-3.fc20
Comment 15 Fedora Update System 2013-10-23 12:35:11 EDT
vcsh-1.20130909-3.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/vcsh-1.20130909-3.fc19
Comment 16 Fedora Update System 2013-10-23 12:36:47 EDT
vcsh-1.20130909-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/vcsh-1.20130909-3.fc18
Comment 17 Fedora Update System 2013-10-23 20:53:18 EDT
vcsh-1.20130909-3.fc18 has been pushed to the Fedora 18 testing repository.
Comment 18 Fedora Update System 2013-11-03 00:33:30 EDT
vcsh-1.20130909-3.fc19 has been pushed to the Fedora 19 stable repository.
Comment 19 Fedora Update System 2013-11-03 00:39:23 EDT
vcsh-1.20130909-3.fc18 has been pushed to the Fedora 18 stable repository.
Comment 20 Fedora Update System 2013-11-10 01:58:27 EST
vcsh-1.20130909-3.fc20 has been pushed to the Fedora 20 stable repository.
Comment 21 Pavel Alexeev 2015-03-29 08:32:46 EDT
Could you please made it for epel7 too?

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