[PATCHv2 1-14/14] Re: Disabling Python Modules

[Alexander Bokovoy]

On la, 28 tammi 2017, Ian Stakenvicius wrote:
> On 28/01/17 05:02 AM, Alexander Bokovoy wrote:
> > On la, 28 tammi 2017, Ian Stakenvicius wrote:
> >> diff --git a/buildtools/wafsamba/samba_python.py b/buildtools/wafsamba/samba_python.py
> >> index 057a017..b6bf079 100644
> >> --- a/buildtools/wafsamba/samba_python.py
> >> +++ b/buildtools/wafsamba/samba_python.py
> >> @@ -40,22 +40,31 @@ def SAMBA_CHECK_PYTHON(conf, mandatory=True, version=(2,4,2)):
> >>  
> >>  @conf
> >>  def SAMBA_CHECK_PYTHON_HEADERS(conf, mandatory=True):
> >> [...]
> > 
> > Could you please turn the logic upside down here?
> > Instead of 
> > 
> >    if not conf.env.disable_python:
> >     -> re-indent code
> >    else:
> >     ....
> > 
> > please do 
> > 
> >    if conf.env.disable_python:
> >       ....
> >       return
> > 
> >    original code
> > 
> > This would further reduce the diff size and will not touch the original
> > code at all.
> > 
> 
> Definitely.
> 
> 
> >>  
> >>      # we don't want PYTHONDIR in config.h, as otherwise changing
> >>      # --prefix causes a complete rebuild
> >> @@ -77,6 +86,14 @@ def _check_python_headers(conf, mandatory):
> >>          conf.env['PYTHON_SO_ABI_FLAG'] = ''
> >>  
> >>  
> >> +def PYTHON_BUILD_IS_ENABLED(self):
> >> +    if self.CONFIG_SET('HAVE_PYTHON_H'):
> >> +        return True
> >> +    return False
> > CONFIG_SET already returns True or False, so this could be simplified
> > down to
> > 
> >    def PYTHON_BUILD_IS_ENABLED(self):
> >        return self.CONFIG_SET('HAVE_PYTHON_H')
> > 
> > 
> 
> Will do.  Note that I found the code pattern I used a lot, so it might
> be worth doing a cleanup commit to change them all to your suggested
> format?
> 
Sounds like a good idea, thanks.


> >> +
> >> +Build.BuildContext.PYTHON_BUILD_IS_ENABLED = PYTHON_BUILD_IS_ENABLED
> >> +
> >> +
> >>  def SAMBA_PYTHON(bld, name,
> >>                   source='',
> >>                   deps='',
> >> @@ -91,6 +108,9 @@ def SAMBA_PYTHON(bld, name,
> >>                   enabled=True):
> >>      '''build a python extension for Samba'''
> >>  
> >> +    if not bld.PYTHON_BUILD_IS_ENABLED():
> >> +        enabled = False
> >> +
> > As PYTHON_BUILD_IS_ENABLED already returns True or False, this could be
> > written as
> > 
> >    enabled = bld.PYTHON_BUILD_IS_ENABLED()
> > 
> 
> 
> We would lose the passed-in value of 'enabled' if we overwrote it in
> that manner.  I could do:
> 
> 	enabled = (enabled and bld.PYTHON_BUILD_IS_ENABLED())
> 
> ..if that would be preferable?  I'm thinking the earlier form would be
> easier to understand when someone looks at it in a year, though.
Ok, you are right. However, please add a comment there why we are
forcing an override. In a year nobody will remember that.

> -----
> 
> So as-is, would this patchset be complete or should I also include the
> code to remove --disable-python from the wscript of standalone modules?
Please add patches for the standalone modules too.

-- 
/ Alexander Bokovoy