https://bugzilla.redhat.com/show_bug.cgi?id=1491281
--- Comment #2 from Miro Hrončok <mhron...@redhat.com> --- 1) First of all: Always post links that can be downloaded and viewed as plaintext / SRPM. There are automated tools here that expect that. That would be: Spec URL: https://github.com/Traceur759/python-libsass/raw/master/python-libsass.spec SRPM URL: https://github.com/Traceur759/python-libsass/raw/master/python-libsass-0.13.2-1.fc26.src.rpm 2) You don't need to define a sum macro, just fill in the Summary: line and then use %{summary} everywhere else. 3) Is suggest to drop the %{libname} macro and hardcode libsass-python in the URL. It is convenient when one can just copy paste it form the spec. Also, you can use %{url} in Source0: URL: https://github.com/dahlia/libsass-python Source0: %{url}/releases/download/%{version}/%{srcname}-%{version}.tar.gz 6) This bundles libsass C/C++ library. That one is packaged in Fedora: https://apps.fedoraproject.org/packages/libsass You should make every effort to use that package instead of the bundled code in the tarball. This might involve patching. If you absolutely fail (e.g. because the bundled version of libsass is very diffferent from original (probably not, they use a git submodule), than you need to indicate bundling by providing bundled(libsass). See more about bundling at https://fedoraproject.org/wiki/Packaging:Guidelines#Bundling_and_Duplication_of_system_libraries 5) I see you use ExclusiveArch tag. Please, add a comment that explains why is thet necessary. Or maybe it's not necessary at all? Even the libsass C/C++ package builds for all arches in Fedora. 6) Putting each BuildRequires on a separate line makes them more maintainable. (But current way works as well and is OK.) 7) sed "1d" -i %{buildroot}/%{python2_sitearch}/sassc.py I guess this is for shebang? If so: - is it possible to get rid of it once in prep? - can the command be chanaged to only delete the shebang if it's actually there? 8) # Note that there is no %%files section for the unversioned python module if we are building for several python runtimes This is such common situation, that it might not require this comment any more. 9) README.rst is no license and should not be used with %license tag. Upstream has a nice LICENSE file in git, maybe propose (pull request) a change to their MANIFEST.in file to include LICENSE in the tarballs. In the meantime, this package can live easily without the file. Note: Somebody did this already: https://github.com/dahlia/libsass-python/commit/a00117129e0e8a863195c378337fc416ee69b179 10) PKG-INFO is not documentation and does not belong to %doc 11) I see hardcoded 2.7, 3.6 and 36 in the %files section. While this might eb Ok for 2.7, because theat will be the last version of 2, this is not Ok for 3.6. Use the following macros instead: https://fedoraproject.org/wiki/Packaging:Python#Macros %{python3_version_nodots} for 36 %{python3_version} for 3.6 %{python2_version} for 2.7 12) %{python3_sitearch}/__pycache__/ You have this in the files section. It will make your package own the entire directory, which is not desired, python3-libs owns that directory. Either use: %{python3_sitearch}/__pycache__/* to own everything inside, or be more explicit: %{python3_sitearch}/__pycache__/sass.* %{python3_sitearch}/__pycache__/sassc.* ... 13) The bindir section in %files looks like it doesn't belong anywhere, where in fact it belongs to the python3-package. I suggest moving it more top in the files list. (But that is not a strong opinion). Feel free to ping me on #fedora-python for more info. -- 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
