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

Miro Hrončok <[email protected]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[email protected]



--- Comment #2 from Miro Hrončok <[email protected]> ---
From
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2159976-python-scikit-build/fedora-rawhide-x86_64/05216165-python-scikit-build/fedora-review/review.txt

> python3-scikit-build.x86_64: E: no-binary

This indicates this should be a noarch package.
The need for `%global debug_package %{nil}` indicates the same.

> python-scikit-build.spec:1: W: macro-in-comment %files

That is in the comment about debug_package and will go away.

> python-scikit-build.src: E: description-line-too-long cross compilation, and 
> locating dependencies and determining their build requirements.
> python3-scikit-build.x86_64: E: description-line-too-long cross compilation, 
> and locating dependencies and determining their build requirements.

Please, wrap the lines.


=========


Spec comments:

> BuildRequires:  gcc-c++
> BuildRequires:  gcc-gfortran

I suggest also adding gcc. It is transitively pulled but used directly.
I also suggest separating the tests deps visually from the build deps e.g.:

  BuildRequires:  python3-devel

  # For tests:
  BuildRequires:  cmake
  BuildRequires:  gcc
  BuildRequires:  gcc-c++
  BuildRequires:  gcc-gfortran
  BuildRequires:  git-core
  BuildRequires:  ninja-build



> Requires:       cmake

Is ninja also required on runtime or not?




> Patch:          0001-Remove-test-deps-missing-in-Fedora.patch

Consider not numbering the patch file, please. When/if more patches are added
and some are removed, it gets messy. We got rid of the PatchX: numbering
recently, let's not dig ourselves back with patch filenames?




> %pytest -k "not pep518 and not \
>             test_hello_sdist and not \
>             test_hello_sdist_with_base and not \
>             test_sdist_with_symlinks and not \
>             test_manifest_in_sdist" -m "not deprecated"

Consider putting -m "not deprecated" on a separate line after backslash, IMHO
it improves readability.


-- 
You are receiving this mail because:
You are always notified about changes to this product and component
You are on the CC list for the bug.
https://bugzilla.redhat.com/show_bug.cgi?id=2159976
_______________________________________________
package-review mailing list -- [email protected]
To unsubscribe send an email to [email protected]
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/[email protected]
Do not reply to spam, report it: 
https://pagure.io/fedora-infrastructure/new_issue

Reply via email to