[Twisted-Python] resolving release management conflict

[Craig Rodrigues]

On Mon, Feb 1, 2021 at 1:00 PM Glyph <glyph at twistedmatrix.com> wrote:

>
> I believe the proximate cause of the conflict here is that we're dealing
> with this regression incorrectly.  When a regression is introduced, there's
> a process for dealing with it, originally documented here:
> https://twistedmatrix.com/trac/wiki/ReviewProcess#Revertingachange. [1]
>  The principle we need to outline clearly is this: if we have a regression,
> the change that introduced it should be reverted as soon as possible.  If
> we *can't* revert it directly because of changes to trunk in the
> meanwhile that introduce conflicts, ideally the change that we would make
> would be the closest possible thing to reverting it, even if that
> reintroduces a known bug that we have to reopen.
>
> In the case where a lot of work has happened in the interim, and it's *less
> work* to just "fix forward" on top of trunk, it's fine to do a small fix
> as an end run around this process.  The goal of this process is to minimize
> the work required to restore trunk to a releasable state.
>
> Right now we have three competing fixes which have all been flagged as
> incorrect in various ways:
>
>
>    - from Tom: https://github.com/twisted/twisted/pull/1499
>       - Craig rejected this one by saying it will break "certain
>       versions" of Python, but I don't understand why it was closed rather than
>       reviewed / reassigned as normal.  (Or for that matter which versions are
>       the problems, or how it breaks them.)
>
>
Yes, that is correct.  Over the summer, while investigating failures on
Azure CI caused by to non-ASCII characters in
a submitter's GitHub user ID.  I spent a lot of time testing in a Windows
environment on Python 3.5, 3.6, 3.7, and 3.8.
I found that there were subtle differences in the default encoding on the
console between each Python version,
and that affected the logic in _checkProcessArgs().
The fix that I did over the summer addressed the problems and fixed all the
CI issues.

There were two corner cases that were uncovered.  One by me, and one by Tom
Most.

CORNER CASE 1:    In Buildbot, they patch sys.stdout with io.StringIO in
their tests which affects their tests which call the Twisted Reactor.
                                   io.StringIO is not a 100% proper
replacement for sys.stdout which is of type _io.TextIOWrapper on Python 3+.
                                   However, Buildbot's tests have been
around for a long time, so I decided

CORNER CASE 2:    In a Windows GUI Application, sys.stdout is None.  So you
cannot get the encoding from sys.stdout.encoding


In https://github.com/twisted/twisted/pull/1502, I have a very small patch
which addresses those two corner cases.
I need to write a good test for it, but I think it is good enough to go
into the release.

Post-release:
(1) I would like to drop Python 3.5
(2)  Look at removing the _checkProcessArgs() function.  The complexity of
bytes/unicode logic in there was more useful
       in the Python 2 days when dealing with the differences between how
Python on Windows handled args/env vs.
       how Python on POSIX handled args/env.  I think this function can
completely go in a Python 3.6+ world, and will greatly simplify the code.
       However, this needs to be carefully tested and reviewed.



>
>    - from Adi: https://github.com/twisted/twisted/pull/1501
>       - Craig rejected this one by saying "please stop working on this"
>       but it also doesn't have any technical reason that this solution is
>       incorrect.
>
> Yes, that is correct.  Sorry if that was an aggressive action on my part,
but I did a fix for this, as described above.






>
>    - from Craig: https://github.com/twisted/twisted/pull/1502
>       - This was originally given a passing review by Povilas from the
>       Buildbot project, but twm added an additional blocking review shortly
>       thereafter explaining that it introduced an additional regression.  It
>       seems as though this one is now actively being worked on?
>
>
> Craig: you closed two of these issues on the basis that you were "already
> working on a fix", but that's not a valid reason to *close* a PR.  Or,
> for that matter, to give it a failing review.  In the future, please
> comment on PRs that you don't want merged by pointing out their technical
> insufficiency, so that we can take the opportunity to learn for the benefit
> of future maintenance.  In this case, the underlying issue is clearly quite
> nuanced and requires expertise that touches platform differences in
> Python's core APIs, so having multiple people working on different fixes is
> totally fine; this effort is not wasted if everybody learns a little bit
> about the code in question, and it's *definitely* not wasted if we have
> to synthesize our approaches in the end anyway because they address
> different cases.
>

OK, thanks for the feedback, and sorry for any process violations or hurt
feelings from my actions.



>
> All of these fixes have each been outstanding for weeks, which strongly
> suggests that this is *way* too hard to fix forward and we should have
> stopped trying a while ago.  We should fall back to the previous one if in
> *any* way possible, then reintroduce the changes which caused it.  If
> we're really just about to land 1502, then feel free to complete the work
> there, but in the future we should regard this kind of delay in reverting
> as a process failure.
>

No, the fixes have been outstanding for weeks mainly due to the fact
that certain personal circumstances came up in my life, such that I had to
refocus my time away from Twisted for a few months,
compared to the time I had over the summer of 2020.  I now have more time,
and am committed to getting this release out.



>
> If there are too many conflicts to address in a revert, and reverting
> means we need to revert 3 or 4 tickets which have built on top of that
> work, then let's just do that and get those re-merged later as well.  It's
> unfortunate to ship without features but it's better than multi-month
> release hold-ups.
>
> (Note: a straight revert doesn't require a full code review.  If you can
> revert a merge and CI is happy with it, feel free to get an administrator
> to smash the red button to merge immediately.)
>
>
>    1. Can we revert the change that introduced 10069 so that we can
>    follow the process to re-introduce it after a release has been made, and
>    have the conversation about which PR to accept and why post-release?
>
>
I would rather not do this.  I would rather proceed with
https://github.com/twisted/twisted/pull/1502 , which is minimal,
and addresses the corner cases.

Going through the whole revert, resubmit, retest is going to add more delay
to the release.



>
>    1. Do we need to revert the release process changes from
>    https://github.com/twisted/twisted/pull/1464 for this release?  If so,
>    what is the specific technical need this addresses?
>
>
I do not agree with the changes in PR 1464.  I think a better solution
would have been to have a separate release.yml workflow,
which could be triggered.  By clicking the "Create a New Release" link on
GitHub.
I had a working prototype here: https://github.com/twisted/twisted/pull/1423
, and I successfully pushed some releases to
https://test.pypi.org

However, even though I disagree with the approach in PR 1464, since
multiple people have decided that this is what they want,
I'll just go with this.

--
Craig
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20210203/5d8890f1/attachment.htm>