Troy Curtis Jr wrote on Wed, Oct 18, 2017 at 03:49:57 +0000:
> > Thanks for posting that. Is that patch ready for commit, or is it
> > intended just as a proof of concept / basis for discussions?
> >
>
> Definitely not ready for commit, it is the WIP of what I was hacking on
> Sunday. I was just getting a feel for the kinds of things involved, and
> attached it to give you a sense of what I was talking about. I even have a
> FIXME note in there where I *think* there may be a potential use after free
> issue. We may be getting away with it because of the delay of the python
> garbage collector, but I plan on taking a harder look at that one before
> reaching a conclusion.
Thanks, that'd be great. To be clear, as a preexisting issue, I would
not consider it a blocker to accepting the py3 compatibility patch; I
would consider the two issues independent.
> > The questions to decide on are:
> > > 1. Are you generally comfortable with the build changes necessary to
> > > (optionally) build Py2 and Py3 bindings at the same time?
> >
> > Do you mean that a single 'make install' would install two versions of
> > libsvn_swig_py.so, one compiled against CPython 2 and one compiled
> > against CPython 3?
> >
> > Or do you mean, having a (say) configure-time knob that controls whether
> > bindings would be built against py2 or against py3?
> >
> > I think the latter would be uncontroversial. As to the former, it would
> > involve more moving pieces, so I would be concerned about maintenance
> > overhead of the new build system code. (We have few active developers
> > nowadays.)
> >
> >
> Right, I mean something like having "--enable-swig-py2" and
> "--enable-swig-py3" so that Py2 and Py3 could be enabled/disabled
> independently, with the option of both being built in the same build. I'll
> admit it is specifically with a mind towards packaging, since many (all?)
> distros include bindings for both versions if possible. I knew that might
> be a stretch, which is why I asked ;) I'll plan on implementing the
> straight forward either-or method first, and putting the dual-compile
> option in a separate commit that can be dropped if desired.
Thanks. I agree it would be nice to be able to build both py2 and py3
in one build.
Re "droppable patch", as you did for this thread, if you find that that
patch would take much time to write then do seek consensus on that
patch's acceptability on dev@ before investing that time: no one here
would want you to spend time writing a patch that wouldn't be
incorporated.
> Oh and I won't forget about the ctypes implementation either.
We generally require feature parity between the RA layers and the
non-{deprecated,experimental} FS layers, but there is no such lockstep
policy for swig-py and ctypes: there is no requirement that patches to
one be accompanied by patches to the other. Now that I got that
clarification out of the way, I can answer: Thanks! We'd welcome
patches to ctypes.
> > > 2. Do you want to pick up 'py3c' as a new dep, or implement the handful
> > of
> > > necessary wrappers?
> >
> > I don't know the swig-py bindings well enough to answer that. I guess
> > it depends on how much a "handful" would be.
> >
> > Datapoints: We use 73 distinct CPython API symbols (see attached grep
> > results). Of these, only three(?) entry points (all of them PyInt_*)
> > appear in py3c/compat.h.
> >
> >
> I'll default to not pulling it in, I don't think it is really needed. But
> I try to default to resisting the "Not Invented Here" tendency. However,
> adding a new dep, especially for so little gain, probably doesn't make
> sense IMHO. The converse is that while some of the calls might not be
> currently used, later additions might have to add more and more compat
> functions, approaching what is provided in py3c.
>
> Note also the PyString_* variants would go to PyStr_* in py3c (or
> svn_swig_py_*_string*() like in my WIP patch), as well as the PyFile_*
> calls.
I think we are conducting a balanced and objective decision process
regarding whether to include py3c or to crib or reimplement it. If that
process results in a decision to crib or reimplement, that'll be okay:
NIH is not an uncontravenable axiom. Our due diligence to NIH is to
conduct this decision process, not to ensure that this decision process
makes a NIH-compatible decision.
> > > 3. Is the assumption of utf8 encoding sufficiently reasonable?
> >
> > In Subversion, we assume that argv and stdout are encoded in APR's
> > "system encoding", but basically everything else — including nearly
> > every single string parameter to every public API — is required to be in
> > UTF-8. svn_cmdline.h contains some of the exceptions.
> >
> > I'm not sure whether that information answers your question, though. In
> > what concrete cases do the bindings have to convert between str and
> > bytes? What are the compatibility considerations for user code
> > (consumers of the swig-py2 bindings that want to upgrade to py3)?
> >
> >
> It answers it well enough. The implicit assumption of string handling in
> py2 is one of the things py3 set out to address. Since currently py2 are
> really making assumptions already, going from 2->3 using utf8 should be
> reasonable IMHO.
I don't follow how you reach the conclusion that using utf8 should be
reasonable. Could you trace your steps?
Our goal, presumably, is for py3's "import svn" to be as much of a
drop-in replacement to py2's "import svn" as possible. In what cases
does py2 code pass py2 str objects to libsvn_swig_py? What happens to
those objects in py3? The whole question of default encoding only
matters if what in py2 was a str object is under py3 a bytes object.
Maybe we should require py3 user code to pass a str object, mooting the
question entirely?
> > > My general plan of attack will be:
> > > 1. Replace deprecated Py2 swig functions and syntax with Py2/Py3
> > > cross-compatible versions or wrappers.
> > > 2. Ensure existing full Py2 support works correctly.
> > > 3. Update build to build both, Py2 and Py3, and get Py3 working.
> > > 4. Look at the remainder of the python usage to ensure Py3 compliance.
> >
> > At some point we'll want to switch one of the buildbot builders to
> > use python3. (See http://subversion.apache.org/buildbot/all)
> >
> > Could you explain how you — and we — test patches written in step #1,
> > given that "update the build to support py3" only comes later, in #3?
> > Is it currently possible to build the bindings against py3 (well, to
> > _run_ the build against py3 and get compiler errors about missing
> > symbols)?
> >
> >
> My main concern on step #1 is the "not breaking existing py2 bindings",
> laying the ground work for the py3 build changes. I'll probably send all
> the patches at the same time, but if they can stand alone, hopefully it'll
> make review easier.
I'd expect "Not breaking swig-py2" to be an invariant regardless of
which order swig-py3's development proceeded in.
> > Thanks for the very detailed email!
> >
> >
> Err, yeah. My '--verbose' can't really be disabled... :P
Heheh :-)
Daniel
Received on 2017-10-19 05:11:17 CEST