Bug 994474 - Review Request: python-qrencode - Python wrapper for the qrencode library
Review Request: python-qrencode - Python wrapper for the qrencode library
Status: CLOSED DUPLICATE of bug 1195835
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-07 07:07 EDT by Marcel Haerry
Modified: 2015-07-21 08:29 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-02-24 19:50:31 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Package Review (8.79 KB, text/plain)
2013-11-29 09:58 EST, Marcel Haerry
no flags Details

  None (edit)
Description Marcel Haerry 2013-08-07 07:07:35 EDT
Spec URL: http://git.scrit.ch/srpm/python-qrencode/plain/SPECS/python-qrencode.spec
SRPM URL: http://scrit.ch/assets/python-qrencode-1.01-1.fc19.src.rpm
Description: A simple wrapper for the C qrencode library.
Fedora Account System Username: maha

Koji Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5789205

This is my first package and I'm seeking for a sponsor.
Comment 1 Christopher Meng 2013-08-07 07:20:38 EDT
1. Remove

%defattr(-, root, root)

2. Filter out the private library.

https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering

3. BuildRequires:  python-devel

should be

BuildRequires:  python2-devel

4. If possible, merge the %check section into setup.py.
Comment 2 Marcel Haerry 2013-08-07 07:41:05 EDT
(In reply to Christopher Meng from comment #1)
> 1. Remove
> 
> %defattr(-, root, root)

done. Will push new spec file once I solved the other issues.

> 2. Filter out the private library.
> 
> https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering

Sorry, but I don't yet understand which private library you mean, as I'm not sure which of the Requires/Provides should be seen as private.

> 3. BuildRequires:  python-devel
> 
> should be
> 
> BuildRequires:  python2-devel

changed.

> 4. If possible, merge the %check section into setup.py.

Will ask the maintainer to integrate once I will notify him, that the library now got included into Fedora.
Comment 3 Marcel Haerry 2013-08-07 09:19:35 EDT
I pushed the changes to the spec file and uploaded the new srpm.

However, it's not clear to me which private library you mean, so I couldn't work on excluding it.
Comment 4 Michael Schwendt 2013-08-07 11:56:06 EDT
    $ rpmls -p python-qrencode-1.01-1.fc20.x86_64.rpm 
(!) -rwxr-xr-x  /usr/lib64/python2.7/site-packages/qr_encode.so
    drwxr-xr-x  /usr/lib64/python2.7/site-packages/qrencode
    -rw-r--r--  /usr/lib64/python2.7/site-packages/qrencode-1.01-py2.7.egg-info
    -rw-r--r--  /usr/lib64/python2.7/site-packages/qrencode/__init__.py
    -rw-r--r--  /usr/lib64/python2.7/site-packages/qrencode/__init__.pyc
    -rw-r--r--  /usr/lib64/python2.7/site-packages/qrencode/__init__.pyo
Comment 5 Marcel Haerry 2013-08-07 19:03:10 EDT
(In reply to Michael Schwendt from comment #4)
>     $ rpmls -p python-qrencode-1.01-1.fc20.x86_64.rpm 
> (!) -rwxr-xr-x  /usr/lib64/python2.7/site-packages/qr_encode.so

Sorry, I'm not really sure I understand correctly what you mean with that. Can you elaborate a little bit more?

As I said this is my first package and I don't fully understand what is meant with private library. Thanks.

As far as I understood qr_encode.so is an internal library that should not be advertised as being provided by the package. However, it does not do that in my opinion:

$ rpm -q --provides -p RPMS/python-qrencode-1.01-1.fc20.x86_64.rpm 
python-qrencode = 1.01-1.fc20
python-qrencode(x86-64) = 1.01-1.fc20
$ 

And it's definitely not requiring it:


$ rpm -q --requires -p RPMS/python-qrencode-1.01-1.fc20.x86_64.rpmlibc.so.6()(64bit)
libc.so.6(GLIBC_2.2.5)(64bit)
libc.so.6(GLIBC_2.4)(64bit)
libpthread.so.0()(64bit)
libpython2.7.so.1.0()(64bit)
libqrencode.so.3()(64bit)
python(abi) = 2.7
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PartialHardlinkSets) <= 4.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rtld(GNU_HASH)
rpmlib(PayloadIsXz) <= 5.2-1
$

But I think it definitely should be part of the package.
Comment 6 Zbigniew Jędrzejewski-Szmek 2013-08-07 21:29:53 EDT
/usr/lib64/python2.7/site-packages/qr_encode.so is a python extension module. It basically an upstream issue, but now it is a top-level module, called 'qr_encode'. It should instead by part of the python package, called 'qrencode._qr_encode' (or something like that), and live in /usr/lib64/python2.7/site-packages/qrencode/qr_encode.so.
Comment 7 Marcel Haerry 2013-08-08 03:37:11 EDT
> It should instead by part of the python package, called
> 'qrencode._qr_encode' (or something like that), and live in
> /usr/lib64/python2.7/site-packages/qrencode/qr_encode.so.

Ok, I made a patch that changes this behaviour:

http://git.scrit.ch/srpm/python-qrencode/plain/SOURCES/fix_location.patch

I also updated the spec file and the src rpm.

I will work with the maintainer once the package got accepted to integrate these changes directly.
Comment 8 Michael Schwendt 2013-08-08 06:26:49 EDT
That's not what "private library" means. It's not only about the path within the Python modules dir.

"Private" refers to a shared lib that is specific/local to your package (and software), with no public API/ABI for external programs to use, and regardless of where it's stored.

Private non-versioned library names with no "lib" prefix are no real problem, since they bear almost no risk of causing an RPM dependency conflict. They mostly "pollute" the RPM/repository metadata, since they are useless because no other package should depend on them.

  qr_encode.so()(64bit)

It's very unlikely that any other package will ever depend on a "qr_encode.so()(64bit)" library name, and it's similary unlikely that any other shared library will use exactly that library name and no version either.

However, even for a lib name like that, one could construct a packaging scenario, where another program also builds and uses a shared library "something.so", storing it in a sub-package, resulting in a library name dependency between base package and sub-package. If any other package provided the same "something.so" thing, that could cause the dependency resolver to assume that either package provides the same required thing.

It's worse for lib names that bear a higher risk of conflicting with system libs in run-time linker's search path. Private plug-in libs outside run-time linker's search path, for example, using lib names "libfoo.so", perhaps even a versioned "libfoo.so.0" name.
 
Even if the private lib were a compatible copy of a system lib (bundled and not dropped yet), it could not be used to satisfy the dependency at run-time, if it's not stored in run-time linker's search path. That's not a theoretical problem anymore. Cases like that have been encountered in the package collection before.


> it's not clear to me which private library you mean

I took that literally and didn't understand it as a question what a private library is.

The link posted by Christopher explains a bit:

  https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering


> As far as I understood qr_encode.so is an internal library
> that should not be advertised as being provided by the package.
> However, it does not do that in my opinion:

That's what it means. ;-)

The package _does_ contain automatic Provides/Requires for the shared lib, possibly depending on how/where you've built the package.

The .fc20 dist tag in your test-build indicates you've built it for Fedora 20 development (Rawhide), where things may have changed. I'm not up-to-date on whether rpmbuild for F20 has been enhanced to ignore libs in private paths.

For a Fedora 19 build, you could observe it:

$ rpmlint -i python-qrencode-1.01-1.fc19.x86_64.rpm python-qrencode-debuginfo-1.01-1.fc19.x86_64.rpm
python-qrencode.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/qr_encode.so qr_encode.so()(64bit)
A shared object soname provides is provided by a file in a path from which
other packages should not directly load shared objects from.  Such shared
objects should thus not be depended on and they should not result in provides
in the containing package.  Get rid of the provides if appropriate, for
example by filtering it out during build.  Note that in some cases this may
require disabling rpmbuild's internal dependency generator.

python-qrencode.x86_64: W: no-documentation
2 packages and 0 specfiles checked; 0 errors, 2 warnings.


> And it's definitely not requiring it:

That's true also for F19 and older, but that is because being a Python module lib, there is nothing inside your package that is linked with that lib. ;)  
That makes the automatic Provides even more useless, but it's not a big issue.

$ rpm -qp --provides python-qrencode-1.01-1.fc19.x86_64.rpm |grep -v ^py
qr_encode.so()(64bit)
Comment 9 Marcel Haerry 2013-08-08 08:38:51 EDT
> > As far as I understood qr_encode.so is an internal library
> > that should not be advertised as being provided by the package.
> > However, it does not do that in my opinion:
> 
> That's what it means. ;-)
> 
> The package _does_ contain automatic Provides/Requires for the shared lib,
> possibly depending on how/where you've built the package.
> 
> The .fc20 dist tag in your test-build indicates you've built it for Fedora
> 20 development (Rawhide), where things may have changed. I'm not up-to-date
> on whether rpmbuild for F20 has been enhanced to ignore libs in private
> paths.
> 
> For a Fedora 19 build, you could observe it:
> 
> $ rpmlint -i python-qrencode-1.01-1.fc19.x86_64.rpm
> python-qrencode-debuginfo-1.01-1.fc19.x86_64.rpm
> python-qrencode.x86_64: W: private-shared-object-provides

Right, thank you very much for your lengthy explanation.

Indeed, I only looked at builds on F20.

So I integrated filters for Fedora <= 19 updated the spec file and the srpm:

http://git.scrit.ch/srpm/python-qrencode/plain/SPECS/python-qrencode.spec
http://scrit.ch/assets/python-qrencode-1.01-1.fc19.src.rpm

And there are also two koji builds, which should be fine:

F19: http://koji.fedoraproject.org/koji/taskinfo?taskID=5794022
F20: http://koji.fedoraproject.org/koji/taskinfo?taskID=5794091

$ rpmlint -i python-qrencode-1.01-1.fc19.x86_64.rpm python-qrencode-1.01-1.fc20.x86_64.rpm
python-qrencode.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

python-qrencode.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

2 packages and 0 specfiles checked; 0 errors, 2 warnings.
Comment 10 Michael Schwendt 2013-08-16 17:15:52 EDT
Spec URL: http://git.scrit.ch/srpm/python-qrencode/plain/SPECS/python-qrencode.spec
SRPM URL: http://scrit.ch/assets/python-qrencode-1.01-1.fc19.src.rpm

Let's see also what "fedora-review -b 994474" will find. It evaluates those two "Spec URL:" and "SRPM URL:" lines.

[...]

> %attr(0755, -, -) %{python_sitearch}/qrencode/qr_encode.so

Not in the guidelines. Just a hint/recommendation:

It's okay to use %attr for settings _ordinary_ permissions like that, but better not get used to it. Prefer "chmod" in the %install section, or get it fixed directly in the upstream tarball (e.g. using "install -m0755 …"). In packages with many more files, if you needed to "fix" permissions, overusing %attr would reduce readability a lot. 0755 and 0644 are too common. Restrict usage of %attr to setting really special/unusual permissions (e.g. setuid, setgid, g-rx) and owner/group changes, so special attributes set with %attr really stick out (especially when using syntax highlighting).
Comment 11 Michael Schwendt 2013-08-16 18:35:51 EDT
And indeed, the %attr usage is enough distraction to miss a packaging mistake, which I would spot easily under normal circumstances:

  %attr(0755, -, -) %{python_sitearch}/qrencode/qr_encode.so
  %{python_sitearch}/qrencode*

Make that

  %{python_sitearch}/qrencode/qr_encode.so
  %{python_sitearch}/qrencode*

and it becomes more obvious that it specifies some files multiple times
( http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles ), because

  %{python_sitearch}/qrencode*

also includes the file

  %{python_sitearch}/qrencode/qr_encode.so

(!)

The fedora-review tool finds it, of course:

> Issues:
> =======
> - Package does not contain duplicates in %files.
>   Note: warning: File listed twice: /usr/lib64/python2.7/site-
>   packages/qrencode/qr_encode.so
>   See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles

$ rpmls -p python-qrencode-1.01-1.fc20.x86_64.rpm 
drwxr-xr-x  /usr/lib64/python2.7/site-packages/qrencode
-rw-r--r--  /usr/lib64/python2.7/site-packages/qrencode-1.01-py2.7.egg-info
-rw-r--r--  /usr/lib64/python2.7/site-packages/qrencode/__init__.py
-rw-r--r--  /usr/lib64/python2.7/site-packages/qrencode/__init__.pyc
-rw-r--r--  /usr/lib64/python2.7/site-packages/qrencode/__init__.pyo
-rwxr-xr-x  /usr/lib64/python2.7/site-packages/qrencode/qr_encode.so

Several different solutions for the %files list would be possible.
When fixing the .so file permissions in %install, e.g. this single line:

  %{python_sitearch}/qrencode*

Or with explicitly listed .egg-info file:

  %{python_sitearch}/qrencode*.egg-info
  %{python_sitearch}/qrencode/

Or with explicitly listed dir and files entries:

  %{python_sitearch}/qrencode*.egg-info
  %dir %{python_sitearch}/qrencode
  %{python_sitearch}/qrencode/*.py*
  %{python_sitearch}/qrencode/*.so
Comment 12 Marcel Haerry 2013-08-17 08:43:35 EDT
(In reply to Michael Schwendt from comment #11)
> And indeed, the %attr usage is enough distraction to miss a packaging
> mistake, which I would spot easily under normal circumstances:
> 
>   %attr(0755, -, -) %{python_sitearch}/qrencode/qr_encode.so
>   %{python_sitearch}/qrencode*
> 
> Make that
> 
>   %{python_sitearch}/qrencode/qr_encode.so
>   %{python_sitearch}/qrencode*
> 
> and it becomes more obvious that it specifies some files multiple times
> ( http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles ),
> because
> 
>   %{python_sitearch}/qrencode*
> 
> also includes the file
> 
>   %{python_sitearch}/qrencode/qr_encode.so
> 
> (!)


right. Thank you for your comments.

I changed the spec file according your first recommendation and pushed the new SPEC file as well as the new srpm and 2 new test builds:

http://koji.fedoraproject.org/koji/taskinfo?taskID=5824366
http://koji.fedoraproject.org/koji/taskinfo?taskID=5824375

Also the review output looks good to me.
Comment 13 Zbigniew Jędrzejewski-Szmek 2013-08-17 20:01:13 EDT
With the "private library" issue, there are two aspects:
1. conforming to Fedora packaging guidelines, which is now done,
2. abiding by Python style guidelines. Moving the library to qrencode/qr_encode.so is already a big improvement. But since you're fixing it, it would be nice to fix it all the way, and rename the library as "private" by Python standards, i.e. starting with an underscore. The file name would be the same as the module name, except for the .so extension.
Comment 14 Marcel Haerry 2013-08-20 08:13:13 EDT
Ok I moved the library now to qrencode/_qrencode.so.

SPEC, Patch and SRPM are updated.

Testbuilds:

F20: http://koji.fedoraproject.org/koji/taskinfo?taskID=5832578
F19: http://koji.fedoraproject.org/koji/taskinfo?taskID=5832600
Comment 15 Zbigniew Jędrzejewski-Szmek 2013-08-20 11:01:28 EDT
Michael, the package looks good to go. Will you do the formal review or do you want me to do it?
Comment 16 Zbigniew Jędrzejewski-Szmek 2013-08-22 17:25:06 EDT
I'll do the review, but I can't sponsor you.

_privatelibs still mentions qr_encode.so, but the name is _qr_encode.so now.
So the bogus Provides is back.

pyqrencode sources don't specify the license at all: neither in setup.py, nor in a separate files, nor in file headers. Upstream should add it explicitly at least in the first two places. Actually, where does the license come from?

"-n %{pkgname}-%{version}" is not needed, it is the default.

The %check section doesn't do anything: there are no .py files there except __init__.py, which is filtered out. 'import qrencode' might work though.

It doesn't actually seem to work:
>>> import qrencode
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python2.7/site-packages/qrencode/__init__.py", line 1, in <module>
    from qrencode._qrencode import encode as _encode
ImportError: dynamic module does not define init function (init_qrencode)
→ the init function must be also renamed in the patch.

Also, it would be nice to have Python3 version too. Unfortunately the .c module doesn't support Python3, so let's leave that for later.
Comment 17 Zbigniew Jędrzejewski-Szmek 2013-08-23 15:09:41 EDT
Oh, I can't do a format review either, because it must be done by the sponsor in case of new packagers.
Comment 18 Michael Schwendt 2013-08-31 16:35:40 EDT
> Oh, I can't do a format review either, 

Anyone can do reviews. Approval is a different matter.


> Actually, where does the license come from?

PKG-INFO and setup.py


> "-n %{pkgname}-%{version}" is not needed, it is the default.

Not true. The default is  -n %{name}-%{version}  and %pkgname != %name for this package.

Btw, if it were  -n %{name}-%{version}  nothing forbids keeping that in the spec file. It can be convenient when packaging snapshots temporarily, as it makes it easier to prepend/append something to %version.


> The %check section doesn't do anything:

True.   Hmmm, ... upstream git is five years old. I wouldn't spend much time on thinking about a %check section. If not test-suite is available, adding one is beyond the scope of the review request. If at all, I'd suggest %checking the files in the %buildroot by adding to Python module search path and then running a test script. Running directly inside builddir may give different results.


> ImportError: dynamic module does not define init function (init_qrencode)
> → the init function must be also renamed in the patch.

Well, without doing it upstream, this (moving, renaming and changing symbols) is going into a wrong direction. Filtering the Provides would have been enough.

  >>> import qr_encode
  dlopen("/usr/lib64/python2.7/site-packages/qr_encode.so", 2);
  import qr_encode # dynamically loaded from /usr/lib64/python2.7/site-packages/qr_encode.so
Comment 19 Zbigniew Jędrzejewski-Szmek 2013-09-06 09:59:59 EDT
(In reply to Michael Schwendt from comment #18)
> > Actually, where does the license come from?
> 
> PKG-INFO and setup.py
OK, I missed that because github upstream does not contain this information.
But the package source is taken from pypi, so the license information is OK.


> > "-n %{pkgname}-%{version}" is not needed, it is the default.
> 
> Not true. The default is  -n %{name}-%{version}  and %pkgname != %name for
> this package.
True.

> > The %check section doesn't do anything:
> 
> True.   Hmmm, ... upstream git is five years old. I wouldn't spend much time
> on thinking about a %check section. If not test-suite is available, adding
> one is beyond the scope of the review request. If at all, I'd suggest
> %checking the files in the %buildroot by adding to Python module search path
> and then running a test script. Running directly inside builddir may give
> different results.
Yes, a trivial import and encode will certainly be more than enough.

I'd propose this test for basic functionality:

%check
pushd build/*lib*
python -c "import qrencode; print(qrencode.encode('''Schrödinger's cat''')[2].size)"
popd

> > ImportError: dynamic module does not define init function (init_qrencode)
> > → the init function must be also renamed in the patch.
> 
> Well, without doing it upstream, this (moving, renaming and changing
> symbols) is going into a wrong direction. Filtering the Provides would have
> been enough.
> 
>   >>> import qr_encode
>   dlopen("/usr/lib64/python2.7/site-packages/qr_encode.so", 2);
>   import qr_encode # dynamically loaded from
> /usr/lib64/python2.7/site-packages/qr_encode.so
Patches have been sent upstream: https://github.com/Arachnid/pyqrencode/pull/3.

Actually the issue of where the .so installed should be considered as more important: in opposition to the Provides issue, it is actually user visible. In ipython, a user can
say qr<tab>, and will now see two possible completions. So it's nice to install it cleanly since the beginning of the package in fedora.

We now have python 3 compatiblity patch, and could also package the package for python3.
Comment 20 Marcel Haerry 2013-11-17 09:20:27 EST
Sorry, it took me a while to come back to this work.

But I integrated all the patches. Added python3 support and have done a test-build against F21.


http://koji.fedoraproject.org/koji/taskinfo?taskID=6190524
http://git.scrit.ch/srpm/python-qrencode/plain/SPECS/python-qrencode.spec
http://scrit.ch/assets/python-qrencode-1.01-1.fc19.src.rpm
Comment 21 Zbigniew Jędrzejewski-Szmek 2013-11-17 18:05:07 EST
Requires
--------
python3-qrencode (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libpthread.so.0()(64bit)
    libpython3.3m.so.1.0()(64bit)
    libqrencode.so.3()(64bit)
    python(abi)
    rtld(GNU_HASH)

python-qrencode (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libpthread.so.0()(64bit)
    libpython2.7.so.1.0()(64bit)
    libqrencode.so.3()(64bit)
    python(abi)
    python-imaging
    python3-pillow                         <---- ?
    rtld(GNU_HASH)
Comment 22 Zbigniew Jędrzejewski-Szmek 2013-11-17 18:08:58 EST
Oh, it should be enough to move the Requires: python3-pillow to the proper place.
Comment 23 Marcel Haerry 2013-11-18 03:05:39 EST
Doh, good catch. I misunderstood the multi-python guidelines in the wiki. Moved it down to the python3 section and it looks better now:

http://koji.fedoraproject.org/koji/taskinfo?taskID=6192319

Also updated the spec file and the SRPM.
Comment 24 Marcel Haerry 2013-11-29 09:57:36 EST
Attached is the review. I probably need now a sponsor?
Comment 25 Marcel Haerry 2013-11-29 09:58:24 EST
Created attachment 830693 [details]
Package Review
Comment 26 Michael Schwendt 2013-12-01 21:00:24 EST
> Package Review

Filling in all the '[ ]' items in that "review" would have been interesting. The beginning of the file mentions that. ;-)
Comment 27 Christopher Meng 2014-07-27 06:19:51 EDT
Hi,

Have you found a sponsor yet? Are you willing to work on this still?
Comment 28 Christopher Meng 2015-02-24 19:50:31 EST

*** This bug has been marked as a duplicate of bug 1195835 ***

Note You need to log in before you can comment on or make changes to this bug.