third_party (ex-lib_3p) now ready for review.

[Amitay Isaacs]

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.

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

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.

- 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')

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?

Amitay.