<div dir="ltr"><div dir="ltr">On Tue, Jul 23, 2019 at 3:06 PM Anand B Pillai <<a href="mailto:anand@anvetsu.com">anand@anvetsu.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="#FFFFFF">
<div class="gmail-m_8870913889991612175moz-cite-prefix">On Tuesday 23 July 2019 12:16 PM, Anand
B Pillai wrote:<br>
</div>
<blockquote type="cite">
<p><font face="Bitstream Vera Sans Mono">Hi all,</font></p>
<font face="Bitstream Vera Sans Mono"> I have an open "PR" -
for review purposes and early feedback only - against my own
master branch</font><br>
<font face="Bitstream Vera Sans Mono">of the foolscap fork from
the py3kport branch.<br>
<br>
<a class="gmail-m_8870913889991612175moz-txt-link-freetext" href="https://github.com/anvetsu/foolscap/pull/1" target="_blank">https://github.com/anvetsu/foolscap/pull/1</a><br>
</font></blockquote>
<br>
Okay, so this code is not yet ready to be raised as a PR against the
original repos master because<br>
it passes just one test (test_banana.py). <br>
<br>
I fixed some basic issues which was causing the Travis job to not
run but all tests except <br>
test_banana.py will *fail* on Python2 (and Python3). So there is no
point in raising a PR<br>
against the original repo at the moment. <br></div></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
<br>
The "PR" which I have raised against my own fork's master is so that
- any one interested could<br>
take a look at the changes and comment. And if you are interested,
you can checkout my repo <br>
and run the test_banana.py test (only) on Python2 and Python3 and
verify (or otherwise) that<br>
it passes for you.<br>
<br>
Once I make all tests pass - against Python2 and Python3 - I will
be in a position to raise a PR against<br>
the original repo. It will be a big PR at that stage.<br>
<br>
Another option if of course - if we get commit rights to Warner's
repo, I can raise frequent PRs against<br>
an "integration" branch - as discussed in call - as soon as test (or
multiple tests) are passing - with <br>
the understanding that CI checks may still fail. We keep
incrementally merging against this branch <br>
till all tests pass(along with CI checks) and we finally merge the
"integration" branch to foolscap master.<br></div></blockquote><div><br></div><div><div>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 <i>more than</i> twice as hard to review. If anything, for the Foolscap codebase, with which <i>no one</i> 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).</div><div><br></div><div>Thanks,</div><div>Jean-Paul</div><div> <br></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
<br>
Hope this is clear. Let me know if there are any questions.<br>
<br>
Thanks.<br>
<br>
<blockquote type="cite"> I'd
like someone to review the changes. Kindly email me your github id
so I can add you<br>
to the project - and as a reviewer.<br>
<br>
There are many changed files though you can ignore almost all of
them as they are changes made by the 2to3 tool <br>
and focus on the files listed in the PR comment. <br>
<br>
Expectations:<br>
<br>
1. Checkout the branch and try the "test_banana.py" test on
Python3 (Mine is 3.6.7, so try anything same and upwards)<br>
and Python2 (2.7.15) <br>
2. Review the core files mentioned in the port and give feedback
of any.<br>
<br>
I will close this "PR" after I get any feedback.<br>
<br>
<p>Thanks.<br>
</p>
<pre class="gmail-m_8870913889991612175moz-signature" cols="72">--
Kind Regards,
--Anand
-----------------------------------
Founder & Director,
Anvetsu Technologies Pvt Ltd (OPC),
<a class="gmail-m_8870913889991612175moz-txt-link-freetext" href="https://www.anvetsu.com" target="_blank">https://www.anvetsu.com</a>
<a class="gmail-m_8870913889991612175moz-txt-link-freetext" href="https://www.anvetsu.training" target="_blank">https://www.anvetsu.training</a>
-----------------------------------</pre>
</blockquote>
<p><br>
</p>
<pre class="gmail-m_8870913889991612175moz-signature" cols="72">--
Kind Regards,
--Anand
-----------------------------------
Founder & Director,
Anvetsu Technologies Pvt Ltd (OPC),
<a class="gmail-m_8870913889991612175moz-txt-link-freetext" href="https://www.anvetsu.com" target="_blank">https://www.anvetsu.com</a>
<a class="gmail-m_8870913889991612175moz-txt-link-freetext" href="https://www.anvetsu.training" target="_blank">https://www.anvetsu.training</a>
-----------------------------------</pre>
</div>
_______________________________________________<br>
tahoe-dev mailing list<br>
<a href="mailto:tahoe-dev@tahoe-lafs.org" target="_blank">tahoe-dev@tahoe-lafs.org</a><br>
<a href="https://tahoe-lafs.org/cgi-bin/mailman/listinfo/tahoe-dev" rel="noreferrer" target="_blank">https://tahoe-lafs.org/cgi-bin/mailman/listinfo/tahoe-dev</a><br>
</blockquote></div></div>