On Wed, Jul 09, 2014 at 08:43:27AM -0400, Bohuslav Kabrda wrote:
----- Original Message -----
> On Tue, Jul 08, 2014 at 10:38:00AM -0400, Bohuslav Kabrda wrote:
> > Some comments:
> > - The patch tries to maintain backward compatibility for Python 2.6 and 2.7
> > - I admit I only tested 2.7 so far, but it seemed to work ok.
>
> The oldest OS release we support is RHEL-6. I haven't checked what
> Ubuntu uses, but I would expect at least 2.7
Yeah, RHEL 6 has 2.6, so we should be fine there. I'll do the testing as soon as I
find some time for it :)
> > - I didn't yet solve the build part - for now, I just handedit
> > configure.ac to set "PYTHON=python3" and src/external/python.m4 to
set
> > "AC_PATH_PROG(PYTHON, python3)" and compile with that.
>
> This is fine for testing, but do you have some experience from other
> projects on the best approach? I think we can help with coding up the
> build system part, I'm just not sure what the right approach is..
So, there are basically two different approaches to this and I don't think any of
them is "the best". I do admit I prefer the second one though:
1) configure can be modified to accept an argument for --with-python, e.g. user would be
able to use "--with-python=python2" or "--with-python=python3"; you
have to do two builds if you want to build for two Python releases
2) configure can be modified to accept a "--with-python3" option in addition to
"--with-python"
One more question -- why do you see as preferable to select the python
version we are building for? With your patch, the code would run equally
well with /usr/bin/python pointing to python2 and python3, right? So can
we just use whatever /usr/bin/python points to and be happy? Why should
the maintainer select one (or even two) python releases?
(libreport is doing something that is closer to 2), although they
strictly require both Python major versions and don't build if one is not present [3].
I don't think we should require both python versions.
> > - I managed to run Python tests in src/tests except python-test.py itself
> > (it says "OSError: [Errno 5] Could not initialize connection to the
> > confdb" and I didn't yet investigate why that happens).
>
> Yeah, don't worry about that one, it requires root currently because the
> files are owned by root. I wonder if we should remove that test and the
> python sysdb API altogether, I don't think anyone ever uses it.
Ok, thanks for the info.
> > - To run tests, one must modify the hashbangs to /usr/bin/python3 (I now
> > see that I left one of these in the actual patch, so please disregard
> > that).
>
> Maybe I miss something here, but if the changes are backwards
> compatible, why does it matter to use python3 explicitly?
So, the way I think this should be done is that you run tests for all versions for Python
version(s) that you're currently building for. The tests have explicit hashbang that
refers to /usr/bin/python2, so you have to modify that when running tests for Python 3.
The way I think this should be done systematically is, that you should invoke tests using
the Python interpreter executable found by configure - for example, invoke
"/usr/bin/python2 src/test/foo-test.py" rather than just
"foo-test.py".
> >
> > I'd like to ask you, sssd devels, to give me some comments on the patch
(or
> > ask questions) and if you have some time to spare, doing more extensive
> > testing would also be very welcome.
> >
> > --
> > Regards,
> > Bohuslav "Slavek" Kabrda.
> >
> > [1]
> >
https://fedorahosted.org/sssd/attachment/ticket/2017/sssd-python3-compat....
> > [2]
https://fedorahosted.org/sssd/ticket/2017
[3]
https://github.com/abrt/libreport/commit/6ee995deaf7195c024d77a9ba44509b2...
Thanks for the feedback,
Slavek