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

[Ian Stakenvicius]

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?


>> +
>> +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.

-----

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?


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20170128/75dec2bd/signature.sig>