[Bug 1825456] Review Request: libvirt-test-API - Python based regression tests for libvirt API

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1825456



--- Comment #2 from Richard W.M. Jones <rjones@xxxxxxxxxx> ---
Some general comments on the spec file:

(1) There are some typos / formatting issues in the very first # comment
at the top of the file.  It would be better reformatted as:

# Disable the shebangs checks on scripts that currently don't
# define a Python version. The point here is that these scripts
# will be copied to guest VM instances, which may be running
# operating systems that can have either Python 2 or Python 3,
# but it's impossible to know for sure at packaging time.

(2) In the section %if %{with_python3} ... %else ... %endif that
defines BuildRequires and Requires, it is better to factor out the
common BRs/Rs, ie:

BuildRequires: mock

%if %{with_python3}
BuildRequires: python3-devel
BuildRequires: python3-lxml
BuildRequires: python3-pytest
BuildRequires: python3-setuptools
BuildRequires: python3-six
BuildRequires: python3-attrs
BuildRequires: python3-libvirt
BuildRequires: python3-pexpect

Requires: qemu-img
Requires: python3-six
Requires: python3-lxml
Requires: python3-libvirt
%else
BuildRequires: python2-devel
BuildRequires: python2-pytest
BuildRequires: python2-setuptools
BuildRequires: python2-attrs
BuildRequires: python-six
BuildRequires: python2-pexpect

Requires: python-six
Requires: python-lxml
%endif

Requires: libvirt
Requires: qemu-kvm
Requires: virt-install

(3) Is it really the case that qemu-img is only required if using
Python 3, or is that a mistake revealed by the refactoring?

(4) Whitespace in:

%if 0%{?rhel} && 0%{?rhel} < 8
Requires: libvirt-python
%endif

(5) The %files section should also be refactored.  You can see that
it's much clearer afterwards:

%files
%doc README.md
%license LICENSE
%{_bindir}/%{name}
%if %{with_python2}
%{python2_sitelib}/libvirt_test_api*
%{python2_sitelib}/libvirttestapi*
%endif
%if %{with_python3}
%{python3_sitelib}/libvirt_test_api*
%{python3_sitelib}/libvirttestapi*
%endif
%{_datadir}/libvirt-test-api*

I'll do a formal review in a minute.


-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux