https://bugzilla.redhat.com/show_bug.cgi?id=1825456
--- Comment #2 from Richard W.M. Jones <rjo...@redhat.com> ---
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@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
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@lists.fedoraproject.org
