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.
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.
(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.
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.
$ 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
(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.
/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.
> 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.
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)
> > 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.
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).
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
(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.
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.
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
Michael, the package looks good to go. Will you do the formal review or do you want me to do it?
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.
Oh, I can't do a format review either, because it must be done by the sponsor in case of new packagers.
> 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
(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.
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
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)
Oh, it should be enough to move the Requires: python3-pillow to the proper place.
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.
Attached is the review. I probably need now a sponsor?
Created attachment 830693 [details] Package Review
> Package Review Filling in all the '[ ]' items in that "review" would have been interesting. The beginning of the file mentions that. ;-)
Hi, Have you found a sponsor yet? Are you willing to work on this still?
*** This bug has been marked as a duplicate of bug 1195835 ***