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

[Andrew Bartlett]

On Wed, 2017-02-01 at 07:40 +1300, Andrew Bartlett wrote:
> 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>

I got a review from Garming and this is waiting for autobuild now.

Thanks!

Andrew Bartlett