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

[Andrew Bartlett]

On Sun, 2017-01-29 at 00:28 -0500, Ian Stakenvicius wrote:
> On 28/01/17 12:25 PM, Alexander Bokovoy wrote:
> > 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.
> > 
> 
> 
> I've integrated the suggested changes and finished adjustments to the
> standalone modules.  I also squashed the patchset a bit more.  Please
> find attached the patchset for review and approval.

The only thing I found disquieting is that the whole of libnet and
torture are disabled.  However, I said at the outset of this patch set
that I didn't want to make Python a second class language in Samba:  If
a distributor or embedded vendor wants to cut out python, then major
features may well be missing, and everyone will just live with that.  

Therefore the patch is correct, and #ifdef in the libnet code would be
against that standard.  (It can be added later if we change our mind). 

On to the actual issues with the patch:

I can't get this to build when I set --disable-python.

I get "ADDC requires python" even if I set --without-ad-dc, sorry. 
Additionally the error string needs to be modified to be "--disable-
python requires --without-ad-dc".

The attached autobuild patch will keep this working, once we sort out
what is going on here.

> As promised, I've also rebased Petr's multi-python work on top of
> this
> patchset, available for review at
> https://github.com/axs-gentoo/samba/tree/disable-python-v2-with-pytho
> n3
> -- note that with this patchset there are only two conflicting lines
> between them that I had to compensate for.
> 
> (edit - re-sending without openpgp signature)

Thanks.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-autobuild-Add-nopython-environment-to-test-disable-p.patch
Type: text/x-patch
Size: 2972 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20170130/5ec50715/0001-autobuild-Add-nopython-environment-to-test-disable-p.bin>