[Bug 2135713] Review Request: python-tpm2-pytss - TPM 2.0 TSS Bindings for Python

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

 



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



--- Comment #5 from Jakub Jelen <jjelen@xxxxxxxxxx> ---
(In reply to Miro Hrončok from comment #2)
> > License:        BSD
> 
> New packages are required to sue the SPDX license identifiers. See
> https://docs.fedoraproject.org/en-US/legal/allowed-licenses/

Thanks! Its some time since I was creating a new package so I probably missed
the announcement.

> > This is my first python package so any guidance would be welcomed.
> 
> Any chance you created it with pyp2rpm? it has much-outdated badness that
> seems to be very much like pyp2rpm. Any chance you could start at
> https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/
> #_example_spec_file instead?

I started from the copr build that was probably created by that tool some time
ago:
https://bugzilla.redhat.com/show_bug.cgi?id=2064490#c4

> Anyway, here they are:
> 
> > Source0:        %{pypi_source}
> > Patch0:		python-tpm2-pytss-1.2.0-openssl.patch
> 
> In Fedora, there is no reason to number the patches and sources.
> The spec also mixes tabs and spaces here.
> The %{pypi_source} macro without the name argument is deprecated.

Thanks, fixed.

> > %{?python_provide:%python_provide python3-%{pypi_name}}
> 
> This macro is deprecated and SHOULD not be used.

Thanks, removed.

> > Requires:       python3-asn1crypto
> > Requires:       python3-cffi
> > Requires:       python3-cryptography
> > Requires:       python3-pkgconfig
> 
> Python runtime dependencies are autogenerated and MUST not be repeated
> manually. 
> The package already requires:
> 
> python3.11dist(asn1crypto)
> python3.11dist(cffi) >= 1
> python3.11dist(cryptography) >= 3
> python3.11dist(packaging)
> python3.11dist(pyyaml)

Thanks, removed.

> > Requires:       %{name}%{?_isa} = %{version}-%{release}
> 
> This is a dependency on a package that is not built, but it is met by the
> very same package, as it provides python-tpm2-pytss = 1.2.0-1.fc38. In other
> words, this is entirely redundant and a bit obfuscated slefdependency.

Removed as well.

> > # Remove bundled egg-info
> > rm -rf %{pypi_name}.egg-info
> 
> This is not needed.

Removed as well.

> > %pyproject_wheel
> 
> This macro is not supported without %pyproject_buildrequires.

Added.

> > # tests are very dependent on the python/openssl versions and fail at various places
> 
> All of them?

Yeah, that was just me being too lazy to exclude them properly after seeing
different results on Fedora 35 and rawhide as they take 15 minutes to execute.
It looks good now on Fedora 36, but rawhide still fails. I will try to
investigate it again.

> ------
> 
> Personal opinions:
> 
> I recommend using %pyproject_save_files a lot -- it allows you to use
> %{pyproject_files} in %files, and %pyproject_check_import.

Thanks. This looks like it makes the stuff simpler.

> I consider the %pypi_name, %_name and %name combo really hard to read. have
> you considered not defining such macros and literally spelling the name in
> the spec where needed? Same for %pypi_version which is defined and only used
> once to define %version.

Thanks. I removed the %pypi_version. I still find the names useful as they need
to be written on several places and it is super-easy to swap them.

> > BuildRequires:  python3-codecov
> > BuildRequires:  python3-coverage
> > BuildRequires:  python3-pytest-cov
> 
> Such dependencies are highly discouraged, see https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_linters

Thanks. Removed that too.

> Many fo the other BuildRequires seem like they are only required for tests, but tests do not run in %check, hence they might not be needed.

I will try to enable the tests.

Anyway, updated the spec file and SRPM in here:

Spec URL: https://jjelen.fedorapeople.org/python-tpm2-pytss.spec
SRPM URL:
https://jjelen.fedorapeople.org/python-tpm2-pytss-1.2.0-1.fc38.src.rpm


-- 
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=2135713
_______________________________________________
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
Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue




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

  Powered by Linux