third_party (ex-lib_3p) now ready for review.

Ira Cooper ira at samba.org
Wed Jul 23 10:20:31 MDT 2014


On Wed, Jul 23, 2014 at 6:52 AM, Jelmer Vernooij <jelmer at samba.org> wrote:

> On Wed, Jul 23, 2014 at 01:22:46AM -0700, Ira Cooper 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
>
> Comments, in random order:
>
> Yay for splitting these out!
>
> How are we going to keep the copy of third_party on the build farm up
> to date?
>

clone and pull the third_party repo is the easiest way.


> The commit message says "remove all third party libraries from the tree",
> but
> there are more.. Please either remove them all, or update the commit
> message
> to mention just those that you are removing. :) I'd propose the latter,
> we don't have to do them all at once.
>
> (lib/{ccan, pep8, testtools, subunit, dnspython} are among the others)
>

We should get a full list of them together.  I doubt we'll nuke them all in
one pass, but over a few... we'll get them.

Checks for specific libraries don't belong under buildtools/wafsamba/,
> which is our set of support libraries on top of waf. Please add
> these checks in wscript or wscript_third_party or something
> like that.


I wasn't a fan of it either... but adding the changes to wscript is just
ugly and stops us from reusing them properly.

I am open to better ways, but the way I did it before seems notably worse,
also note: I call parts of this in the build, and in ldb's wscript.  So, it
is a bit more "meta".

-Ira


More information about the samba-technical mailing list