Bug 1168333

taking it on

Can't run the full fedora-review tool yet because the needed vagrant RPM is not in the rawhide repo yet.

But one initial comment:

The policy-kit rule file installed by vagrant-libvirt is called 10-vagrant.rules.
Shouldn't it rather be named 10-vagrant-libvirt.rules ?

Michael

I hit a couple of issues:

In order to mock build the package at all, for the time being
I first enable the  [local] in my fedora-rawhide-x86_64 mock config,
because official fedora repos don't have the vagrant package yet.
(Thanks for the hint, Vít.)

I am running "fedora-review -b 1168333 -m fedora-rawhide-x86_64.local".
Now the build fails running %check:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

+ pushd ./usr/share/vagrant/gems/gems/vagrant-libvirt-0.0.23
+ sed -i '/:git/ s|:git.*$|:path => "/usr/share/vagrant"|' Gemfile
+ sed -i '/rspec/ s|\[\".*\"]|["~> 2.0"]|' vagrant-libvirt.gemspec
+ bundle exec rspec2 spec
Your Gemfile lists the gem vagrant-libvirt (>= 0) more than once.
You should probably keep only one of them.
While it's not a problem now, it could cause errors if you change the version of just one of them later.
/usr/share/gems/gems/bundler-1.7.8/lib/bundler/resolver.rb:357:in `resolve': Could not find gem 'rake (= 10.1.0) ruby' in the gems available on this machine. (Bundler::GemNotFound)
        from /usr/share/gems/gems/bundler-1.7.8/lib/bundler/resolver.rb:164:in `start'
        from /usr/share/gems/gems/bundler-1.7.8/lib/bundler/resolver.rb:129:in `resolve'
        from /usr/share/gems/gems/bundler-1.7.8/lib/bundler/definition.rb:193:in `resolve'
        from /usr/share/gems/gems/bundler-1.7.8/lib/bundler/definition.rb:132:in `specs'
        from /usr/share/gems/gems/bundler-1.7.8/lib/bundler/definition.rb:177:in `specs_for'
        from /usr/share/gems/gems/bundler-1.7.8/lib/bundler/definition.rb:166:in `requested_specs'
        from /usr/share/gems/gems/bundler-1.7.8/lib/bundler/environment.rb:18:in `requested_specs'
        from /usr/share/gems/gems/bundler-1.7.8/lib/bundler/runtime.rb:13:in `setup'
        from /usr/share/gems/gems/bundler-1.7.8/lib/bundler.rb:122:in `setup'
        from /usr/share/gems/gems/bundler-1.7.8/lib/bundler/setup.rb:17:in `<top (required)>'
        from /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:54:in `require'
        from /usr/share/rubygems/rubygems/core_ext/kernel_require.rb:54:in `require'
RPM build errors:
error: Bad exit status from /var/tmp/rpm-tmp.1BBUt3 (%check)
    Bad exit status from /var/tmp/rpm-tmp.1BBUt3 (%check)
Child return code was: 1
EXCEPTION: Command failed. See logs for output.
 # bash --login -c /usr/bin/rpmbuild -bb --target x86_64 --nodeps  /builddir/build/SPECS/vagrant-libvirt.spec 
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/mockbuild/trace_decorator.py", line 84, in trace
    result = func(*args, **kw)
  File "/usr/lib/python2.7/site-packages/mockbuild/util.py", line 481, in do
    raise mockbuild.exception.Error("Command failed. See logs for output.\n # %s" % (command,), child.returncode)
Error: Command failed. See logs for output.
 # bash --login -c /usr/bin/rpmbuild -bb --target x86_64 --nodeps  /builddir/build/SPECS/vagrant-libvirt.spec 
LEAVE do --> EXCEPTION RAISED

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

So there is 1 warning:
 - Gemfile lists vagrant-libvirt gem more than once.

And one error:
 - The dependency for rake is wrong.
   Apparently 10.1.0 is hard-wired somewhere, but
   rawhide now sports 10.4.2: rubygem-rake-0:10.4.2-3.fc22.noarch

It exceeds my ruby expertise to fix this quickly.

Michael

ok, I fixed it. following up soon.

Spec URL: https://fedorapeople.org/~obnox/rpms/vagrant-libvirt/0.0.23-4/vagrant-libvirt.spec
SRPM URL: https://fedorapeople.org/~obnox/rpms/vagrant-libvirt/0.0.23-4/vagrant-libvirt-0.0.23-4.fc22.src.rpm

The code can be found on

git://fedorapeople.org/~obnox/fedora-vagrant-libvirt.git

And I did an update to 0.0.24:

Spec URL: https://fedorapeople.org/~obnox/rpms/vagrant-libvirt/0.0.24-1/vagrant-libvirt.spec
SRPM URL: https://fedorapeople.org/~obnox/rpms/vagrant-libvirt/0.0.24-1/vagrant-libvirt-0.0.24-1.fc22.src.rpm

Ok, now this is really strange:
While I can build these srpms with mock without problems, and have also built them on copr (https://copr.fedoraproject.org/coprs/obnox/vagrant-libvirt/), I can't seem to run "fedora-review" on them, neither with the bug number nor specifying the srpm/spec locally. This fails immediately with:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

$ fedora-review -m fedora-rawhide-x86_64 -n vagrant-libvirt   
INFO: Processing local files: vagrant-libvirt
INFO: Getting .spec and .srpm Urls from : Local files in /home/obnox/review/vagrant-libvirt/0.0.24-1/tmp
INFO:   --> SRPM url: file:///home/obnox/review/vagrant-libvirt/0.0.24-1/tmp/vagrant-libvirt-0.0.24-1.fc22.src.rpm
INFO:   --> Spec url: file:///home/obnox/review/vagrant-libvirt/0.0.24-1/tmp/vagrant-libvirt.spec
INFO: Using review directory: /home/obnox/review/vagrant-libvirt/0.0.24-1/tmp/review-vagrant-libvirt
RPM version 4.12.0.1
Copyright (C) 1998-2002 - Red Hat, Inc.
This program may be freely redistributed under the terms of the GNU GPL

Usage: rpm [-afgpcdLlsiv?] [-a|--all] [-f|--file] [-g|--group] [-p|--package] [--pkgid] [--hdrid]
        [--triggeredby] [--whatrequires] [--whatprovides] [--nomanifest] [-c|--configfiles] [-d|--docfiles]
        [-L|--licensefiles] [--dump] [-l|--list] [--queryformat=QUERYFORMAT] [-s|--state] [--nofiledigest]
        [--nofiles] [--nodeps] [--noscript] [--allfiles] [--allmatches] [--badreloc] [-e|--erase=<package>+]
        [--excludedocs] [--excludepath=<path>] [--force] [-F|--freshen=<packagefile>+] [-h|--hash] [--ignorearch]
        [--ignoreos] [--ignoresize] [-i|--install] [--justdb] [--nodeps] [--nofiledigest] [--nocontexts]
        [--noorder] [--noscripts] [--notriggers] [--oldpackage] [--percent] [--prefix=<dir>]
        [--relocate=<old>=<new>] [--replacefiles] [--replacepkgs] [--test] [-U|--upgrade=<packagefile>+]
        [--reinstall=<packagefile>+] [-D|--define='MACRO EXPR'] [--undefine=MACRO] [-E|--eval='EXPR']
        [--macros=<FILE:...>] [--noplugins] [--nodigest] [--nosignature] [--rcfile=<FILE:...>] [-r|--root=ROOT]
        [--dbpath=DIRECTORY] [--querytags] [--showrc] [--quiet] [-v|--verbose] [--version] [-?|--help]
        [--usage] [--scripts] [--setperms] [--setugids] [--conflicts] [--obsoletes] [--provides] [--requires]
        [--recommends] [--suggests] [--supplements] [--enhances] [--info] [--changelog] [--xml] [--triggers]
        [--last] [--dupes] [--filesbypkg] [--fileclass] [--filecolor] [--fscontext] [--fileprovide]
        [--filerequire] [--filecaps]
ERROR: Exception down the road...(logs in /home/obnox/.cache/fedora-review.log)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

and the contents of the logfile:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
01-22 21:37 root         DEBUG    fedora-review 0.5.2 63c24cb 2014-07-14 15:08:50 +0200 started
01-22 21:37 root         DEBUG    Command  line: /usr/bin/fedora-review -m fedora-rawhide-x86_64 -n vagrant-libvirt
01-22 21:37 root         INFO     Processing local files: vagrant-libvirt
01-22 21:37 root         INFO     Getting .spec and .srpm Urls from : Local files in /home/obnox/review/vagrant-libvirt/0.0.24-1/tmp
01-22 21:37 root         DEBUG    Active settings after processing options
01-22 21:37 root         DEBUG        resultdir: None
01-22 21:37 root         DEBUG        verbose: False
01-22 21:37 root         DEBUG        no_report: False
01-22 21:37 root         DEBUG        session_log: /home/obnox/.cache/fedora-review.log
01-22 21:37 root         DEBUG        list_flags: False
01-22 21:37 root         DEBUG        list_checks: False
01-22 21:37 root         DEBUG        single: None
01-22 21:37 root         DEBUG        rpm_spec: False
01-22 21:37 root         DEBUG        plugins: {}
01-22 21:37 root         DEBUG        exclude: None
01-22 21:37 root         DEBUG        configdir: None
01-22 21:37 root         DEBUG        log_level: 20
01-22 21:37 root         DEBUG        init_done: True
01-22 21:37 root         DEBUG        cache: False
01-22 21:37 root         DEBUG        mock_config: fedora-rawhide-x86_64
01-22 21:37 root         DEBUG        version: False
01-22 21:37 root         DEBUG        uniqueext: None
01-22 21:37 root         DEBUG        flags: []
01-22 21:37 root         DEBUG        bz_url: https://bugzilla.redhat.com
01-22 21:37 root         DEBUG        mock_options: --no-cleanup-after --no-clean
01-22 21:37 root         DEBUG        list_plugins: False
01-22 21:37 root         DEBUG        _log_config_done: True
01-22 21:37 root         DEBUG        other_bz: None
01-22 21:37 root         DEBUG        plugins_arg: None
01-22 21:37 root         DEBUG        repo: None
01-22 21:37 root         DEBUG        use_colors: True
01-22 21:37 root         DEBUG        bug: None
01-22 21:37 root         DEBUG        prebuilt: False
01-22 21:37 root         DEBUG        name: vagrant-libvirt
01-22 21:37 root         DEBUG        url: None
01-22 21:37 root         DEBUG        checksum: sha256
01-22 21:37 root         DEBUG        nobuild: False
01-22 21:37 root         DEBUG        _con_handler: <logging.StreamHandler object at 0x7f08171d70d0>
01-22 21:37 root         INFO       --> SRPM url: file:///home/obnox/review/vagrant-libvirt/0.0.24-1/tmp/vagrant-libvirt-0.0.24-1.fc22.src.rpm
01-22 21:37 root         INFO       --> Spec url: file:///home/obnox/review/vagrant-libvirt/0.0.24-1/tmp/vagrant-libvirt.spec
01-22 21:37 root         DEBUG    find_urls completed: 0.003
01-22 21:37 root         INFO     Using review directory: /home/obnox/review/vagrant-libvirt/0.0.24-1/tmp/review-vagrant-libvirt
01-22 21:37 root         DEBUG    Avoiding init of working mock root
01-22 21:37 root         DEBUG    Url download completed: 2.886
01-22 21:37 root         DEBUG    Exception down the road...
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/FedoraReview/review_helper.py", line 232, in run
    self._do_run(outfile)
  File "/usr/lib/python2.7/site-packages/FedoraReview/review_helper.py", line 222, in _do_run
    self._do_report(outfile)
  File "/usr/lib/python2.7/site-packages/FedoraReview/review_helper.py", line 96, in _do_report
    self._run_checks(self.bug.spec_file, self.bug.srpm_file, outfile)
  File "/usr/lib/python2.7/site-packages/FedoraReview/review_helper.py", line 105, in _run_checks
    self.checks = Checks(spec, srpm)
  File "/usr/lib/python2.7/site-packages/FedoraReview/checks.py", line 271, in __init__
    self.spec = SpecFile(spec_file, self.flags)
  File "/usr/lib/python2.7/site-packages/FedoraReview/spec_file.py", line 91, in __init__
    update_macros()
  File "/usr/lib/python2.7/site-packages/FedoraReview/spec_file.py", line 65, in update_macros
    expanded = Mock.get_macro(macro, self, flags)
  File "/usr/lib/python2.7/site-packages/FedoraReview/mock.py", line 346, in get_macro
    self._macros = self._get_default_macros()
  File "/usr/lib/python2.7/site-packages/FedoraReview/mock.py", line 134, in _get_default_macros
    values = self._rpm_eval(tags).split()
  File "/usr/lib/python2.7/site-packages/FedoraReview/mock.py", line 259, in _rpm_eval
    return check_output(cmd).decode('utf-8').strip()
  File "/usr/lib64/python2.7/subprocess.py", line 573, in check_output
    raise CalledProcessError(retcode, cmd, output=output)
CalledProcessError: Command '['mock', '-r', 'fedora-rawhide-x86_64', '--no-cleanup-after', '--no-clean', '--resultdir=/home/obnox/review/vagrant-libvirt/0.0.24-1/tmp/review-vagrant-libvirt/results', '--quiet', '--shell', 'rpm --eval \\"%dist %fedora %epel %buildarch %_libdir %_isa %arch\\"']' returned non-zero exit status 9
01-22 21:37 root         ERROR    Exception down the road...(logs in /home/obnox/.cache/fedora-review.log)
01-22 21:37 root         DEBUG    Report completed:  3.518 seconds
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

When I manually copy the command and run it the via mock it works:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
$ mock -r fedora-rawhide-x86_64 --no-cleanup-after --no-clean --resultdir=/home/obnox/review/vagrant-libvirt/1168333-vagrant-libvirt/results --shell 'rpm --eval "%dist %fedora %epel %buildarch %_libdir %_isa %arch"'
INFO: mock.py version 1.2.4 starting (python version = 2.7.8)...
Start: init plugins
INFO: selinux disabled
Finish: init plugins
Start: run
Start: chroot init
INFO: calling preinit hooks
INFO: enabled root cache
INFO: enabled yum cache
Start: cleaning yum metadata
Finish: cleaning yum metadata
INFO: enabled ccache
Finish: chroot init
Start: shell
.fc22 22 %epel %buildarch /usr/lib64 (x86-64) %arch
Finish: shell
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

but there seems to be a quotation problem inside fedora-review:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
$ mock -r fedora-rawhide-x86_64 --no-cleanup-after --no-clean --resultdir=/home/obnox/review/vagrant-libvirt/1168333-vagrant-libvirt/results --shell 'rpm --eval \"%dist %fedora %epel %buildarch %_libdir %_isa %arch\"'
INFO: mock.py version 1.2.4 starting (python version = 2.7.8)...
Start: init plugins
INFO: selinux disabled
Finish: init plugins
Start: run
Start: chroot init
INFO: calling preinit hooks
INFO: enabled root cache
INFO: enabled yum cache
Start: cleaning yum metadata
Finish: cleaning yum metadata
INFO: enabled ccache
Finish: chroot init
Start: shell
".fc22
RPM version 4.12.0.1
Copyright (C) 1998-2002 - Red Hat, Inc.
This program may be freely redistributed under the terms of the GNU GPL

Usage: rpm [-afgpcdLlsiv?] [-a|--all] [-f|--file] [-g|--group] [-p|--package] [--pkgid] [--hdrid]
        [--triggeredby] [--whatrequires] [--whatprovides] [--nomanifest] [-c|--configfiles] [-d|--docfiles]
        [-L|--licensefiles] [--dump] [-l|--list] [--queryformat=QUERYFORMAT] [-s|--state] [--nofiledigest]
        [--nofiles] [--nodeps] [--noscript] [--allfiles] [--allmatches] [--badreloc] [-e|--erase=<package>+]
        [--excludedocs] [--excludepath=<path>] [--force] [-F|--freshen=<packagefile>+] [-h|--hash] [--ignorearch]
        [--ignoreos] [--ignoresize] [-i|--install] [--justdb] [--nodeps] [--nofiledigest] [--nocontexts]
        [--noorder] [--noscripts] [--notriggers] [--oldpackage] [--percent] [--prefix=<dir>]
        [--relocate=<old>=<new>] [--replacefiles] [--replacepkgs] [--test] [-U|--upgrade=<packagefile>+]
        [--reinstall=<packagefile>+] [-D|--define='MACRO EXPR'] [--undefine=MACRO] [-E|--eval='EXPR']
        [--macros=<FILE:...>] [--noplugins] [--nodigest] [--nosignature] [--rcfile=<FILE:...>] [-r|--root=ROOT]
        [--dbpath=DIRECTORY] [--querytags] [--showrc] [--quiet] [-v|--verbose] [--version] [-?|--help]
        [--usage] [--scripts] [--setperms] [--setugids] [--conflicts] [--obsoletes] [--provides] [--requires]
        [--recommends] [--suggests] [--supplements] [--enhances] [--info] [--changelog] [--xml] [--triggers]
        [--last] [--dupes] [--filesbypkg] [--fileclass] [--filecolor] [--fscontext] [--fileprovide]
        [--filerequire] [--filecaps]
Finish: shell
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

But I am currently at a loss what triggers it.
What can I have done to the srpm? I did not change a lot.

Ok, this is a bug in fedora-review. Not sure why it was triggered by my packages, but it is fixed in this update:

 https://admin.fedoraproject.org/updates/fedora-review-0.5.2-2.fc21


Thanks to Vít for the hint!!!

Ok, now I completed the review.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Unknown or generated". 48 files have unknown license. Detailed output of
     licensecheck in /home/obnox/review/vagrant-libvirt/1168333-vagrant-
     libvirt/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
     --> fixed
[-]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/share/vagrant/gems/doc,
     /usr/share/vagrant/gems/specifications, /usr/share/vagrant/gems/gems,
     /usr/share/vagrant/gems, /usr/share/vagrant
     --> TODO: fix vagrant RPM
[-]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/vagrant/gems/doc,
     /usr/share/vagrant/gems/specifications, /usr/share/vagrant,
     /usr/share/vagrant/gems, /usr/share/vagrant/gems/gems
     --> TODO: fix vagrant RPM
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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 is included in %doc.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: Avoid bundling fonts in non-fonts packages.
     Note: Package contains font files
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in vagrant-
     libvirt-doc
     --> not applicable for noarch package.
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: vagrant-libvirt-0.0.24-1.fc22.noarch.rpm
          vagrant-libvirt-doc-0.0.24-1.fc22.noarch.rpm
          vagrant-libvirt-0.0.24-1.fc22.src.rpm
vagrant-libvirt.noarch: E: explicit-lib-dependency libvirt-daemon-kvm
--> INVALID - this is not a library.
vagrant-libvirt.noarch: W: summary-not-capitalized C libvirt provider for vagrant
--> fixed
vagrant-libvirt.noarch: W: no-documentation
--> fixed (README.md moved)
vagrant-libvirt-doc.noarch: E: non-executable-script /usr/share/vagrant/gems/gems/vagrant-libvirt-0.0.24/Rakefile 0644L /usr/bin/env
--> fixed
vagrant-libvirt.src: W: summary-not-capitalized C libvirt provider for vagrant
--> fixed
3 packages and 0 specfiles checked; 2 errors, 3 warnings.




Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:


Requires
--------
vagrant-libvirt (rpmlib, GLIBC filtered):
    /bin/bash
    /bin/sh
    libvirt
    libvirt-daemon-kvm
    polkit
    ruby(release)
    ruby(rubygems)
    rubygem(fog)
    rubygem(multi_json)
    rubygem(nokogiri)
    rubygem(ruby-libvirt)
    shadow-utils
    vagrant

vagrant-libvirt-doc (rpmlib, GLIBC filtered):
    vagrant-libvirt



Provides
--------
vagrant-libvirt:
    vagrant-libvirt

vagrant-libvirt-doc:
    vagrant-libvirt-doc



Source checksums
----------------
https://rubygems.org/gems/vagrant-libvirt-0.0.24.gem :
  CHECKSUM(SHA256) this package     : c1086997295119774c289d05839fee4b58634b6a7544ee6d23d94b044436829a
  CHECKSUM(SHA256) upstream package : c1086997295119774c289d05839fee4b58634b6a7544ee6d23d94b044436829a


Generated by fedora-review 0.5.2 (63c24cb) last change: 2014-07-14
Command line :/usr/bin/fedora-review -v -b 1168333 -m fedora-rawhide-x86_64
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api
Disabled plugins: Java, C/C++, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


I have myself addressed all issues above.
The issues are addressed in the following SRPM:

Spec URL: https://fedorapeople.org/~obnox/rpms/vagrant-libvirt/0.0.24-2/vagrant-libvirt.spec
SRPM URL: https://fedorapeople.org/~obnox/rpms/vagrant-libvirt/0.0.24-2/vagrant-libvirt-0.0.24-2.fc22.src.rpm

With these versions I am happy.

Change history for this version on top of Vít's is to be found here:

https://fedorapeople.org/cgit/obnox/public_git/fedora-vagrant-libvirt.git/


Michael

Vít,

Setting the review flag and reassigning to you
for final review of my changes and further processing.

If you don't like something I changed, please bounce back to me.
I'd be glad if you would incorporate the changes from my git
in the scm if possible. (I was not aware how I can create a repo
in the exact format for fedpkg.)

Thanks - Michael

FYI: this RPM seems to be missing the work maxamillion and I did ~7 months ago:
https://github.com/maxamillion/vagrant-libvirt-rpm/
In particular it's missing:
.bashrc_vagrant
vagrant-libvirt.pkla

Please include these two changes. They're quite vital for making vagrant on fedora suck less :)

Michael, thanks for the review! A few comments from me ...

(In reply to Michael Adam from comment #10)
> [-]: Package requires other packages for directories it uses.
>      Note: No known owner of /usr/share/vagrant/gems/doc,
>      /usr/share/vagrant/gems/specifications, /usr/share/vagrant/gems/gems,
>      /usr/share/vagrant/gems, /usr/share/vagrant
>      --> TODO: fix vagrant RPM
> [-]: Package must own all directories that it creates.
>      Note: Directories without known owners: /usr/share/vagrant/gems/doc,
>      /usr/share/vagrant/gems/specifications, /usr/share/vagrant,
>      /usr/share/vagrant/gems, /usr/share/vagrant/gems/gems
>      --> TODO: fix vagrant RPM


This should be fixed in vagrant-1.6.5-18.fc22:

http://koji.fedoraproject.org/koji/taskinfo?taskID=8721662

> Rpmlint
> -------
> Checking: vagrant-libvirt-0.0.24-1.fc22.noarch.rpm
>           vagrant-libvirt-doc-0.0.24-1.fc22.noarch.rpm
>           vagrant-libvirt-0.0.24-1.fc22.src.rpm
> vagrant-libvirt.noarch: E: explicit-lib-dependency libvirt-daemon-kvm
> --> INVALID - this is not a library.

This specifies driver which is going to be in use with vagrant-libvirt. According to documentation, only kvm and qemu are supported ATM.

https://github.com/pradels/vagrant-libvirt#provider-options

This could be better to replace by Recommends probably, but that is not handled by YUM.

> vagrant-libvirt.noarch: W: summary-not-capitalized C libvirt provider for
> vagrant
> --> fixed

libvirt is official name of the project if I am not mistaken. In this sense it is false positive.

> I have myself addressed all issues above.
> The issues are addressed in the following SRPM:
> With these versions I am happy.

Thanks for the patches. I merged them into my version:

Spec URL: http://people.redhat.com/vondruch/vagrant-libvirt.spec
SRPM URL: http://people.redhat.com/vondruch/vagrant-libvirt-0.0.24-2.fc22.src.rpm

Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=8721967


(In reply to James (purpleidea) from comment #13)
> .bashrc_vagrant

This is missing on purpose. Although it definitely provides useful utilities, I am not convinced about they comprehensiveness and I am not convinced they belong in this package. Vagrant itself would be better place, but I would argue about inclusion as well.

Moreover, they have no upstream, no tests, no documentation, no specification etc => sooner or later, they will break without notice.

If you are serious about them, please consider to convince some upstream to include them in the package.

> vagrant-libvirt.pkla

This was replaced by 10-vagrant-libvirt.rules (although I am still not convinced we should ship this file, since there are security concerns such as bug 957300).

> > vagrant-libvirt.pkla
> 
> This was replaced by 10-vagrant-libvirt.rules (although I am still not
> convinced we should ship this file, since there are security concerns such
> as bug 957300).

Actually, lets see what is polkit's maintainer point of view.

(In reply to Vít Ondruch from comment #15)
> > > vagrant-libvirt.pkla
> > 
> > This was replaced by 10-vagrant-libvirt.rules (although I am still not
> > convinced we should ship this file, since there are security concerns such
> > as bug 957300).
> 
> Actually, lets see what is polkit's maintainer point of view.

In principle, I think using groups (or even better, groups-of-groups, for “role” groups containing groups of users) to manage permissions is a good model; getting the exact role design is rather tricky (e.g. for VMs it could likely make sense to have a role that allows starting/stopping/restarting VMs and a separate one for more general administration) but that is something the various upstreams are probably best equipped to design, at least for roles that don’t span more roles.


AFAICS 10-vagrant-libvirt.rules differs from the above in a significant way, though: it is in a package _different from_ libvirt that overrides rules that affect _all_ of libvirt.  I strongly think that this should not be happening.  

* If an author of a polkit-using service designs some default rules and group membership rules, that is fine (upstream documentation currently prohibits this, but that should be changed, bug #956005).

* If a site administrator wants to override defaults in their own polkit rules RPM, that is obviously fine.

* Changing the system policy by installing an apparently unrelated RPM is _not_ good.


It might make sense for vagrant-libvirt to install rules that apply only to vagrant VMs (I don’t know, I haven’t looked into vagraint-libvirt details at all at the moment).  But if you are effectively trying to implement the libvirt-general RFE from bug #957300, that really needs to happen in the general libvirt RPM, not as a side-effect of installing (not even choosing to use?) a particular backend.

(In reply to Miloslav Trmač from comment #16)
> (or even better, groups-of-groups, for
> “role” groups containing groups of users)

(And no, Linux can not do groups-of-groups outside of specific setups like sssd.)

> It might make sense for vagrant-libvirt to install rules that apply only to
> vagrant VMs (I don’t know, I haven’t looked into vagraint-libvirt details at
> all at the moment).

This might have been drowned in the rest of the text.  What exactly is the use case and your desired policy design?  I have only seen what (I think) the policy file does (and I think it shouldn’t do that); perhaps the right path really is to do bug #957300, perhaps libvirt-vagrant should be installing some other rule file; perhaps I just don’t understand how libvirt with this package installed works.

(In reply to Vít Ondruch from comment #14)
> Michael, thanks for the review! A few comments from me ...
> 
> ...
> 
> Thanks for the patches. I merged them into my version:
> 
> Spec URL: http://people.redhat.com/vondruch/vagrant-libvirt.spec
> SRPM URL:
> http://people.redhat.com/vondruch/vagrant-libvirt-0.0.24-2.fc22.src.rpm

Thanks! This looks good to me, except for the open question
regarding the policykit rules.

> Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=8721967

Er... Newbie question: How to you do koji builds without having the SCM yet?

> (In reply to James (purpleidea) from comment #13)
> > .bashrc_vagrant
> 
> This is missing on purpose. Although it definitely provides useful
> utilities, I am not convinced about they comprehensiveness and I am not
> convinced they belong in this package. Vagrant itself would be better place,
> but I would argue about inclusion as well.

Right, I had discussed that with James before but was not fast enough
to update this BZ. So this could either go into the main vagrant
RPM or into an RPM of its own.
 
> Moreover, they have no upstream, no tests, no documentation, no
> specification etc => sooner or later, they will break without notice.
> 
> If you are serious about them, please consider to convince some upstream to
> include them in the package.

Not sure what would be a good place to upstream this.
Keeping that separate, i.e. maintained by James in a separate
repo, might make some sense.
If this is just some convenience shell foo, the fedora packaging
may even be the place to maintain this bashrc snippet.

Not sure what to do here.
But again, this shouldn't be blocking vagrant-libvirt. 


> > vagrant-libvirt.pkla
> 
> This was replaced by 10-vagrant-libvirt.rules 

Right, this should have the same effect.
and the .pkla seems to be the more legacy variant.

> (although I am still not
> convinced we should ship this file, since there are security concerns such
> as bug 957300).

Apart from clarifying this, the current RPM is reviewed+ by me.

After discussion with mitr, I am going to drop the polkit rule for now. The policy is too big hammer ATM. We need something more tailored. Unfortunately, I am not libvirt nor polkit expert. This should be probably solved in similar manner to bug 957300.


Thanks for the review!


New Package SCM Request
=======================
Package Name: vagrant-libvirt
Short Description: libvirt provider for Vagrant
Upstream URL: https://github.com/pradels/vagrant-libvirt
Owners: vondruch jstribny obnox
Branches: 
InitialCC:

(In reply to Vít Ondruch from comment #19)
> After discussion with mitr, I am going to drop the polkit rule for now. The
> policy is too big hammer ATM. We need something more tailored.

Agreed, though it is very convenient. ;-)

> Unfortunately, I am not libvirt nor polkit expert. This should be probably
> solved in similar manner to bug 957300.
> 
> 
> Thanks for the review!
> 
> 
> New Package SCM Request
> =======================
> Package Name: vagrant-libvirt
> Short Description: libvirt provider for Vagrant
> Upstream URL: https://github.com/pradels/vagrant-libvirt
> Owners: vondruch jstribny obnox
> Branches: 
> InitialCC:

Can we request a f21 branch right away?
I will then take care of the f21 builds.

Michael

(In reply to Michael Adam from comment #20)
> Can we request a f21 branch right away?
> I will then take care of the f21 builds.


Sure! Sorry, forgot about that :/


New Package SCM Request
=======================
Package Name: vagrant-libvirt
Short Description: libvirt provider for Vagrant
Upstream URL: https://github.com/pradels/vagrant-libvirt
Owners: vondruch jstribny obnox
Branches: f21
InitialCC:

(In reply to Vít Ondruch from comment #19)
> After discussion with mitr, I am going to drop the polkit rule for now. The
> policy is too big hammer ATM. We need something more tailored.
> Unfortunately, I am not libvirt nor polkit expert. This should be probably
> solved in similar manner to bug 957300.
> 

I'm not sure entirely how this relates to the other bug, but not including the polkit file makes this package quite useless... Every user will have to add it themselves. At the bare minimum, please include that polkit file in /usr/share/vagrant/ or something, so the user can at least run: sudo cp -a ... /etc/ ...

I'd love to hear alternate solutions :)

Thanks,
James

In parallel one solution could be to use the libvirt qemu:///session instead of system, but it's not clear how vagrant would be then able to manipulate networks... If someone wants to help hack on that, I'm down to write some code too, but I'm not sure about what networking changes would need to be done.

See: https://github.com/pradels/vagrant-libvirt/issues/272

Git done (by process-git-requests).

(In reply to James (purpleidea) from comment #22)
James, would you mind to open separate BZ ticket for this? I am going to simple remove the file for the moment, but we can add it later to allow users to cp it into appropriate place. Also, I was suggested to explore either pkexec probably console helper (e.g. something along the lines mock is done). Also, libvirt has a lot of polkit rules already define, but I was not able to trigger any of them. Not sure it that is due to old version of rubygem-ruby-libvirt

(In reply to James (purpleidea) from comment #22)
> (In reply to Vít Ondruch from comment #19)
> > After discussion with mitr, I am going to drop the polkit rule for now. The
> > policy is too big hammer ATM. We need something more tailored.
> > Unfortunately, I am not libvirt nor polkit expert. This should be probably
> > solved in similar manner to bug 957300.
> > 
> 
> I'm not sure entirely how this relates to the other bug

Including the file we are talking about, pretty much verbatim (except perhaps using a different group name), in one of the main libvirt packages, would be a resolution to bug 957300.  (This does not automatically mean that this is precisely the right thing to do, but if this is to be done anywhere it should be in the main libvirt srpm).

(In reply to Vít Ondruch from comment #25)
> (In reply to James (purpleidea) from comment #22)
> James, would you mind to open separate BZ ticket for this? I am going to
> simple remove the file for the moment, but we can add it later to allow
> users to cp it into appropriate place.

lol, no way. it will take me less time to write the patch :) send me a link to the packaging git repo, and I'll include a patch if you'd like one :)

(In reply to James (purpleidea) from comment #22)
> (In reply to Vít Ondruch from comment #19)
> > After discussion with mitr, I am going to drop the polkit rule for now. The
> > policy is too big hammer ATM. We need something more tailored.
> > Unfortunately, I am not libvirt nor polkit expert. This should be probably
> > solved in similar manner to bug 957300.
> > 
> 
> I'm not sure entirely how this relates to the other bug, but not including
> the polkit file makes this package quite useless... 
> Every user will have to add it themselves.

Well, I think we are in search for a better solution.

The main concern raised with the vagrant-specific solution was as far
as my understanding goes that while it is introduced by vagrant
and for the vagrant group, the effect is for all of libvirt.

The bug 957300 to my understanding propose to introduce a group
libvirt that has the dersired polkit rule installed.

That would mean in turn that in order to use vagrant-libirt
without all the nasty password prompts, you would have to be
a member of the libvirt group.

I think this would be more than ok.

Michael

(In reply to James (purpleidea) from comment #27)
> (In reply to Vít Ondruch from comment #25)
> > (In reply to James (purpleidea) from comment #22)
> > James, would you mind to open separate BZ ticket for this? I am going to
> > simple remove the file for the moment, but we can add it later to allow
> > users to cp it into appropriate place.
> 
> lol, no way. it will take me less time to write the patch :) send me a link
> to the packaging git repo, and I'll include a patch if you'd like one :)

I don't think that we should rush something in that is
not a thought though to the end.

We want to have a proper solution.
And our discussion goes towards that... :)

Cheers - Michael

(In reply to Michael Adam from comment #29)
> (In reply to James (purpleidea) from comment #27)
> > (In reply to Vít Ondruch from comment #25)
> > > (In reply to James (purpleidea) from comment #22)
> > > James, would you mind to open separate BZ ticket for this? I am going to
> > > simple remove the file for the moment, but we can add it later to allow
> > > users to cp it into appropriate place.
> > 
> > lol, no way. it will take me less time to write the patch :) send me a link
> > to the packaging git repo, and I'll include a patch if you'd like one :)
> 
> I don't think that we should rush something in that is
> not a thought though to the end.
> 
> We want to have a proper solution.
> And our discussion goes towards that... :)
> 
> Cheers - Michael

I agree... I thought the BZ request was if we should include the .polkit file in /usr/share/doc/ or something or not, to convenience the user if they want to use root and copy it in. No discussion needed there :)

For the "best" proper solution (which might not be possible) see my comment#23.

vagrant-libvirt-0.0.24-3.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/vagrant-libvirt-0.0.24-3.fc21

If you are interested in the polkit issue, please continue discussion in bug 1187019.

(In reply to James (purpleidea) from comment #30)
> (In reply to Michael Adam from comment #29)
> > (In reply to James (purpleidea) from comment #27)
> > > (In reply to Vít Ondruch from comment #25)
> > > > (In reply to James (purpleidea) from comment #22)
> > > > James, would you mind to open separate BZ ticket for this? I am going to
> > > > simple remove the file for the moment, but we can add it later to allow
> > > > users to cp it into appropriate place.
> > > 
> > > lol, no way. it will take me less time to write the patch :) send me a link
> > > to the packaging git repo, and I'll include a patch if you'd like one :)
> > 
> > I don't think that we should rush something in that is
> > not a thought though to the end.
> > 
> > We want to have a proper solution.
> > And our discussion goes towards that... :)
> 
> I agree... I thought the BZ request was if we should include the .polkit
> file in /usr/share/doc/ or something or not, to convenience the user if they
> want to use root and copy it in. No discussion needed there :)

OK, I probably got your earlier comment wrong.
I just did updates to include the polkit rules file
under the vagrant-gem's doc dir.

Also pushed update to f21.

(In reply to Vít Ondruch from comment #32)
> If you are interested in the polkit issue, please continue discussion in bug
> 1187019.

Awesome! And thanks for work on the packaging issue!!

Package Change Request
======================
Package Name: vagrant-libvirt
New Branches: epel7
Owners: obnox vondruch jstribny humaton

Git done (by process-git-requests).

vagrant-libvirt-0.0.24-3.fc21 has been pushed to the Fedora 21 stable repository.