Hi,
On 12 July 2018 at 18:54, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Jul 12, 2018 at 04:10:41PM +0200, frenzy@xxxxxxxxx wrote:
> > From: Lumir Balhar <lbalhar@xxxxxxxxxx>
> >
> > Python 2 is still the default but it can be changed by
> > setting environment variable PYTHON before build/test.
> >
> > Signed-off-by: Lumir Balhar <lbalhar@xxxxxxxxxx>
>
> As I noted on an earlier version, I'd like to get Simon Glass's review
> before merging these, since the Python bindings are his work.
> However, a few preliminary comments from me.
>
> > ---
> > Makefile | 3 ++-
> > pylibfdt/Makefile.pylibfdt | 4 ++--
> > tests/Makefile.tests | 6 +++---
> > tests/run_tests.sh | 2 +-
> > 4 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 6d55e13..c84eb66 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -24,6 +24,7 @@ BISON = bison
> > LEX = flex
> > SWIG = swig
> > PKG_CONFIG ?= pkg-config
> > +PYTHON ?= python2
> >
> > INSTALL = /usr/bin/install
> > INSTALL_PROGRAM = $(INSTALL)
> > @@ -133,7 +134,7 @@ all: $(BIN) libfdt
> > # We need both Python and swig to build/install pylibfdt.
> > # This builds the given make ${target} if those deps are found.
> > check_python_deps = \
> > - if $(PKG_CONFIG) --cflags python2 >/dev/null 2>&1; then \
> > + if $(PKG_CONFIG) --cflags $(PYTHON) >/dev/null 2>&1; then \
> > if which swig >/dev/null 2>&1; then \
> > can_build=yes; \
> > fi; \
> > diff --git a/pylibfdt/Makefile.pylibfdt b/pylibfdt/Makefile.pylibfdt
> > index 9507d3d..a05984c 100644
> > --- a/pylibfdt/Makefile.pylibfdt
> > +++ b/pylibfdt/Makefile.pylibfdt
> > @@ -8,13 +8,13 @@ PYMODULE = $(PYLIBFDT_objdir)/_libfdt.so
> > define run_setup
> > SOURCES="$(1)" CPPFLAGS="$(CPPFLAGS)" OBJDIR="$(PYLIBFDT_objdir)"
> > VERSION="$(dtc_version)"
> > - $(PYLIBFDT_objdir)/setup.py --quiet $(2)
> > + $(PYTHON) $(PYLIBFDT_objdir)/setup.py --quiet $(2)
> > endef
>
> Using the environment variable to select the default python for
> install is fine, but..
>
> > $(PYMODULE): $(PYLIBFDT_srcs)
> > @$(VECHO) PYMOD $@
> > $(call run_setup, $^, build_ext --inplace)
> > - mv _libfdt.so $@
> > + mv _libfdt.*so $@
> >
> > install_pylibfdt: $(PYMODULE)
> > $(VECHO) INSTALL-PYLIB; \
> > diff --git a/tests/Makefile.tests b/tests/Makefile.tests
> > index 2c2c4fd..554c840 100644
> > --- a/tests/Makefile.tests
> > +++ b/tests/Makefile.tests
> > @@ -76,13 +76,13 @@ tests_clean:
> > rm -f $(TESTS_CLEANFILES)
> >
> > check: tests ${TESTS_BIN} $(TESTS_PYLIBFDT)
> > - cd $(TESTS_PREFIX); ./run_tests.sh
> > + cd $(TESTS_PREFIX); PYTHON=$(PYTHON) ./run_tests.sh
> >
> > checkm: tests ${TESTS_BIN} $(TESTS_PYLIBFDT)
> > - cd $(TESTS_PREFIX); ./run_tests.sh -m 2>&1 | tee vglog.$$$$
> > + cd $(TESTS_PREFIX); PYTHON=$(PYTHON) ./run_tests.sh -m 2>&1 | tee vglog.$$$$
>
> ..for testing, I'd be much happied if we ran the pylibfdt tests under
> *both* Python2 and Python3 if available. Makes it much likelier that
> we'll catch breakages for one or the other early.
I definitely agree with that, it is pretty important.
Other than that it looks fine.
Regards,
Simon
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
