Py3kport: Foolscap porting

[Jean-Paul Calderone]

On Tue, Jul 23, 2019 at 3:06 PM Anand B Pillai <anand at anvetsu.com> wrote:

> On Tuesday 23 July 2019 12:16 PM, Anand B Pillai wrote:
>
> Hi all,
>     I have an open "PR" - for review purposes and early feedback only -
> against my own master branch
> of the foolscap fork from the py3kport branch.
>
> https://github.com/anvetsu/foolscap/pull/1
>
>
> Okay, so this code is not yet ready to be raised as a PR against the
> original repos master because
> it passes just one test (test_banana.py).
>
> I fixed some basic issues which was causing the Travis job to not run but
> all tests except
> test_banana.py will *fail* on Python2 (and Python3). So there is no point
> in raising a PR
> against the original repo at the moment.
>

This makes it sound like the plan is to break everything and then
incrementally fix pieces.  Then, once all the pieces work, the complete
changeset can be merged into master.  This isn't the plan that I would pick
but I'm not doing the bulk of the porting work so I'm not going to argue
too much about it.  The one thing I would suggest is that if this is the
plan then you also need to explicitly track which pieces are expected to be
broken and which pieces are expected to be working.  For example, CI should
be configured with a list of working pieces.  All PRs should include an
addition to this list.  CI can run just these tests and then provide the
same "green is good, red is bad" signal you get in the course of normal
development.  Without this, porting and review quickly becomes very
confusing and error prone as humans try to keep track in their head or with
other ad hoc means what is supposed to be working at each point in the
process.

Building this extra CI system is extra work and that's why I wouldn't pick
this plan.  Instead, I'd pick a plan that leverages the existing CI
system.  This is something like the "make one focused, well-defined set of
changes that add Python 3 support and don't break anything" plan that has
been proposed and agreed upon for the Tahoe-LAFS codebase itself.  Since
you're starting from "everything works" and maintaining "everything works"
the normal CI system continues to provide the desired check.  If you start
from "everything's broken" and then "fix one thing" you've broken the
assumptions of normal CI systems and need to fix the mismatch somehow.


> The "PR" which I have raised against my own fork's master is so that - any
> one interested could
> take a look at the changes and comment. And if you are interested, you can
> checkout my repo
> and run the test_banana.py test (only) on Python2 and Python3 and verify
> (or otherwise) that
> it passes for you.
>
> Once I make all tests pass - against Python2 and Python3  - I will be in a
> position to raise a PR against
> the original repo. It will be a big PR at that stage.
>
> Another option if of course - if we get commit rights to Warner's repo, I
> can raise frequent PRs against
> an "integration" branch - as discussed in call - as soon as test (or
> multiple tests) are passing - with
> the understanding that CI checks may still fail. We keep incrementally
> merging against this branch
> till all tests pass(along with CI checks) and we finally merge the
> "integration" branch to foolscap master.
>

Regardless of what the outcome of the discussion from above is, you can
still create an integration branch in your own repo and make PRs against
that branch.  When we get access to upstream Foolscap we can push the
integration branch there easily and then proceed with PRs against that
target instead.  Incremental merges are absolutely essential to the porting
process.  "a big PR" is never going to be reviewed or merged, period.
Every stage of the process should be looking at keeping the size of the
change for review & merge minimal.  As previously discussed, around 500
lines of diff is about as large as you really want to deal with at a time.
This is a maximum, not a target.  If there is a self-contained change
that's 50 lines of diff, that should be its own PR.  A diff of twice the
size is *more than* twice as hard to review.  If anything, for the Foolscap
codebase, with which *no one* on the porting has much experience, we should
be targeting a even smaller diff sizes (but the practice only scales down
so far due to properties of the language).

Thanks,
Jean-Paul


>
> Hope this is clear. Let me know if there are any questions.
>
> Thanks.
>
> I'd like someone to review the changes. Kindly email me your github id so
> I can add you
> to the project - and as a reviewer.
>
> There are many changed files though you can ignore almost all of them as
> they are changes made by the 2to3 tool
> and focus on the files listed in the PR comment.
>
> Expectations:
>
> 1. Checkout the branch and try the "test_banana.py" test on Python3 (Mine
> is 3.6.7, so try anything same and upwards)
> and Python2 (2.7.15)
> 2. Review the core files mentioned in the port and give feedback of any.
>
> I will close this "PR" after I get any feedback.
>
> Thanks.
>
> --
> Kind Regards,
>
> --Anand
>
> -----------------------------------
> Founder & Director,
>
> Anvetsu Technologies Pvt Ltd (OPC),
> https://www.anvetsu.comhttps://www.anvetsu.training
> -----------------------------------
>
>
> --
> Kind Regards,
>
> --Anand
>
> -----------------------------------
> Founder & Director,
>
> Anvetsu Technologies Pvt Ltd (OPC),
> https://www.anvetsu.comhttps://www.anvetsu.training
> -----------------------------------
>
> _______________________________________________
> tahoe-dev mailing list
> tahoe-dev at tahoe-lafs.org
> https://tahoe-lafs.org/cgi-bin/mailman/listinfo/tahoe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://tahoe-lafs.org/pipermail/tahoe-dev/attachments/20190724/a69ee5b7/attachment.html>