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

[Andrew Bartlett]

On Mon, 2017-01-30 at 14:11 -0500, Ian Stakenvicius wrote:
> On 30/01/17 01:44 PM, Andrew Bartlett wrote:
> > We seem to have moved off-list.  Can we move this back on-list?
> 
> Done.
> 
> 
> > On Mon, 2017-01-30 at 10:06 -0500, Ian Stakenvicius wrote:
> > > On 29/01/17 10:56 PM, Andrew Bartlett wrote:
> > > > 
> > > > With:
> > > > 
> > > > ./configure.developer  --picky-developer --
> > > > prefix=/tmp/b9407/prefix/samba-nopython  --with-profiling-
> > > > data  
> > > > --without-ad-dc --disable-python && make -j
> > > > 
> > > > I now get:
> > > > 
> > > > In file included from ../lib/replace/replace.h:32:0,
> > > >                  from ../dynconfig/dynconfig.c:41:
> > > > default/include/config.h:431:0: error: "PYTHONDIR" redefined [-
> > > > Werror]
> > > >  #define PYTHONDIR "/tmp/b9407/prefix/samba-
> > > > nopython/lib/python2.7/site-packages"
> > > >  
> > > > <command-line>:0:0: note: this is the location of the previous
> > > > definition
> > > > In file included from ../lib/replace/replace.h:32:0,
> > > >                  from ../dynconfig/dynconfig.c:41:
> > > > default/include/config.h:432:0: error: "PYTHONARCHDIR"
> > > > redefined [-
> > > > Werror]
> > > >  #define PYTHONARCHDIR "/tmp/b9407/prefix/samba-
> > > > nopython/lib64/python2.7/site-packages"
> > > >  
> > > 
> > > 
> > > RIGHT, now I recall why I had coded the
> > > SAMBA_CHECK_PYTHON_HEADERS()
> > > the way I did originally, it was so that deleting the defines
> > > would
> > > still happen regardless of whether the check was skipped.  The
> > > following should work, alternatively I can put it back to the way
> > > it
> > > was with most of the function wrapped by the conditional..
> > > 
> > > --- a/buildtools/wafsamba/samba_python.py
> > > +++ b/buildtools/wafsamba/samba_python.py
> > > @@ -47,6 +47,10 @@ def SAMBA_CHECK_PYTHON_HEADERS(conf,
> > > mandatory=True):
> > >                                   "--disable-python specified")
> > > 
> > >          conf.msg("python headers", "Check disabled due to
> > > --disable-python")
> > > +        # we don't want PYTHONDIR in config.h, as otherwise
> > > changing
> > > +        # --prefix causes a complete rebuild
> > > +        del(conf.env.defines['PYTHONDIR'])
> > > +        del(conf.env.defines['PYTHONARCHDIR'])
> > >          return
> > > 
> > >      if conf.env["python_headers_checked"] == []:
> > > 
> > 
> > My preference would be that the call is:
> > 
> > if not conf.env.disable_python:
> >         conf.SAMBA_CHECK_PYTHON_HEADERS(mandatory=True)
> > 
> > I didn't make a bother about it before as I really didn't want to
> > bikeshed you to death, but I don't think we should call
> > SAMBA_CHECK_PYTHON_HEADERS if we disable python.  
> > 
> > I'm OK with checking for the python interpreter: it might be handy,
> > or
> > it might just bite us later.  The main hope is that we bail early
> > before hitting future python 2.7 syntax in the remainder of the
> > build
> > system.
> > 
> 
> I don't think that would fix this particular issue, in fact I think
> avoiding the call to SAMBA_CHECK_PYTHON_HEADERS would cause the
> dynconfig problem to occur more rather than less.  The original
> version of my patch ensured that those calls to
> del(conf.env.defines['PYTHON*]) always occurred regardless of whether
> the header check(s) were being skipped, the issue you ran into was
> when they don't occur, which would happen if
> SAMBA_CHECK_PYTHON_HEADERS isn't called at all.
> 
> 
> > > Travis is still running on a PR with the above change, with luck
> > > it'll
> > > pass on everything except for samba-nopython; that one's already
> > > failed due to:
> > > 
> > > > [2770/3250] Compiling testsuite/headers/test_headers.c
> > > > 
> > > > ../testsuite/headers/test_headers.c:26:20: fatal error:
> > > > Python.h:
> > > > No such file or directory
> > > > 
> > > >  #include <Python.h>
> > > > 
> > > >                     ^
> > > > 
> > > > compilation terminated.
> > > > 
> > > > Waf: Leaving directory `/tmp/b10412/samba-nopython/bin'
> > > > 
> > > > Build failed:  -> task failed (err #1): 
> > > > 
> > > > 	{task: cc test_headers.c -> test_headers_2.o}
> > > > 
> > > > make: *** [all] Error 1
> > > > 
> > > 
> > > ...which makes sense given the code.  However, I'm not sure if
> > > this
> > > should be skipped in the build system or if the include line
> > > above
> > > should just be wrapped with #ifdef HAVE_PYTHON_H  ..?  
> > 
> > I think an #ifdef is appropriate here.
> > 
> 
> *nod*, tested already and it works, travis reports samba-nopython
> succeeds:
> 
> https://github.com/samba-team/samba/pull/74

I've looked over the diff with the previous patches, and it looks good!

Can someone else on the team have a look at this and give review if OK?

Reviewed-by: Andrew Bartlett <abartlet at samba.org>

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