[PATCHv2 1/x] Re: Disabling Python Modules

Andrew Bartlett abartlet at samba.org
Sat Jan 28 01:21:54 UTC 2017


On Fri, 2017-01-27 at 15:24 -0500, Ian Stakenvicius wrote:
> On 27/01/17 03:02 PM, Andrew Bartlett wrote:
> > On Fri, 2017-01-27 at 14:21 -0500, Ian Stakenvicius wrote:
> > > On 27/01/17 11:36 AM, Ian Stakenvicius wrote:
> > > > On 27/01/17 01:10 AM, Andrew Bartlett wrote:
> > > > > On Wed, 2017-01-25 at 23:56 -0500, Ian Stakenvicius wrote:
> > > > > > > 
> > > > > > > For all others we should use something like
> > > > > > > enabled=bld.PYTHON_BUILD_IS_ENABLED(),
> > > > > > > see git grep 'enabled=' for examples.
> > > > > > 
> > > > > > 
> > > > > > Is this just for style? or..
> > > > > 
> > > > > No, it is more than style.  It allows the build system to
> > > > > know
> > > > > that the
> > > > >  name of the target subsystem exists, but it not in
> > > > > use.  This
> > > > > allows
> > > > > dependencies to be resolved as nothing, rather than having
> > > > > 'if
> > > > > python'
> > > > > in dep strings.  While this may just move the dep problem to
> > > > > the
> > > > > C
> > > > > layer, it allow an #ifdef at that layer.  
> > > > > 
> > > > > It is also cleaner, in general, and while not totally
> > > > > consistent,
> > > > > it is
> > > > > how we have tried to handle other features.  
> > > > > 
> > > 
> > > OK, let me know if this is a bit more on track.
> > > 
> > > The following patch adds the --disable-python option globally.
> > > 
> > > It also adds some code to skip meat in SAMBA_PYTHON_CHECK_HEADERS
> > > ,
> > > so
> > > that even if python is available the HAVE_PYTHON_H will not be
> > > defined
> > > (an appropriate exception is raised if this check is mandatory).
> > > 
> > > It also adds bld.PYTHON_BUILD_IS_ENABLED() which uses
> > > HAVE_PYTHON_H
> > > to
> > > determine its value.
> > > 
> > > My thought here is that, since the codebase has the HAVE_PYTHON_H
> > > define, there's no need to make a PYTHON_BUILD_IS_ENABLED define
> > > to
> > > essentially do the same thing.  Also, the check sets
> > > 'disable_python'
> > > in the env so that the subprojects do not need to update their
> > > checks
> > > and logic to some new variable.
> > > 
> > > If this looks good, I'll adjust the rest of the patchset using
> > > this
> > > as
> > > the starting point.
> > 
> > This looks pretty good.
> > 
> > Does it still work if you change:
> > 
> > @@ -89,8 +106,9 @@ def SAMBA_PYTHON(bld, name,
> >                   vars=None,
> >                   install=True,
> >                   enabled=True):
> > -    '''build a python extension for Samba'''
> > +  '''build a python extension for Samba'''
> >  
> > +  if bld.PYTHON_BUILD_IS_ENABLED():
> >      if bld.env['IS_EXTRA_PYTHON']:
> >          name = 'extra-' + name
> > 
> > To instead set enabled=False?  That would still declare the
> > (disabled)
> > build object to waf.  Either way, this should not be hard to tweak
> > later. 
> > 
> 
> I will check and adjust the patch if it works, thanks!
> 
> ---
> 
> So, one thing that I just discovered with this method, is that the
> existing wscript for lib/talloc happens to work the same way more or
> less that my whole original patchset did, in that if python is
> disabled then the entirety of pytalloc-util is skipped.
> 
> I attempted to change its structure back so that the SAMBA_LIBRARY
> and
> SAMBA_PYTHONs are skipped via enabled=, but this line is still
> causing
> problems:
> 
> > name = bld.pyembed_libname('pytalloc-util')
> 
> 
> ...resulting in:
> 
> 
> >   File "./lib/talloc/wscript", line 128, in build
> >     name = bld.pyembed_libname('pytalloc-util')
> >   File "./buildtools/wafsamba/samba_python.py", line 159, in
> > pyembed_libname
> >     return name + bld.env['PYTHON_SO_ABI_FLAG']
> > TypeError: cannot concatenate 'str' and 'list' objects
> > 
> > Makefile:8: recipe for target 'all' failed
> > make: *** [all] Error 1
> 
> 
> Any thoughts on how to get around this one?

Waf is strange.  When something under bld.env is undefined, it returns
[], not "".  We just need to set conf.env['PYTHON_SO_ABI_FLAG'] = '' or
cope with the [].  We don't need that when we don't have python, but it
might keep everything more happy.  

If all this doesn't work out, then a backup plan could be to make
making gen_python_environments() return an empty list if python is
disabled?  The complex yeild stuff can be skipped, and just have it
return [] making:
 
       for env in bld.gen_python_environments(['PKGCONFIGDIR']):

behave just like the
 if !bld.python_disabled:
in your original patch

That could make it all fall our naturally, at least once the multi-
python stuff is finished, and the enabled stuff might be enough in the
meantime.  I know this is a bit of a contradiction with the previous
requirement - I think we are all trying to feel our way to making this
all as natural as possible. 

Sorry for the total confusion here,

Andrew Bartlett
-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba




More information about the samba-technical mailing list