[PATCH 29] Re: Disabling Python Modules

[Ian Stakenvicius]

On 27/01/17 02:46 PM, Andrew Bartlett wrote:
> On Fri, 2017-01-27 at 11:36 -0500, Ian Stakenvicius wrote:
>> On 27/01/17 01:10 AM, Andrew Bartlett wrote:
>>> On Wed, 2017-01-25 at 23:56 -0500, Ian Stakenvicius wrote:
>>>> On 25/01/17 03:29 AM, Stefan Metzmacher wrote:
>>>>> Hi Ian,
>>>>>
>>>>>> I can't be certain, but I think that may indeed be the last
>>>>>> of
>>>>>> them.
>>>>>> All wscript_build files that contain modules with deps=
>>>>>> including
>>>>>> py*util, and LIBPYTHON, are now all covered.
>>>>>
>>>>> I know I'm late in the discussion, but is there a chance
>>>>> that we could avoid all changes like this:
>>>>>
>>>>> -bld.SAMBA3_PYTHON('pys3param',
>>>>> +if not bld.env.disable_python:
>>>>> +    bld.SAMBA3_PYTHON('pys3param',
>>>>>
>>>>> And just to that globally within SAMBA_PYTHON()
>>>>> instead of changing each caller?
>>>>
>>>>
>>>> I considered doing that, but decided against it due to the fact
>>>> that
>>>> this hides too much from the wscript_build of each module.  When
>>>> a
>>>> bld.env.disable_python conditional exists it makes it clear that
>>>> all
>>>> components the conditional applies to (whether SAMBA_PYTHON() or
>>>> not,
>>>> and there's more than a few that aren't SAMBA_PYTHON()).
>>>>
>>>>
>>>>
>>>>> [...]
>>>>>
>>>>> For all others we should use something like
>>>>> enabled=bld.PYTHON_BUILD_IS_ENABLED(),
>>>>> see git grep 'enabled=' for examples.
>>>>
>>>>
>>>> Is this just for style? or..
>>>
>>> No, it is more than style.  It allows the build system to know that
>>> the
>>>  name of the target subsystem exists, but it not in use.  This
>>> allows
>>> dependencies to be resolved as nothing, rather than having 'if
>>> python'
>>> in dep strings.  While this may just move the dep problem to the C
>>> layer, it allow an #ifdef at that layer.  
>>>
>>> It is also cleaner, in general, and while not totally consistent,
>>> it is
>>> how we have tried to handle other features.  
>>>
>>
>> Makes sense.  Is there a particular use of this form already in the
>> build system that you would recommend I mirror for the implementation
>> of PYTHON_BUILD_IS_ENABLED()?  Or does waf already provide that
>> simply
>> because the configure flag exists?
> 
> The normal way would be to check:
> 
> bld.CONFIG_SET('HAVE_PYTHON')
> 
> In your first patch the detection is a little odd.  It seems we still
> look for python even if disabled?
> 
>      conf.SAMBA_CHECK_PYTHON(mandatory=True, version=(2, 6, 0))
> -    conf.SAMBA_CHECK_PYTHON_HEADERS(mandatory=True)
> +    if conf.env.disable_python:
> +        conf.SAMBA_CHECK_PYTHON_HEADERS(mandatory=False)
> +    else:
> +        conf.SAMBA_CHECK_PYTHON_HEADERS(mandatory=True)
> 
> Shouldn't it be:
> 
> if not conf.env.disable_python: 
>   conf.SAMBA_CHECK_PYTHON(mandatory=True, version=(2, 6, 0))
>   conf.SAMBA_CHECK_PYTHON_HEADERS(mandatory=True)
> 
> or such?
> 


When originally building the patchset, I only wanted to strip out
enough of the python dependencies such that it would build without a
python available on the target.  I barely understood waf at the time
(and am still lacking in knowledge now to be honest) and so I wasn't
sure how much the build system itself needed the results of these checks.

In general, with what i've learned to date, I think leaving the checks
for the python runtime is likely still a good idea; this requires
there to be a python available on the build system but that is
necessary for waf anyhow.  It's the header checks, in particular, that
we need to disable in order to bypass a target that doesn't have them
available.




>> Also, does this mean that the various SAMBA_LIBRARY() modules I'm
>> turning off due to their dependency on, say, LIBPYTHON, will now also
>> need to be patched in C so that things won't fail?  Or will the
>> module
>> still be skipped by the build system since 'enabled=' is false?
> 
> It may be cleaner to handle some of it there, rather than a cascade of
> dependencies.  
> 
> See what still needs to be disabled once you do this, and once you no
> longer need to care about stuff already disabled by --without-samba-ad-
> dc
> 
> BTW I'm quite suspicious of
> 
> Subject: [PATCH 11/28] waf: disable-python - don't build libcli echo
> torture test
> 
> What does this have to do with python?
> 

That may be a red herring -- as part of patch #1 in this set, I
skipped recursion into source4/torture due to all of the various
python requirements inside; I believe this made smbtorture
unavailable, which is required for that particular item.

Version 2 of the patchset will need a from-scratch rewrite and so I
will be able to confirm whether or not there is any python depenency
at that point.


>>
>> SO, to accomplish what's described here:
>>
>> (A) what other features are python-only or are otherwise useless to
>> build when there isn't any python, so I can turn them all off at the
>> same time?
> 
> So far just the AD DC.
> 
>> (B) the action taken: to me --disable-python without any addc options
>> should just disable addc, while --disable-python and --with-addc
>> should error.  Does that make sense or should --disable-python always
>> error unless all of the dependent features are disabled?
> 
> --disable-python should error unless all of the dependent features are
> disabled.  While it looks like this will become a supported feature,
> the target is for folks already setting --without-ad-dc and I really
> want folks to realise what they are missing out on.
> 
> If we don't, I'm quite confident we will get a user saying 'I built
> with --disable-python because I don't like python, why can't I use the
> AD DC!' ;-)
> 

No problem.  I'll add this directly to the root wscript as this looks
to be where the ad-dc configure options are defined.

-------------- 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/20170127/854127b7/signature.sig>