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
