This is an archive of the discontinued Mercurial Phabricator instance.

worker: adapt _blockingreader to work around a python3.8.[0-1] bug (issue6444)
ClosedPublic

Authored by mharbison72 on Tue, May 17, 3:33 PM.

Details

Summary

Python 3.8.0 is the latest I can load on Ubuntu 18.04, and I regularly hit the
TypeError because this function is missing. While it can be avoided by
disabling worker usage via config option, that's a bit obscure.

I'm limiting the function definition to the narrow range of affected pythons
because there were other bugs in this area that were worked around, that I don't
fully understand. See the bug report for discussions on why the narrow range,
and related commits working around other bugs.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mharbison72 created this revision.Tue, May 17, 3:33 PM

This is meant for stable.

The conflict resolution when merging this into default is a simple de-indent.

mjacob requested changes to this revision.Tue, May 17, 11:31 PM
mjacob added a subscriber: mjacob.
mjacob added inline comments.
mercurial/worker.py
86

This could change the size of b, which shouldn’t be done.

If pickle calls readinto() (e.g. on my machine, this happens when the pickled data contains a bytestring >= 2**16 bytes long), it passes a memoryview which doesn’t allow changing its size. Because the size of b is the size of the pickled object and readall() returns all bytes in the stream, containing the bytes for the pickled object + at least one trailing byte, this line would always raise a “ValueError: memoryview assignment: lvalue and rvalue have different structures” exception.

What I suggest instead is to mostly copy-paste the implementation of read() and modify it such that it reads into the provided buffer instead of creating one.

This revision now requires changes to proceed.Tue, May 17, 11:31 PM

I *think* you're supposed to open a "merge request" on https://foss.heptapod.net/mercurial/mercurial-devel these days. Right, @Alphare?

I *think* you're supposed to open a "merge request" on https://foss.heptapod.net/mercurial/mercurial-devel these days. Right, @Alphare?

I didn't think we made the switch yet, but I'm fine doing that as a pilot. (I ended up amending a commit comment typo since the last push, so it will need to run CI again.)

I *think* you're supposed to open a "merge request" on https://foss.heptapod.net/mercurial/mercurial-devel these days. Right, @Alphare?

I didn't think we made the switch yet, but I'm fine doing that as a pilot. (I ended up amending a commit comment typo since the last push, so it will need to run CI again.)

The reason I suggested it was that I queued the patch and tried to push it to hg-committed, but that failed and told me to create a merge request instead. So I suppose it's actually you who should create that merge request since it's your patch. I'm not sure I understood it correctly, though.

Alphare requested changes to this revision.Wed, May 18, 7:16 PM

I *think* you're supposed to open a "merge request" on https://foss.heptapod.net/mercurial/mercurial-devel these days. Right, @Alphare?

Right!

I didn't think we made the switch yet, but I'm fine doing that as a pilot. (I ended up amending a commit comment typo since the last push, so it will need to run CI again.)

I have actually already merged a couple of MRs using Heptapod.

The reason I suggested it was that I queued the patch and tried to push it to hg-committed, but that failed and told me to create a merge request instead. So I suppose it's actually you who should create that merge request since it's your patch. I'm not sure I understood it correctly, though.

Yup. Phabricator is deprecated and slated for read-only state on/around June 1st. The deprecation means that any new patches should be sent to Heptapod unless absolutely infeasible (because something is terribly broken), and old patches will be reviewed here, then put in a topic by yours truly for a CI pass and merged accordingly. Any diff that still needs review/turn-around by June 1st will need to be resent through Heptapod.

@mharbison72 you should have no trouble at all since you're already using Hetpapod for the CI. Please read the contributor guide here: https://www.mercurial-scm.org/wiki/Heptapod

I hope I'm clear, it's very late and my brain is half-working. :)

This revision now requires changes to proceed.Wed, May 18, 7:16 PM

@mharbison72 you should have no trouble at all since you're already using Hetpapod for the CI. Please read the contributor guide here: https://www.mercurial-scm.org/wiki/Heptapod
I hope I'm clear, it's very late and my brain is half-working. :)

Done. I couldn't remember where I saw these instructions, and I created it before you responded here, but I think I did it right. I don't see a "Ready" button now, but I do see one "Mark as Draft", so I assume it's already in that state.

Is there any way to link that wiki page from the top of the page that creates the merge request, for easier discoverability by new users?

baymax updated this revision to Diff 33418.Fri, May 20, 11:14 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)
⚠ This patch is intended for stable ⚠

baymax updated this revision to Diff 33447.Tue, May 24, 12:43 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)
⚠ This patch is intended for stable ⚠

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.