Bug 1634957

What distros are you targeting with the spec?

%{!?__python2: %global __python2 %__python}
%{!?python2_sitelib: %global python2_sitelib %(%{__python2} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib())")}

Is not needed on EPEL (and obviously not on Fedora).

Also, python2-test_server is a no go for Fedora:

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

> Software using python2 MUST NOT be newly packaged without FESCo exception.

The %with_pythonX_tests macros do not conditionalize test requirements. Maybe you want to do that.

BuildRequires:  %{_bindir}/sphinx-build
%if 0%{?with_python3}
BuildRequires:  python%{python3_pkgversion}-sphinx-theme-alabaster
%endif

This cannot work together.

%{_bindir}/sphinx-build is in python2-sphinx and the theme is python3.

(In reply to Miro Hrončok from comment #1)
> What distros are you targeting with the spec?

Rawhide and EPEL7. Maybe F29/28.

> %{!?__python2: %global __python2 %__python}
> %{!?python2_sitelib: %global python2_sitelib %(%{__python2} -c "from
> distutils.sysconfig import get_python_lib; print(get_python_lib())")}
> 
> Is not needed on EPEL (and obviously not on Fedora).

OK

> Also, python2-test_server is a no go for Fedora:

Oh, thought python2 is disabled for the Fedora build. Will check.

> The %with_pythonX_tests macros do not conditionalize test requirements.
> Maybe you want to do that.

OK

(In reply to Miro Hrončok from comment #2)
> BuildRequires:  %{_bindir}/sphinx-build
> %if 0%{?with_python3}
> BuildRequires:  python%{python3_pkgversion}-sphinx-theme-alabaster
> %endif
> 
> This cannot work together.

Well, it works but needs obviously some improvment. Sphinx is built with python3 in Fedora, no? EPEL7 uses python2 but shows an issue with the theme.

> %{_bindir}/sphinx-build is in python2-sphinx and the theme is python3.

Orly?

(In reply to Miro Hrončok from comment #1)
…
> The %with_pythonX_tests macros do not conditionalize test requirements.
> Maybe you want to do that.

What do you mean? Maybe you wonder python36-bottle is not available in epel7, is it needed for tests only? And I don't want to conditionalize tests in case of python2 for legacy reasons, all or nothing there with py2.


For the sphinx part, I've no idea how to keep the code as readable as possible without confusing python2 and python3 runtimes.

pythonXY-bottle is a runtime dependency but not available yet in EPEL7:
./test_server/server.py:        from bottle import request, LocalResponse
./test_server/server.py:import bottle

That means python3_other pkg can't actually work in EPEL7.

Erstellt: /builddir/build/SRPMS/python-test_server-0.0.31-2.el7.src.rpm
Last metadata expiration check: 0:00:04 ago on Mi 03 Okt 2018 16:46:21 CEST.
No matching package to install: 'python34-webtest'
Not all dependencies satisfied
Error: Some packages could not be found.
ERROR: Exception(SRPMS/python-test_server-0.0.31-2.fc29.src.rpm) Config(epel-7-x86_64) 0 minutes 40 seconds

[builder@builder29]~/fedora-scm/python-webtest% fedpkg request-branch epel7
Could not execute request_branch: This package is already an EL package and is built on all supported arches, therefore, it cannot be in EPEL. If this is a mistake or you have an exception, please contact the Release Engineering team.

Bad situation in epel7 that makes me think about any usefulness.

Okay, let's restart this review without thinking about epel7 in this moment.
Although PyPI provides sources as well, don't use because docs folder is missing in that tarball, better get it released from GitHub archive.

I uploaded new SPEC and SRPM files, the links keep the same.

Task info: https://koji.fedoraproject.org/koji/taskinfo?taskID=30031016

Oh my, typos all around. Another try to re-upload.
Task info: https://koji.fedoraproject.org/koji/taskinfo?taskID=30031053

BUILDSTDERR: /var/tmp/rpm-tmp.oEjnV7: line 40: sphinx-build: command not found

This is what i get in mock. It works in Koji, but not in mock (no idea, but i have seen this before).

Use:

sphinx-build-3 -d docs/doctrees docs docs/html

Also, since you skip epel, the %{python3_pkgversion} macro is redundant, use 3 instead please.

As you wish:

SPEC: https://raphgro.fedorapeople.org/review/py/python-test_server/python-test_server.spec
SRPM: https://raphgro.fedorapeople.org/review/py/python-test_server/python-test_server-0.0.31-2.fc29.src.rpm

%changelog
* Sat Oct 06 2018 Raphael Groner <projects.rg> - 0.0.31-2
- replace python3 version macro with concrete value, currently no plan for epel7
- use binary suffix for sphinx due to weird error with mock

Task info: https://koji.fedoraproject.org/koji/taskinfo?taskID=30072153

Thanks for the review!


> BUILDSTDERR: /var/tmp/rpm-tmp.oEjnV7: line 40: sphinx-build: command not found
> 
> This is what i get in mock. It works in Koji, but not in mock (no idea, but i
> have seen this before).

That's indeed weird. It works in my local mock for F29 and f-r for rawhide, both times without the binary suffix. Anyways, I've added the requested suffix.

Please remove /usr/share/doc/python-test_server-doc/html/.buildinfo

Assuming you do, this package is APPROVED.


Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated



===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Expat License", "Unknown or generated".
[x]: License file installed when any subpackage combination is installed.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

Python:
[x]: Python eggs must not download any dependencies during the build
     process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Packages MUST NOT have dependencies (either build-time or runtime) on
     packages named with the unversioned python- prefix unless no properly
     versioned package exists. Dependencies on Python packages instead MUST
     use names beginning with python2- or python3- as appropriate.
[x]: Binary eggs must be removed in %prep

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[-]: Fully versioned dependency in subpackages if applicable.
[?]: Package functions as described.
[?]: Latest version is packaged.
[-]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[?]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: python3-test_server-0.0.31-2.fc30.noarch.rpm
          python-test_server-doc-0.0.31-2.fc30.noarch.rpm
          python-test_server-0.0.31-2.fc30.src.rpm
python-test_server-doc.noarch: W: hidden-file-or-dir /usr/share/doc/python-test_server-doc/html/.buildinfo
3 packages and 0 specfiles checked; 0 errors, 1 warnings.




Rpmlint (installed packages)
----------------------------
sh: /usr/bin/python: No such file or directory
python-test_server-doc.noarch: W: invalid-url URL: https://github.com/lorien/test_server <urlopen error [Errno -2] Name or service not known>
python-test_server-doc.noarch: W: hidden-file-or-dir /usr/share/doc/python-test_server-doc/html/.buildinfo
python3-test_server.noarch: W: invalid-url URL: https://github.com/lorien/test_server <urlopen error [Errno -2] Name or service not known>
2 packages and 0 specfiles checked; 0 errors, 3 warnings.



Requires
--------
python-test_server-doc (rpmlib, GLIBC filtered):

python3-test_server (rpmlib, GLIBC filtered):
    python(abi)
    python3.7dist(bottle)
    python3.7dist(six)
    python3.7dist(webtest)



Provides
--------
python-test_server-doc:
    python-test_server-doc

python3-test_server:
    python3-test_server
    python3.7dist(test-server)
    python3dist(test-server)



Source checksums
----------------
https://github.com/lorien/test_server/archive/v0.0.31.tar.gz#/test_server-0.0.31.tar.gz :
  CHECKSUM(SHA256) this package     : 09092febb07aa770bab223e416826a5842db96f30546ded9b0628ed99b6da481
  CHECKSUM(SHA256) upstream package : 09092febb07aa770bab223e416826a5842db96f30546ded9b0628ed99b6da481


Generated by fedora-review 0.6.1 (c495bba) last change: 2018-09-11
Command line :./try-fedora-review -b 1634957 -m fedora-rawhide-x86_64
Buildroot used: fedora-rawhide-x86_64
Active plugins: Python, Generic, Shell-api
Disabled plugins: Java, C/C++, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, BATCH, DISTTAG, EPEL5, EPEL7, EPEL6

https://pagure.io/releng/fedora-scm-requests/issue/8566

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/python-test_server

As it turns out, enabled pysocks tests use the API of test_server version 0.0.30 but upstream changed the API with 0.0.31 and replaced tornado with bottle as the backend instructed with the now gone parameter named engine.

ERROR: setUpClass (test.test_pysocks.PySocksTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/PySocks-1.6.8/test/test_pysocks.py", line 83, in setUpClass
    engine='subprocess')
TypeError: __init__() got an unexpected keyword argument 'engine'

Because imported already version 0.0.31 should I introduce Epoch to have version 0.0.30?

(In reply to Raphael Groner from comment #14)

https://github.com/Anorov/PySocks/commit/c8d06769f281e9eac16ae8e85260e45233fe4107#diff-6e58ae2c8bca00a60f7041a82425b9caR25

https://github.com/lorien/test_server/commit/5465db0fbb25665f785a474ab18380c534cc311d#diff-c5ce133e5ada690e691b433ea45f0519L277

> Because imported already version 0.0.31 should I introduce Epoch to have version 0.0.30?

No idea. Better to fix the code.

(In reply to Miro Hrončok from comment #16)
> > Because imported already version 0.0.31 should I introduce Epoch to have version 0.0.30?
> 
> No idea. Better to fix the code.

Backport the old constructor with a downstream patch and let it print a deprecation warning about the yet unsupported parameter named engine? That would support at least pysocks in the current state without any change needed to be done in the sources of pysocks.

Right. Let me know if you need code review.

Well, there seems to be no official recommendation how to mark deprecated methods in python. Do you know something?

https://bugs.python.org/issue19569 - not fixed.
https://pypi.org/project/deprecated/ - not packaged.

import warnings
warnings.warn("deprecated", DeprecationWarning)

https://github.com/Anorov/PySocks/issues/117

Could you take a quick look with your review eyes into my patch?

https://src.fedoraproject.org/rpms/python-test_server/c/33d225d7de8b7bc6d11d5e21ebd2c68d5642f568?branch=master

(In reply to Raphael Groner from comment #22)
> Could you take a quick look with your review eyes into my patch?
> 
> https://src.fedoraproject.org/rpms/python-test_server/c/
> 33d225d7de8b7bc6d11d5e21ebd2c68d5642f568?branch=master

and https://src.fedoraproject.org/rpms/python-test_server/c/03c4799aff7d5a6a600b1805fc5bca6ea8cb1bf0?branch=master

Instead of:

text = "value of parameter engine is ignored with version 0.0.31"
text += " and param might get removed later"

I'd do:

text = ("value of parameter engine is ignored with version 0.0.31"
        " and the param will be removed later")


But otherwise it looks good.

There's another issue. Enabled tests for python-requests run into "Connection refused" errors. Investigating.

The package was built. If there are more problems, please open new bugs for this package.