Re: [PATCH v3 1/4] pylibfdt: allow build out of tree

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4560 bytes --]

On Tue, Sep 29, 2020 at 07:37:22PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Sep 29, 2020 at 6:42 PM David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Tue, Sep 29, 2020 at 03:46:16PM +0400, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Mon, Sep 28, 2020 at 12:03 PM David Gibson
> > > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > >
> > > > On Mon, Sep 21, 2020 at 12:22:06PM +0400, Marc-André Lureau wrote:
> > > > > Hi
> > > > >
> > > > > On Mon, Sep 21, 2020 at 10:29 AM David Gibson
> > > > > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > > > >
> > > > > > On Tue, Sep 15, 2020 at 11:27:02PM +0400, marcandre.lureau@redhat.com wrote:
> > > > > > > From: Marc-André Lureau <marcandre.lureau-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > > > > >
> > > > > > > With meson, we have to support out-of-tree build. Fix path lookup.
> > > > > > >
> > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > ---
> > > > > > >  pylibfdt/Makefile.pylibfdt |  2 +-
> > > > > > >  pylibfdt/setup.py          | 25 +++++++++++++++++--------
> > > > > > >  2 files changed, 18 insertions(+), 9 deletions(-)
> > > > > > >
> > > > > > > diff --git a/pylibfdt/Makefile.pylibfdt b/pylibfdt/Makefile.pylibfdt
> > > > > > > index 6866a0b..32ae1c5 100644
> > > > > > > --- a/pylibfdt/Makefile.pylibfdt
> > > > > > > +++ b/pylibfdt/Makefile.pylibfdt
> > > > > > > @@ -10,7 +10,7 @@ PYLIBFDT_CLEANDIRS_L = build __pycache__
> > > > > > >  PYLIBFDT_CLEANDIRS = $(PYLIBFDT_CLEANDIRS_L:%=$(PYLIBFDT_dir)/%)
> > > > > > >
> > > > > > >  SETUP = $(PYLIBFDT_dir)/setup.py
> > > > > > > -SETUPFLAGS =
> > > > > > > +SETUPFLAGS = --top-builddir .
> > > > > > >
> > > > > > >  ifndef V
> > > > > > >  SETUPFLAGS += --quiet
> > > > > > > diff --git a/pylibfdt/setup.py b/pylibfdt/setup.py
> > > > > > > index 53f2bef..f8ec924 100755
> > > > > > > --- a/pylibfdt/setup.py
> > > > > > > +++ b/pylibfdt/setup.py
> > > > > > > @@ -19,23 +19,31 @@ import sys
> > > > > > >  VERSION_PATTERN = '^#define DTC_VERSION "DTC ([^"]*)"$'
> > > > > > >
> > > > > > >
> > > > > > > +def get_top_builddir():
> > > > > > > +    assert '--top-builddir' in sys.argv
> > > > > >
> > > > > > I see you've added the option to the Makefile, but if this were
> > > > > > invoked manually, I'm not sure that you want to just die if the
> > > > > > --top-builddir option isn't included.
> > > > > >
> > > > > > Even if you do, I think you want a more meaningful error than an
> > > > > > assertion failure.
> > > > >
> > > > > The assertion is quite explicit:
> > > > >
> > > > > Traceback (most recent call last):
> > > > >   File "pylibfdt/setup.py", line 30, in <module>
> > > > >     top_builddir = get_top_builddir()
> > > > >   File "pylibfdt/setup.py", line 23, in get_top_builddir
> > > > >     assert '--top-builddir' in sys.argv
> > > > > AssertionError
> > > > >
> > > > > Having extra arguments to setuptools is tricky:
> > > > > https://stackoverflow.com/questions/677577/distutils-how-to-pass-a-user-defined-parameter-to-setup-py
> > > > >
> > > > > Since it's a programmer error, that you are not suppose to run into
> > > > > because you'd use make or ninja, it seems enough to me.
> > > >
> > > > Hm.  Invoking this only via make or ninja would be normal for someone
> > > > building dtc as a unit.  But, I'm not so familiar with the Python
> > > > ecosystem, so I wonder if someone specifically looking at the Python
> > > > bindings would expect to be able to invoke setup.py directly.
> > >
> > > Maybe, so if the error message about missing '--top-builddir' is not
> > > explicit enough, I can try to make the error a bit more human
> > > friendly.
> > >
> > > But as you can see from the stackoverflow question, it's not uncommon
> > > to want to have extra arguments to setup.py, and there are various
> > > solutions offered for that.
> >
> > It's not the fact it's an extra argument that concerns me, only the
> > fact that it is a *required* extra argument.
> 
> I can try to make it default to the current directory, but it will
> probably have to handle errors elsewhere then in more archaic ways.

Have a look, please.  If it proves really hard, I'll reconsider.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]