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

[Ian Stakenvicius]

> On Feb 9, 2017, at 1:45 PM, Andrew Bartlett <abartlet at samba.org> wrote:
> 
>> On Thu, 2017-02-09 at 16:20 +1300, Andrew Bartlett wrote:
>>> 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.
> 
> Sadly this sill doesn't pass autobuild, this time on ctdb.   I'll add
> it to my stack to look at today, but if I don't get back to you can you
> please run:
> 
> ./script/autobuild.py ctdb --testbase=/tmp
> 
> locally.  Sadly ctdb is not set up on travis as it exceeded the
> resource limits or was flapping (I can't remember which). 
> 

I was relying on Travis for auto build results so if ctdb was disabled there it makes sense I missed it.  I will give it a try locally and make any adjustments as needed.