https://bugzilla.redhat.com/show_bug.cgi?id=1979275
Bug ID: 1979275 Summary: Review Request: python-lsp-jsonrpc - Python implementation of JSON RPC 2.0 protocol Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: nonamedotc@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://nonamedotc.fedorapeople.org/pkgreview/python-packages/python-lsp-jso... SRPM URL: https://nonamedotc.fedorapeople.org/pkgreview/python-packages/python-lsp-jso...
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
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
Mukundan Ragavan nonamedotc@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value
--- Comment #1 from Mukundan Ragavan nonamedotc@gmail.com --- koji scratch build -
https://koji.fedoraproject.org/koji/taskinfo?taskID=71344275
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
Mukundan Ragavan nonamedotc@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |1979315
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1979315 [Bug 1979315] Review Request: python-lsp-server - Python implementation of language server protocol
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
--- Comment #2 from Mukundan Ragavan nonamedotc@gmail.com --- 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/
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |zbyszek@in.waw.pl
--- Comment #3 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- 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 ;]
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
--- Comment #4 from Mukundan Ragavan nonamedotc@gmail.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
Miro Hrončok mhroncok@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mhroncok@redhat.com
--- Comment #5 from Miro Hrončok mhroncok@redhat.com --- (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
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
--- Comment #6 from Miro Hrončok mhroncok@redhat.com --- Spec sanity:
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/%%7Bname%7D/%%7Bname%7D-%%7...
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?
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |POST Assignee|nobody@fedoraproject.org |zbyszek@in.waw.pl Flags| |fedora-review+
--- Comment #7 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl ---
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
--- Comment #8 from Miro Hrončok mhroncok@redhat.com --- (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/
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
--- Comment #9 from Miro Hrončok mhroncok@redhat.com --- 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 =========================
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
--- Comment #10 from Miro Hrončok mhroncok@redhat.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
--- Comment #11 from Mukundan Ragavan nonamedotc@gmail.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
--- Comment #12 from Mukundan Ragavan nonamedotc@gmail.com --- (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 beempty # 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 ==============================
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
--- Comment #13 from Mukundan Ragavan nonamedotc@gmail.com --- (In reply to Miro Hrončok from comment #6)
Spec sanity:
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/%%7Bname%7D/%%7Bname%7D-%%7...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
--- Comment #14 from Mukundan Ragavan nonamedotc@gmail.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
--- Comment #15 from Gwyn Ciesla gwync@protonmail.com --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/python-lsp-jsonrpc
https://bugzilla.redhat.com/show_bug.cgi?id=1979275
Mukundan Ragavan nonamedotc@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |RAWHIDE Last Closed| |2021-07-07 16:36:05
--- Comment #16 from Mukundan Ragavan nonamedotc@gmail.com --- Built on rawhide.
package-review@lists.fedoraproject.org