Bug 1979275 - Review Request: python-lsp-jsonrpc - Python implementation of JSON RPC 2.0 protocol
Summary: Review Request: python-lsp-jsonrpc - Python implementation of JSON RPC 2.0 pr...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1979315
TreeView+ depends on / blocked
 
Reported: 2021-07-05 12:49 UTC by Mukundan Ragavan
Modified: 2021-07-07 16:36 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2021-07-07 16:36:05 UTC
Type: ---
Embargoed:
zbyszek: fedora-review+


Attachments (Terms of Use)

Description Mukundan Ragavan 2021-07-05 12:49:36 UTC
Spec URL: https://nonamedotc.fedorapeople.org/pkgreview/python-packages/python-lsp-jsonrpc/python-lsp-jsonrpc.spec
SRPM URL: https://nonamedotc.fedorapeople.org/pkgreview/python-packages/python-lsp-jsonrpc/python-lsp-jsonrpc-1.0.0-1.fc34.src.rpm

Description:
A python server implementation of JSON RPC 2.0 protocol. This library
has been pulled out of the python LSP server project (a community maintained
fork of python-language-server).

Fedora Account System Username: nonamedotc

Comment 1 Mukundan Ragavan 2021-07-05 12:50:10 UTC
koji scratch build -

https://koji.fedoraproject.org/koji/taskinfo?taskID=71344275

Comment 2 Mukundan Ragavan 2021-07-05 16:00:59 UTC
Since pylsp requires pycodestyle, PR updating pyflakes, flake8 and pycodestyle have been submitted. 

Builds can be seen here - https://copr.fedorainfracloud.org/coprs/nonamedotc/spyder5dev/builds/

Comment 3 Zbigniew Jędrzejewski-Szmek 2021-07-06 09:08:49 UTC
I rebuilt pyflakes, python-pycodestyle, and python-flake8 in koji.
The think they must be built in this particular order, and that they depend on each other,
because the builds would fail until the previous package was present in the buildroot.
This dependency is not declared in the packages, but it probably should be added.
I.e.:
  python-pycodestyle.spec:Requires: python3-pyflakes >= ...
  python-flake8.spec:Requires: python3-pycodestyle >= ...

But I don't know enough about those packages…

--

The build fails (with the updated packages):

AssertionError: assert b'Content-Length: 10\r\nContent-Type: application/vscode-jsonrpc; charset=utf8\r\n\r\n1546300861' in [b'', b'Content-Length: 10\r\nContent-Type: application/vscode-jsonrpc; charset=utf8\r\n\r\n1546304461', b'Content-Length: 10\r\nContent-Type: application/vscode-jsonrpc; charset=utf8\r\n\r\n1546322461']

No idea if that's significant or not, but please fix ;]

Comment 4 Mukundan Ragavan 2021-07-07 00:09:06 UTC
Hi there,


Yes, your order is correct. pyflakes -> pycodestyle -> flake8.



I do not see any build failures for python-lsp-jsonrpc. Here is a build after pycodestyle, etc. were updated - https://koji.fedoraproject.org/koji/taskinfo?taskID=71417694

Comment 5 Miro Hrončok 2021-07-07 07:44:31 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #3)
> I rebuilt pyflakes, python-pycodestyle, and python-flake8 in koji.
> The think they must be built in this particular order, and that they depend
> on each other,
> because the builds would fail until the previous package was present in the
> buildroot.
> This dependency is not declared in the packages, but it probably should be
> added.
> I.e.:
>   python-pycodestyle.spec:Requires: python3-pyflakes >= ...
>   python-flake8.spec:Requires: python3-pycodestyle >= ...
> 
> But I don't know enough about those packages…

pycodestyle does not require pyflakes

And flake8 already has a complex requires on the others:

(python3.10dist(mccabe) < 0.7 with python3.10dist(mccabe) >= 0.6)
(python3.10dist(pycodestyle) < 2.8 with python3.10dist(pycodestyle) >= 2.7)
(python3.10dist(pyflakes) < 2.4 with python3.10dist(pyflakes) >= 2.3)


Note that mu was broken by the upgrade, I'd appreciate a heads up next time: bz1979411

Comment 6 Miro Hrončok 2021-07-07 08:07:48 UTC
Spec sanity:


> URL:            https://github.com/python-lsp/%{name}

I find this rather hard to hrasp, especially since the entire spec file rather uses %{short_name}. COnsidder expanding the URL fully, so it is easier to copy paste for whover needs to read it from the spec.


> Source0:        https://files.pythonhosted.org/packages/source/p/%{name}/%{name}-%{version}.tar.gz

Please, use %{pypi_source} instead. The exact URL is not considered future-stable.


> %description
> %_description
> ...
> %description -n python3-%{short_name}
> %_description

The descriptions now start with a newline. Use this instead to avoid it:

  %description %_description
  %description -n python3-%{short_name} %_description



> # Remove bundled egg-info
> rm -rf %{pypi_name}.egg-info

This is not required with pyproject macros. In fact, it is most likely not ever required.



> %check
> pytest

Is it possible to use the documented %pytest macro instead?

Comment 7 Zbigniew Jędrzejewski-Szmek 2021-07-07 08:11:58 UTC
> And flake8 already has a complex requires on the others:
>
> (python3.10dist(mccabe) < 0.7 with python3.10dist(mccabe) >= 0.6)
> (python3.10dist(pycodestyle) < 2.8 with python3.10dist(pycodestyle) >= 2.7)
> (python3.10dist(pyflakes) < 2.4 with python3.10dist(pyflakes) >= 2.3)

I does, but apparently those requirements are not enough: the builds failed in tests
when done in the "wrong" order.

> I do not see any build failures for python-lsp-jsonrpc. Here is a build after pycodestyle, etc. were updated - https://koji.fedoraproject.org/koji/taskinfo?taskID=71417694

Hmm, the builds still fail in mock for me. Doesn't matter, I used this mock build for review instead.

+ package name is OK
+ license is acceptable for Fedora (MIT)
+ license is specified correctly
+ builds and installs OK
+ BR/R/P look OK (*)
+ rpmlint shows nothing

In general it's the modern packaging template, so there isn't much to review…
All the things that Miro pointed out above get a +1 from me.
Please consider implementing them when importing.

Package is APPROVED.

(*) I see "python3dist(python-lsp-jsonrpc)" in the autogenerated Provides.
I would expect "python3dist(lsp-jsonrpc)". Not sure what to make of this.

Comment 8 Miro Hrončok 2021-07-07 08:18:25 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #7)
> (*) I see "python3dist(python-lsp-jsonrpc)" in the autogenerated Provides.
> I would expect "python3dist(lsp-jsonrpc)". Not sure what to make of this.

That's because "python-lsp-jsonrpc" is the actual Python package name. See for example https://pypi.org/project/python-lsp-jsonrpc/

Comment 9 Miro Hrončok 2021-07-07 08:21:28 UTC
The package does not build for me:

+ pytest
============================= test session starts ==============================
platform linux -- Python 3.10.0b3, pytest-6.2.4, py-1.10.0, pluggy-0.13.1
rootdir: /builddir/build/BUILD/python-lsp-jsonrpc-1.0.0, configfile: setup.cfg, testpaths: test
plugins: cov-2.11.1
collected 27 items

test/test_endpoint.py ......................                             [ 81%]
test/test_streams.py ....F                                               [100%]

=================================== FAILURES ===================================
___________________________ test_writer_bad_message ____________________________

wfile = <_io.BytesIO object at 0x7febc95eba10>
writer = <pylsp_jsonrpc.streams.JsonRpcStreamWriter object at 0x7febc8a5bb50>

    def test_writer_bad_message(wfile, writer):
        # A datetime isn't serializable(or poorly serializable),
        # ensure the write method doesn't throw, but the result could be empty
        # or the correct datetime
        datetime.datetime = JsonDatetime
        writer.write(datetime.datetime(
            year=2019,
            month=1,
            day=1,
            hour=1,
            minute=1,
            second=1,
        ))
    
>       assert wfile.getvalue() in [
            b'',
            b'Content-Length: 10\r\n'
            b'Content-Type: application/vscode-jsonrpc; charset=utf8\r\n'
            b'\r\n'
            b'1546304461',
            b'Content-Length: 10\r\n'
            b'Content-Type: application/vscode-jsonrpc; charset=utf8\r\n'
            b'\r\n'
            b'1546322461'
        ]
E       AssertionError: assert b'Content-Length: 10\r\nContent-Type: application/vscode-jsonrpc; charset=utf8\r\n\r\n1546300861' in [b'', b'Content-Length: 10\r\nContent-Type: application/vscode-jsonrpc; charset=utf8\r\n\r\n1546304461', b'Content-Length: 10\r\nContent-Type: application/vscode-jsonrpc; charset=utf8\r\n\r\n1546322461']
E        +  where b'Content-Length: 10\r\nContent-Type: application/vscode-jsonrpc; charset=utf8\r\n\r\n1546300861' = <built-in method getvalue of _io.BytesIO object at 0x7febc95eba10>()
E        +    where <built-in method getvalue of _io.BytesIO object at 0x7febc95eba10> = <_io.BytesIO object at 0x7febc95eba10>.getvalue

test/test_streams.py:118: AssertionError
- generated xml file: /builddir/build/BUILD/python-lsp-jsonrpc-1.0.0/pytest.xml -

----------- coverage: platform linux, python 3.10.0-beta-3 -----------
Name                           Stmts   Miss  Cover
--------------------------------------------------
pylsp_jsonrpc/__init__.py          2      0   100%
pylsp_jsonrpc/_version.py          2      0   100%
pylsp_jsonrpc/dispatchers.py      19     19     0%
pylsp_jsonrpc/endpoint.py        138      5    96%
pylsp_jsonrpc/exceptions.py       60      6    90%
pylsp_jsonrpc/streams.py          67     14    79%
test/__init__.py                   0      0   100%
test/test_endpoint.py            133      4    97%
test/test_streams.py              49      1    98%
--------------------------------------------------
TOTAL                            470     49    90%
Coverage HTML written to dir htmlcov

=========================== short test summary info ============================
FAILED test/test_streams.py::test_writer_bad_message - AssertionError: assert...
========================= 1 failed, 26 passed in 0.36s =========================

Comment 10 Miro Hrončok 2021-07-07 08:31:17 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #7)
> > And flake8 already has a complex requires on the others:
> >
> > (python3.10dist(mccabe) < 0.7 with python3.10dist(mccabe) >= 0.6)
> > (python3.10dist(pycodestyle) < 2.8 with python3.10dist(pycodestyle) >= 2.7)
> > (python3.10dist(pyflakes) < 2.4 with python3.10dist(pyflakes) >= 2.3)
> 
> I does, but apparently those requirements are not enough: the builds failed
> in tests
> when done in the "wrong" order.

I see. That means the BuildRequires are too relaxed. I thought you meant the runtime Requires.

Comment 11 Mukundan Ragavan 2021-07-07 11:47:56 UTC
(In reply to Miro Hrončok from comment #5)
> Note that mu was broken by the upgrade, I'd appreciate a heads up next time:
> bz1979411

Sorry about that. I did check though ... 

# dnf repoquery --releasever rawhide --whatrequires "python3dist(pycodestyle)"
Last metadata expiration check: 0:02:44 ago on Wed 07 Jul 2021 07:36:17 AM EDT.


For flake8, I got this - 

dnf repoquery --releasever rawhide --whatrequires "python3dist(flake8)"
Last metadata expiration check: 0:02:54 ago on Wed 07 Jul 2021 07:36:17 AM EDT.
python3-molecule-0:3.2.4-2.fc35.noarch
python3-pep8-naming-0:0.11.1-4.fc35.noarch


I checked molecule and pep8-naming. They did not have issues with upgraded versions.

Comment 12 Mukundan Ragavan 2021-07-07 11:49:22 UTC
(In reply to Miro Hrončok from comment #9)
> The package does not build for me:
> 
> + pytest
> ============================= test session starts
> ==============================
> platform linux -- Python 3.10.0b3, pytest-6.2.4, py-1.10.0, pluggy-0.13.1
> rootdir: /builddir/build/BUILD/python-lsp-jsonrpc-1.0.0, configfile:
> setup.cfg, testpaths: test
> plugins: cov-2.11.1
> collected 27 items
> 
> test/test_endpoint.py ......................                             [
> 81%]
> test/test_streams.py ....F                                              
> [100%]
> 
> =================================== FAILURES
> ===================================
> ___________________________ test_writer_bad_message
> ____________________________
> 
> wfile = <_io.BytesIO object at 0x7febc95eba10>
> writer = <pylsp_jsonrpc.streams.JsonRpcStreamWriter object at 0x7febc8a5bb50>
> 
>     def test_writer_bad_message(wfile, writer):
>         # A datetime isn't serializable(or poorly serializable),
>         # ensure the write method doesn't throw, but the result could be
> empty
>         # or the correct datetime
>         datetime.datetime = JsonDatetime
>         writer.write(datetime.datetime(
>             year=2019,
>             month=1,
>             day=1,
>             hour=1,
>             minute=1,
>             second=1,
>         ))
>     
> >       assert wfile.getvalue() in [
>             b'',
>             b'Content-Length: 10\r\n'
>             b'Content-Type: application/vscode-jsonrpc; charset=utf8\r\n'
>             b'\r\n'
>             b'1546304461',
>             b'Content-Length: 10\r\n'
>             b'Content-Type: application/vscode-jsonrpc; charset=utf8\r\n'
>             b'\r\n'
>             b'1546322461'
>         ]
> E       AssertionError: assert b'Content-Length: 10\r\nContent-Type:
> application/vscode-jsonrpc; charset=utf8\r\n\r\n1546300861' in [b'',
> b'Content-Length: 10\r\nContent-Type: application/vscode-jsonrpc;
> charset=utf8\r\n\r\n1546304461', b'Content-Length: 10\r\nContent-Type:
> application/vscode-jsonrpc; charset=utf8\r\n\r\n1546322461']
> E        +  where b'Content-Length: 10\r\nContent-Type:
> application/vscode-jsonrpc; charset=utf8\r\n\r\n1546300861' = <built-in
> method getvalue of _io.BytesIO object at 0x7febc95eba10>()
> E        +    where <built-in method getvalue of _io.BytesIO object at
> 0x7febc95eba10> = <_io.BytesIO object at 0x7febc95eba10>.getvalue
> 
> test/test_streams.py:118: AssertionError
> - generated xml file:
> /builddir/build/BUILD/python-lsp-jsonrpc-1.0.0/pytest.xml -
> 
> ----------- coverage: platform linux, python 3.10.0-beta-3 -----------
> Name                           Stmts   Miss  Cover
> --------------------------------------------------
> pylsp_jsonrpc/__init__.py          2      0   100%
> pylsp_jsonrpc/_version.py          2      0   100%
> pylsp_jsonrpc/dispatchers.py      19     19     0%
> pylsp_jsonrpc/endpoint.py        138      5    96%
> pylsp_jsonrpc/exceptions.py       60      6    90%
> pylsp_jsonrpc/streams.py          67     14    79%
> test/__init__.py                   0      0   100%
> test/test_endpoint.py            133      4    97%
> test/test_streams.py              49      1    98%
> --------------------------------------------------
> TOTAL                            470     49    90%
> Coverage HTML written to dir htmlcov
> 
> =========================== short test summary info
> ============================
> FAILED test/test_streams.py::test_writer_bad_message - AssertionError:
> assert...
> ========================= 1 failed, 26 passed in 0.36s
> =========================




It builds for me. Here is one with the correct macro - https://koji.fedoraproject.org/koji/taskinfo?taskID=71450407


+ CFLAGS='-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -m64 -mcpu=power8 -mtune=power8 -fasynchronous-unwind-tables -fstack-clash-protection'
+ LDFLAGS='-Wl,-z,relro -Wl,--as-needed  -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld '
+ PATH=/builddir/build/BUILDROOT/python-lsp-jsonrpc-1.0.0-1.fc35.noarch/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/sbin
+ PYTHONPATH=/builddir/build/BUILDROOT/python-lsp-jsonrpc-1.0.0-1.fc35.noarch/usr/lib64/python3.10/site-packages:/builddir/build/BUILDROOT/python-lsp-jsonrpc-1.0.0-1.fc35.noarch/usr/lib/python3.10/site-packages
+ PYTHONDONTWRITEBYTECODE=1
+ PYTEST_ADDOPTS=' --ignore=/builddir/build/BUILD/python-lsp-jsonrpc-1.0.0/.pyproject-builddir'
+ /usr/bin/pytest
============================= test session starts ==============================
platform linux -- Python 3.10.0b3, pytest-6.2.4, py-1.10.0, pluggy-0.13.1
rootdir: /builddir/build/BUILD/python-lsp-jsonrpc-1.0.0, configfile: setup.cfg, testpaths: test
plugins: cov-2.11.1
collected 27 items
test/test_endpoint.py ......................                             [ 81%]
test/test_streams.py .....                                               [100%]
- generated xml file: /builddir/build/BUILD/python-lsp-jsonrpc-1.0.0/pytest.xml -
----------- coverage: platform linux, python 3.10.0-beta-3 -----------
Name                           Stmts   Miss  Cover
--------------------------------------------------
pylsp_jsonrpc/__init__.py          2      0   100%
pylsp_jsonrpc/_version.py          2      0   100%
pylsp_jsonrpc/dispatchers.py      19     19     0%
pylsp_jsonrpc/endpoint.py        138      5    96%
pylsp_jsonrpc/exceptions.py       60      6    90%
pylsp_jsonrpc/streams.py          67     14    79%
test/__init__.py                   0      0   100%
test/test_endpoint.py            133      4    97%
test/test_streams.py              49      1    98%
--------------------------------------------------
TOTAL                            470     49    90%
Coverage HTML written to dir htmlcov
============================== 27 passed in 0.58s ==============================

Comment 13 Mukundan Ragavan 2021-07-07 11:50:29 UTC
(In reply to Miro Hrončok from comment #6)
> Spec sanity:
> 
> 
> > URL:            https://github.com/python-lsp/%{name}
> 
> I find this rather hard to hrasp, especially since the entire spec file
> rather uses %{short_name}. COnsidder expanding the URL fully, so it is
> easier to copy paste for whover needs to read it from the spec.
> 
> 
> > Source0:        https://files.pythonhosted.org/packages/source/p/%{name}/%{name}-%{version}.tar.gz
> 
> Please, use %{pypi_source} instead. The exact URL is not considered
> future-stable.
> 
> 
> > %description
> > %_description
> > ...
> > %description -n python3-%{short_name}
> > %_description
> 
> The descriptions now start with a newline. Use this instead to avoid it:
> 
>   %description %_description
>   %description -n python3-%{short_name} %_description
> 
> 
> 
> > # Remove bundled egg-info
> > rm -rf %{pypi_name}.egg-info
> 
> This is not required with pyproject macros. In fact, it is most likely not
> ever required.
> 
> 
> 
> > %check
> > pytest
> 
> Is it possible to use the documented %pytest macro instead?



I will implement all these changes during import and update other package reviews as well.

Comment 14 Mukundan Ragavan 2021-07-07 11:52:09 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #7)
> > And flake8 already has a complex requires on the others:
> >
> > (python3.10dist(mccabe) < 0.7 with python3.10dist(mccabe) >= 0.6)
> > (python3.10dist(pycodestyle) < 2.8 with python3.10dist(pycodestyle) >= 2.7)
> > (python3.10dist(pyflakes) < 2.4 with python3.10dist(pyflakes) >= 2.3)
> 
> I does, but apparently those requirements are not enough: the builds failed
> in tests
> when done in the "wrong" order.
> 
> > I do not see any build failures for python-lsp-jsonrpc. Here is a build after pycodestyle, etc. were updated - https://koji.fedoraproject.org/koji/taskinfo?taskID=71417694
> 
> Hmm, the builds still fail in mock for me. Doesn't matter, I used this mock
> build for review instead.
> 
> + package name is OK
> + license is acceptable for Fedora (MIT)
> + license is specified correctly
> + builds and installs OK
> + BR/R/P look OK (*)
> + rpmlint shows nothing
> 
> In general it's the modern packaging template, so there isn't much to review…
> All the things that Miro pointed out above get a +1 from me.
> Please consider implementing them when importing.
> 
> Package is APPROVED.
> 
> (*) I see "python3dist(python-lsp-jsonrpc)" in the autogenerated Provides.
> I would expect "python3dist(lsp-jsonrpc)". Not sure what to make of this.



I will implement all of Miro's comment when importing. Thanks for the review. I have submitted a scm-request.


I will also look at the other two packages for similar issues.

Comment 15 Gwyn Ciesla 2021-07-07 14:37:31 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/python-lsp-jsonrpc

Comment 16 Mukundan Ragavan 2021-07-07 16:36:05 UTC
Built on rawhide.


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