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

Python: increase minimal supported cffi version #1520

Merged
merged 2 commits into from
Feb 25, 2021
Merged

Conversation

badboy
Copy link
Member

@badboy badboy commented Feb 25, 2021

I think this fixes test failures such as this one: https://app.circleci.com/pipelines/github/mozilla/glean/7162/workflows/33e489f9-56fa-4f0b-b2fa-c4bf03b52604/jobs/124598 (see #1507).

I ran it locally and saw this:

Traceback (most recent call last):
  File "/home/jer/src/mozilla/glean/glean-core/python/.venv3.8/lib64/python3.9/site-packages/glean_sdk-34.1.0-py3.9-linux-x86_64.egg/glean/_subprocess/_process_dispatcher_helper.py", line 36, in <module>
    success = func(*args)
  File "/home/jer/src/mozilla/glean/glean-core/python/.venv3.8/lib64/python3.9/site-packages/glean_sdk-34.1.0-py3.9-linux-x86_64.egg/glean/net/ping_upload_worker.py", line 152, in _process
    time.sleep(incoming_task.wait / 1000)
OverflowError: timestamp too large to convert to C _PyTime_t

Adding some logging it shows that incoming_task.wait is some huge value, likely because it reads the wrong bytes from memory because of the wrong encoding of an anonymous struct inside a union.

cffi 1.0.0 was released in 2015.
cffi 1.11.5 was released in 2018.
IMO it's a safe-bet to increase our minimum required version there.

@badboy badboy marked this pull request as ready for review February 25, 2021 11:45
@auto-assign auto-assign bot requested a review from brizental February 25, 2021 11:46
@badboy badboy requested review from mdboom and removed request for brizental February 25, 2021 11:46
Copy link
Contributor

@mdboom mdboom left a comment

Choose a reason for hiding this comment

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

I assume you picked 1.11.5 to get this from the cffi changelog?

Issue #357: fix ffi.emit_python_code() which generated a buggy Python file if you are using a struct with an anonymous union field or vice-versa.

Makes sense to me. It doesn't look like cffi is vendored in m-c, but if it were, that should be updated also.

@badboy
Copy link
Member Author

badboy commented Feb 25, 2021

I assume you picked 1.11.5 to get this from the cffi changelog?

Issue #357: fix ffi.emit_python_code() which generated a buggy Python file if you are using a struct with an anonymous union field or vice-versa.

Makes sense to me. It doesn't look like cffi is vendored in m-c, but if it were, that should be updated also.

Yeah, I picked the first version which contained something about unions. I didn't confirm that it is THE thing, but the tests seem to pass so at least its no harm?

@badboy badboy force-pushed the upgrade-cffi-min-dep branch from 1c53e2d to bfa0577 Compare February 25, 2021 13:15
@badboy badboy enabled auto-merge February 25, 2021 13:15
@badboy badboy disabled auto-merge February 25, 2021 13:20
@badboy badboy force-pushed the upgrade-cffi-min-dep branch from bfa0577 to 3841ba1 Compare February 25, 2021 13:37
@badboy badboy requested a review from mdboom February 25, 2021 13:38
@badboy
Copy link
Member Author

badboy commented Feb 25, 2021

For visibility the commit message replicated here:

cffi 1.11.5 has a fix when using a struct with an anonymous union field.
Release changelog: cffi.readthedocs.io/en/latest/whatsnew.html#v1-11-5

Relevant part:

Issue #357: fix ffi.emit_python_code() which generated a buggy Python file if you are using a struct with an anonymous union field or vice-versa.

cffi 1.12.2 is the first one to actually support Python 3.8
Release changelog: cffi.readthedocs.io/en/latest/whatsnew.html#v1-12-2

Relevant part:

Added temporary workaround to compile on CPython 3.8.0a2.

cffi 1.13 is the first one NOT listed under "Older versions".
So I picked that for no other reason than "it's still visible in the overview":
cffi.readthedocs.io/en/latest/index.html

cffi 1.11.5 has a fix when using a struct with an anonymous union field.
Release changelog: https://cffi.readthedocs.io/en/latest/whatsnew.html#v1-11-5

Relevant part:

> Issue #357: fix ffi.emit_python_code() which generated a buggy Python file if you are using a struct with an anonymous union field or vice-versa.

cffi 1.12.2 is the first one to actually support Python 3.8
Release changelog: https://cffi.readthedocs.io/en/latest/whatsnew.html#v1-12-2

Relevant part:

> Added temporary workaround to compile on CPython 3.8.0a2.

cffi 1.13 is the first one NOT listed under "Older versions".
So I picked that for no other reason than "it's still visible in the overview":
https://cffi.readthedocs.io/en/latest/index.html
@badboy badboy force-pushed the upgrade-cffi-min-dep branch from 3841ba1 to 3f6ee47 Compare February 25, 2021 13:42
CHANGELOG.md Outdated Show resolved Hide resolved
glean-core/python/setup.py Show resolved Hide resolved
@badboy badboy merged commit d7e092e into main Feb 25, 2021
@badboy badboy deleted the upgrade-cffi-min-dep branch February 25, 2021 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants