https://bugzilla.redhat.com/show_bug.cgi?id=1268380 Björn "besser82" Esser <besser82@xxxxxxxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |besser82@xxxxxxxxxxxxxxxxx Assignee|williamjmorenor@xxxxxxxxx |besser82@xxxxxxxxxxxxxxxxx Summary|Review Request: |Review Request: |python-sphinx-theme-bootstr |python-sphinx-bootstrap-the |ap - A sphinx theme that |me - A sphinx theme that |integrates the Bootstrap |integrates the Bootstrap |framework |framework --- Comment #32 from Björn "besser82" Esser <besser82@xxxxxxxxxxxxxxxxx> --- Since there was no progress within the last three months, I'll take over the review… *** Your link to the spec-file isn't valid, it must point to the raw spec-file… Quick fix… Spec URL: https://raw.githubusercontent.com/stuartcampbell/rpm-packages/master/sphinx-theme-bootstrap/sphinx-theme-bootstrap.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/sic/packages/fedora-rawhide-x86_64/00443690-python-sphinx-theme-bootstrap/python-sphinx-theme-bootstrap-0.4.5-2.fc26.src.rpm *** Please update to the latest version: 0.4.13 *** Your spec-files is named wrong. It should be named 'python-python-sphinx-bootstrap-theme' according to naming guidelines. See: https://fedoraproject.org/wiki/Packaging:Naming?rd=Packaging:NamingGuidelines#Python_source_package_naming *** Comments on the spec-file: > %global summary A sphinx theme that integrates the Bootstrap framework This isn't valid, since %{summary} is set by rpmbuild during parsing of the spec-file and rotates it's contents for every sub-package. Use sth. like: %{common_sum} instead: %global common_sum A sphinx theme that integrates the Bootstrap framework … Summary: %{common_sum} > # RHEL doesn't have python 3 and does not know about __python2 > %if 0%{?rhel} > %global __python2 %{__python} > %global python2_sitelib %{python_sitelib} > %global with_python3 0 > %else > %global with_python3 1 > %endif Doing so is simply wrong… You are shadowing the settings of the system-macros if present… A cleaner solution would be: %{?!__python2:%global __python2 %{__python}} %{?!python2_sitelib:%global python2_sitelib %{python_sitelib}} %if 0%{?fedora} >= 13 || 0%{?rhel} >= 8 %global with_py3 1 %endif # 0#{?fedora} >= 13 || 0#{?rhel} >= 8 This only evals if there is no system-preset for Python2 macros and works without conditionals on a specific distro-release. Just setting with_py, when it is present, simplyfies a lot of conditonals, especially one-liners can profit from conditional expansion. The Url for Source0 is wrong. See: https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL#Python_Packages_.28pypi.29 > Requires: js-jquery1 Is wrong at this place. It must be placed into every sub-pkg, that needs it. The BuildRequires should be moved into corresponding sub-pkg, too. Repeating the same text in %description bloats the spec-file. You can use: %global common_desc \ This sphinx theme integrates the Booststrap CSS / Javascript framework \ with various layout options, hierarchical menu navigation, and mobile-\ friendly responsive design. It is configurable, extensible and can use \ any number of different Bootswatch CSS themes. And then use it in every %description like: %description %{common_desc} The 'egg-info'-dir must be removed during %prep. The package doesn't own the sub-dir it creates. The installed egg-info-dirs should be packaged using the %{pythonX_version}-macros. There some other small adjustments. I've created a PR on github for this. See: https://github.com/stuartcampbell/rpm-packages/pull/4/commits *** Not approved. Please fix the spec-file and post updated links here; I'll give it another shot then. -- 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