third_party (ex-lib_3p) now ready for review.

[Ira Cooper]

On Wed, Jul 23, 2014 at 5:55 AM, Amitay Isaacs <amitay at gmail.com> wrote:

> On Wed, Jul 23, 2014 at 6:22 PM, Ira Cooper <ira at samba.org> wrote:
>
>> The functionality of the patch as far as a "user" is concerned is much the
>> same.  But it has been refactored underneath so it has less code
>> duplication, and it is prepared for the day when we want to change the
>> name
>> of the third_party directory.  (While I haven't plumbed it into configure,
>> one could.)
>>
>> The patchset itself has been broken out into smaller more understandable
>> patches.
>>
>> If you have comments on how to make it better, I'm all ears.
>>
>> The main thing, you will find me strongly opposed to changing is the last
>> commit, removing the libraries, hopefully reading the popt commit will
>> help
>> you understand why, along with all the discussions along the way.
>>
>> I encourage you to see this as a "final draft".  (Or RC1.)  I do think it
>> is much more ready to go than when I started, thank you for the review
>> comments!  They truly did prove useful.
>>
>> Trees for review:
>>
>> Samba:
>> http://git.samba.org/?p=ira/wip.git;a=shortlog;h=refs/heads/third_party
>>
>> Third party git tree:
>> http://git.samba.org/?p=ira/third_party.git;a=summary
>>
>> Thanks,
>>
>> -Ira
>>
>
> Hi Ira,
>
> Looking at the patches, I am assuming third_party would be dropped at the
> top-level.
>

Yes.


> In that case, it would not build any of the subdir builds (e.g. lib/ldb).
> And you forgot CTDB. ;-)
>

That can be fixed on the next pass.  Thanks :).


> LOCATION_OF_THIRD_PARTY has be relative to the srcdir, possibly something
> like Utils.g_module.srcdir + '/third_party'.  Or better still would be use
> mechanism similar to DIST_DIRS to find the correct location of a component.
>
> For DIST_DIRS, instead of trying to construct the path using
> LOCATION_OF_THIRD_PARTY, may be we can just leave it as
> third_party/popt:third_party/popt.  For building tarballs, you would need
> to drop third_party libraries in the correct place anyway.
>

If others are ok with that.  I am also.


> - Patch: third_party/popt: Initial support for popt.
>
> In this patch, lib/ldb/wscript needs to include Utils to raise WafError
> exception.
>
> You missed one substitution of LOCATION_OF_THIRD_PARTY in lib/ldb/wscript.
>
>     -    bld.RECURSE('lib/popt')
>     +
>     +    if os.path.exists(srcdir+'/lib_3p'):
>     +        bld.RECURSE('lib_3p/popt')
>

I'll fix this :).  Oops.


>
> Also, instead of doing the checks in wscript, all the checking can be done
> in samba_third_party.py.
>
>     -    conf.RECURSE('lib/popt')
>     +
>     +    if conf.CHECK_FOR_THIRD_PARTY():
>     +        conf.RECURSE(conf.LOCATION_OF_THIRD_PARTY() + '/popt')
>     +    else:
>     +        if not conf.CHECK_POPT():
>     +            raise Utils.WafError('popt development packages have not
> been found.\nIf third_party is installed, check that it is in the proper
> place.')
>     +        else:
>     +            conf.define('USING_SYSTEM_POPT', 1)
>
> How about macros similar to conf.RECURSE, say conf.THIRD_PARTY('popt') and
> bld.THIRD_PARTY('popt') ?  Thinking about this a little more, it appears to
> be harder because we need to include the check for system library if
> third_party is missing.  May be we can just add macros per third_party
> library (e.g. conf.THIRD_PARTY_POPT and bld.THIRD_PARTY_POPT) and keep all
> the details under the implementation?
>

I'm not against this... but I'd like a bit more of people's thoughts on
what they want it to look like before we go ahead, and make the changes.
It isn't hard.. it is more I'd like to be sure I get it right.  The sharing
of this info between the ctdb, ldb and main builds makes it a bit more
complex.

The DIST_DIRS issue is one that bothers me, because I want to be able to
have this directory not be there, and yet have everything work.

-Ira