[Bug 1491281] Review Request: python-libsass - python bindings for libsass

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

 



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



--- Comment #2 from Miro Hrončok <mhroncok@xxxxxxxxxx> ---
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@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx




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

  Powered by Linux