Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dev-python/python-language-server: allow newer ujson versions #17882

Conversation

AndrewAmmerlaan
Copy link
Member

This fixes a bug in pyls_jsonrpc preventing the use of ujson newer than 1.35 in pyls.

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @AndrewAmmerlaan
Areas affected: ebuilds
Packages affected: dev-python/python-jsonrpc-server, dev-python/python-language-server

dev-python/python-jsonrpc-server: @AndrewAmmerlaan, @gentoo/proxy-maint
dev-python/python-language-server: @AndrewAmmerlaan, @gentoo/proxy-maint

Linked bugs

Bugs linked: 715290


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added self-maintained The PR changes only packages that are maintained by the submitter (i.e. no need to ask anybody else) assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. labels Oct 11, 2020
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-10-11 12:01 UTC
Newest commit scanned: 2d97932
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/8ceda339ca/output.html

Copy link
Member

@juippis juippis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you revbump both of these so people who already have these installed can update to later ujson as well? git mv with your changes is fine here.

@AndrewAmmerlaan AndrewAmmerlaan force-pushed the dev-python/python-language-server branch from 2d97932 to 87ab055 Compare October 12, 2020 09:20
@AndrewAmmerlaan
Copy link
Member Author

Could you revbump both of these so people who already have these installed can update to later ujson as well? git mv with your changes is fine here.

Done

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-10-12 09:36 UTC
Newest commit scanned: 87ab055
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/9d61aac212/output.html

Copy link
Member

@juippis juippis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sooo after your update the tests fail with updated ujson,

============================================== FAILURES ==============================================
______________________________________ test_writer_bad_message _______________________________________

wfile = <_io.BytesIO object at 0x7f8541835e90>
writer = <pyls_jsonrpc.streams.JsonRpcStreamWriter object at 0x7f8541850210>

    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'1546300861'
            ]
E       AssertionError: assert b'Content-Length: 10\r\nContent-Type: application/vscode-jsonrpc; charset=utf8\r\n\r\n1546304461' in [b'Content-Length: 10\r\nContent-Type: application/vscode-jsonrpc; charset=utf8\r\n\r\n1546300861']
E        +  where b'Content-Length: 10\r\nContent-Type: application/vscode-jsonrpc; charset=utf8\r\n\r\n1546304461' = <built-in method getvalue of _io.BytesIO object at 0x7f8541835e90>()
E        +    where <built-in method getvalue of _io.BytesIO object at 0x7f8541835e90> = <_io.BytesIO object at 0x7f8541835e90>.getvalue

test/test_streams.py:124: AssertionError
====================================== short test summary info =======================================
FAILED test/test_streams.py::test_writer_bad_message - AssertionError: assert b'Content-Length: 10\...
==================================== 1 failed, 26 passed in 0.09s ====================================
 * ERROR: dev-python/python-jsonrpc-server-0.4.0-r1::testworld failed (test phase):
 *   Tests fail with python3.7

But the ::gentoo version passes all tests. So there's definitely still something broken with this.

@AndrewAmmerlaan
Copy link
Member Author

Which ujson version where you using? this was working for me with ujson-4.0.0

@juippis
Copy link
Member

juippis commented Oct 12, 2020

[ebuild R ] dev-python/ujson-4.0.1::gentoo

Let me try with 4.0.0
Nope, it still fails :\
So it must be some missing dep issue.

Basically my deps are always the latest available version.

@AndrewAmmerlaan
Copy link
Member Author

Basically my deps are always the latest available version.

Same, but I just retested and this works for me, so I'm a bit confused now

Lets compare equery depgraphs:

* dependency graph for dev-python/python-jsonrpc-server-0.4.0
`--  dev-python/python-jsonrpc-server-0.4.0  ~amd64 
`--  dev-python/ujson-4.0.0  (>=dev-python/ujson-3) ~amd64  [python_targets_python3_7(-)? python_targets_python3_8(-)? python_targets_python3_9(-)? -python_single_target_python3_7(-) -python_single_target_python3_8(-) -python_single_target_python3_9(-)]
`--  dev-lang/python-3.7.9  (dev-lang/python) ~amd64 
`--  dev-lang/python-3.8.6  (dev-lang/python) ~amd64 
`--  dev-lang/python-3.9.0  (dev-lang/python) ~amd64 
`--  dev-lang/python-exec-2.4.6-r2  (>=dev-lang/python-exec-2) ~amd64  [python_targets_python3_7(-)? python_targets_python3_8(-)? python_targets_python3_9(-)? -python_single_target_python3_7(-) -python_single_target_python3_8(-) -python_single_target_python3_9(-)]
`--  dev-python/versioneer-0.18-r1  (dev-python/versioneer) amd64  [python_targets_python3_7(-)? python_targets_python3_8(-)? python_targets_python3_9(-)? -python_single_target_python3_7(-) -python_single_target_python3_8(-) -python_single_target_python3_9(-)]
`--  dev-python/mock-3.0.5-r2  (dev-python/mock) amd64  [python_targets_python3_7(-)? python_targets_python3_8(-)? python_targets_python3_9(-)? -python_single_target_python3_7(-) -python_single_target_python3_8(-) -python_single_target_python3_9(-)]
`--  dev-python/pycodestyle-2.6.0  (dev-python/pycodestyle) amd64  [python_targets_python3_7(-)? python_targets_python3_8(-)? python_targets_python3_9(-)? -python_single_target_python3_7(-) -python_single_target_python3_8(-) -python_single_target_python3_9(-)]
`--  dev-python/pytest-6.1.1  (>=dev-python/pytest-4.5.0) ~amd64  [python_targets_python3_7(-)? python_targets_python3_8(-)? python_targets_python3_9(-)? -python_single_target_python3_7(-) -python_single_target_python3_8(-) -python_single_target_python3_9(-)]
`--  dev-python/setuptools-50.3.0  (>=dev-python/setuptools-42.0.2) ~amd64  [python_targets_python3_7(-)? python_targets_python3_8(-)? python_targets_python3_9(-)? -python_single_target_python3_7(-)-python_single_target_python3_8(-) -python_single_target_python3_9(-)]
[ dev-python/python-jsonrpc-server-0.4.0 stats: packages (11), max depth (1) ]

@juippis
Copy link
Member

juippis commented Oct 13, 2020

* dependency graph for dev-python/python-jsonrpc-server-0.4.0

Do note that your version is -r1, the ::gentoo version (-r0) works for me too as I said previously. But here's mine:

 * dependency graph for dev-python/python-jsonrpc-server-0.4.0-r1
 `--  dev-python/python-jsonrpc-server-0.4.0-r1  ~amd64 
   `--  dev-python/ujson-4.0.1  (>=dev-python/ujson-3) ~amd64  [python_targets_python3_7(-)? python_targets_python3_8(-)? python_targets_python3_9(-)? -python_single_target_python3_7(-) -python_single_target_python3_8(-) -python_single_target_python3_9(-)]
   `--  dev-lang/python-3.7.9  (dev-lang/python) ~amd64 
   `--  dev-lang/python-3.8.6  (dev-lang/python) ~amd64 
   `--  dev-lang/python-3.9.0  (dev-lang/python) ~amd64 
   `--  dev-lang/python-exec-2.4.6-r2  (>=dev-lang/python-exec-2) ~amd64  [python_targets_python3_7(-)? python_targets_python3_8(-)? python_targets_python3_9(-)? -python_single_target_python3_7(-) -python_single_target_python3_8(-) -python_single_target_python3_9(-)]
   `--  dev-python/versioneer-0.18-r1  (dev-python/versioneer) amd64  [python_targets_python3_7(-)? python_targets_python3_8(-)? python_targets_python3_9(-)? -python_single_target_python3_7(-) -python_single_target_python3_8(-) -python_single_target_python3_9(-)]
   `--  dev-python/mock-3.0.5-r2  (dev-python/mock) amd64  [python_targets_python3_7(-)? python_targets_python3_8(-)? python_targets_python3_9(-)? -python_single_target_python3_7(-) -python_single_target_python3_8(-) -python_single_target_python3_9(-)]
   `--  dev-python/pycodestyle-2.6.0  (dev-python/pycodestyle) amd64  [python_targets_python3_7(-)? python_targets_python3_8(-)? python_targets_python3_9(-)? -python_single_target_python3_7(-) -python_single_target_python3_8(-) -python_single_target_python3_9(-)]
   `--  dev-python/pytest-6.1.1  (>=dev-python/pytest-4.5.0) ~amd64  [python_targets_python3_7(-)? python_targets_python3_8(-)? python_targets_python3_9(-)? -python_single_target_python3_7(-) -python_single_target_python3_8(-) -python_single_target_python3_9(-)]
   `--  dev-python/setuptools-50.3.0  (>=dev-python/setuptools-42.0.2) ~amd64  [python_targets_python3_7(-)? python_targets_python3_8(-)? python_targets_python3_9(-)? -python_single_target_python3_7(-) -python_single_target_python3_8(-) -python_single_target_python3_9(-)]
[ dev-python/python-jsonrpc-server-0.4.0-r1 stats: packages (11), max depth (1) ]

@AndrewAmmerlaan
Copy link
Member Author

Do note that your version is -r1, the ::gentoo version (-r0) works for me too as I said previously. But here's mine:

This is because the ebuild in my local overlay does not have the -r1 suffix.

Well I'm even more confused now, does it work for you without the patch? Because the error you're getting looks suspiciously like the patch.

@juippis
Copy link
Member

juippis commented Oct 13, 2020

Yep, they seem to pass happily without the patch.

@AndrewAmmerlaan
Copy link
Member Author

Yep, they seem to pass happily without the patch.

The plot thickens, I get this without the patch:

=============================================================================================== FAILURES ================================================================================================
________________________________________________________________________________________ test_writer_bad_message ________________________________________________________________________________________

wfile = <_io.BytesIO object at 0x7efc2afa0bf0>, writer = <pyls_jsonrpc.streams.JsonRpcStreamWriter object at 0x7efc2afafd90>

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'
]
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']
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 0x7efc2afa0bf0>()
E        +    where <built-in method getvalue of _io.BytesIO object at 0x7efc2afa0bf0> = <_io.BytesIO object at 0x7efc2afa0bf0>.getvalue

test/test_streams.py:124: AssertionError

@AndrewAmmerlaan AndrewAmmerlaan force-pushed the dev-python/python-language-server branch from 87ab055 to 7ef354d Compare October 13, 2020 18:21
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-10-13 18:41 UTC
Newest commit scanned: 7ef354d
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/495ebfc611/output.html

@juippis
Copy link
Member

juippis commented Oct 14, 2020

I get the error you posted with the patch you included here. If no pathes are applied, python-jsonrpc-server-0.4.0-r1 passes all tests and installs just fine. Are you doing something weird on your end? :)

I'm applying this on top of ::gentoo tree and running emerge.

@AndrewAmmerlaan
Copy link
Member Author

Are you doing something weird on your end? :)

Not that I know

Looking at the PR where I got this patch the upstream CI checks seem to be failing as well: palantir/python-jsonrpc-server#49

Though presumably the submitter wrote the patch in the first place because they were getting this error.

So it seems like some people need this patch to get the test to work, and for others the patch breaks the test.

I don't know what's going on, but the test seems to be doing something with date and time, so I suspect locale settings are causing these issues.

@rossbridger
Copy link
Contributor

Probably upstream hasn't fully fixed this issue.

@rossbridger
Copy link
Contributor

I took the liberty of fixing this issue, apply

diff --git a/test/test_streams.py b/test/test_streams.py
index 6985aec..9e64489 100644
--- a/test/test_streams.py
+++ b/test/test_streams.py
@@ -119,6 +119,7 @@ def test_writer_bad_message(wfile, writer):
         hour=1,
         minute=1,
         second=1,
+        tzinfo=datetime.timezone.utc
     ))

     assert wfile.getvalue() in [

It should work like a charm.

Closes: https://bugs.gentoo.org/715290
Package-Manager: Portage-3.0.8, Repoman-3.0.1
Signed-off-by: Andrew Ammerlaan <andrewammerlaan@riseup.net>
Bug: https://bugs.gentoo.org/715290
Package-Manager: Portage-3.0.8, Repoman-3.0.1
Signed-off-by: Andrew Ammerlaan <andrewammerlaan@riseup.net>
@AndrewAmmerlaan AndrewAmmerlaan force-pushed the dev-python/python-language-server branch from 7ef354d to 39ed72d Compare October 16, 2020 08:19
@AndrewAmmerlaan
Copy link
Member Author

I took the liberty of fixing this issue, apply

diff --git a/test/test_streams.py b/test/test_streams.py
index 6985aec..9e64489 100644
--- a/test/test_streams.py
+++ b/test/test_streams.py
@@ -119,6 +119,7 @@ def test_writer_bad_message(wfile, writer):
         hour=1,
         minute=1,
         second=1,
+        tzinfo=datetime.timezone.utc
     ))

     assert wfile.getvalue() in [

It should work like a charm.

Awesome, Thank You!

This works for me, @juippis does this fix the issue for you too?

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-10-16 08:31 UTC
Newest commit scanned: 39ed72d
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/12c3b7cbde/output.html

Copy link
Member

@juippis juippis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, both of you!

@AndrewAmmerlaan AndrewAmmerlaan deleted the dev-python/python-language-server branch October 16, 2020 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. self-maintained The PR changes only packages that are maintained by the submitter (i.e. no need to ask anybody else)
Projects
None yet
6 participants