Hi Lumir, On Mon, 18 Feb 2019 at 04:13, Lumir Balhar <lbalhar@xxxxxxxxxx> wrote: > > Hello. > > Could please anybody take a look on these patches? Should I change > anything? From my point of view, these patches are finished and ready > for merge since November. > > I did a rebase and I am attaching a fresh copy of my patches. Is it possible to send these as patches (using patman or git send-email) instead of attachments? Regards, Simon > > Thank you. > > Lumír > > On 12/11/18 9:35 AM, Lumir Balhar wrote: > > On 12/3/18 8:33 AM, David Gibson wrote: > >> On Mon, Nov 26, 2018 at 02:56:04PM +0100, Lumir Balhar wrote: > >>> On 11/25/18 11:27 AM, David Gibson wrote: > >>>> On Tue, Nov 20, 2018 at 01:13:48PM +0100, Lumir Balhar wrote: > >>>>> Hello. > >>>>> > >>>>> Could we please finish this? > >>>> Yes, sorry. I did some preliminary changes aiming towards > >>>> simultaneous testing of Python2 and Python3, but then got side tracked > >>>> before completing it. > >>> No worries. > >>>>> I did a rebase of all my patches to the top of current master > >>>>> branch. My py3 > >>>>> branch is available here: https://github.com/frenzymadness/dtc/ > >>>>> and all > >>>>> patches are attached. > >>>> Thanks. > >>>> > >>>>> When I test it with Python 2 or Python 3, everything works. Python > >>>>> version > >>>>> has to be specified by environment variable PYTHON in order to > >>>>> change the > >>>>> default (python2). > >>>> > >>>>> $ make check > >>>>> DEP tests/dumptrees.c > >>>>> DEP tests/trees.S > >>>>> DEP tests/testutils.c > >>>>> DEP tests/value-labels.c > >>>>> … > >>>>> Basic sanity check for the FdtRo class ... ok > >>>>> testCreate (__main__.PyLibfdtSwTests) ... ok > >>>>> > >>>>> ---------------------------------------------------------------------- > >>>>> > >>>>> Ran 35 tests in 0.004s > >>>>> > >>>>> OK > >>>>> ********** TEST SUMMARY > >>>>> * Total testcases: 1826 > >>>>> * PASS: 1826 > >>>>> * FAIL: 0 > >>>>> * Bad configuration: 0 > >>>>> * Strange test result: 0 > >>>>> ********** > >>>>> > >>>>> $ PYTHON=python3 make check > >>>>> DEP tests/dumptrees.c > >>>>> DEP tests/trees.S > >>>>> DEP tests/testutils.c > >>>>> DEP tests/value-labels.c > >>>>> … > >>>>> ********** TEST SUMMARY > >>>>> * Total testcases: 1790 > >>>>> * PASS: 1790 > >>>>> * FAIL: 0 > >>>>> * Bad configuration: 0 > >>>>> * Strange test result: 0 > >>>>> ********** > >>>> So, the reason many less tests are run in this case is that the > >>>> Python3 setup.py uses a different name for the extension moule .so. > >>>> That means that the test scripts think there isn't Python support and > >>>> doesn't run those tests. We need to at least fix that before this is > >>>> ready to go. > >>> That's a great point! In my latest version I am able to run all > >>> tests under > >>> both Python 2 and 3. However, Python 3 module has to be renamed > >>> after build. > >>> > >>> With Python 2: > >>> > >>> $ make check > >>> … > >>> ********** TEST SUMMARY > >>> * Total testcases: 1880 > >>> * PASS: 1880 > >>> * FAIL: 0 > >>> * Bad configuration: 0 > >>> * Strange test result: 0 > >>> ********** > >>> > >>> WIth Python 3: > >>> > >>> $ PYTHON=python3 make > >>> $ mv pylibfdt/_libfdt{.cpython-37m-x86_64-linux-gnu,}.so > >>> $ PYTHON=python3 make check > >>> … > >>> ********** TEST SUMMARY > >>> * Total testcases: 1880 > >>> * PASS: 1880 > >>> * FAIL: 0 > >>> * Bad configuration: 0 > >>> * Strange test result: 0 > >>> ********** > >> Right. The trick is we need to work out how to either not make the > >> tests depend on the extension module being in that exact file, or > >> automatically move it as part of the build, and we need to do so in a > >> way that's robust across Python revisions. > >> > >>> The question is whether we need to support dual stack with both > >>> Pythons in > >>> the same time or not. > >> I'd certainly prefer it, but one at a time is better than nothing. > >> > >>> If yes, we have to find a way how to store both built modules and > >>> pick the > >>> right one for tests. > >> Exactly. In fact I think if we don't override the destination > >> setuptools will already put the modules in different locations. The > >> trick is working out how to make our tests link against the right one > >> at runtime. > > > > I rebased all changes to the top of current master and attaching all > > patches again. > > > > I did one more small change in run_tests.sh file where Python module > > is now recognized for both Python versions. > > > > So now, you can run all tests without any manual work. > > > > $ git clean -fdx > > $ make check > > … > > OK > > ********** TEST SUMMARY > > * Total testcases: 1880 > > * PASS: 1880 > > * FAIL: 0 > > * Bad configuration: 0 > > * Strange test result: 0 > > ********** > > > > $ git clean -fdx > > $ PYTHON=python3 make check > > … > > OK > > ********** TEST SUMMARY > > * Total testcases: 1880 > > * PASS: 1880 > > * FAIL: 0 > > * Bad configuration: 0 > > * Strange test result: 0 > > ********** > > > > You can also have dual stack support but it needs Python 3 version to > > be built first. Otherwise, when you build Python 2 extension first, > > build of Python 3 extension is skipped because _libfdt.so file is > > detected so make skips the build. The other way around, it works > > because Python 3 produces .so file with a platform suffix in its name > > and therefore it's not detected by make. > > > > Is this sufficient? > > > > Have a nice day. Lumír > > > >> > >>> If no, we can just rename it automatically after every build to the > >>> standard > >>> name _libfdt.so > >> Right, but while that's easy to do by hand, working out how to > >> reliably do it automatically isn't quite so obvious. > >> > >>> What is the preferred way? > >>> > >>>> I've merged some bits and pieces from your series, and I've made a few > >>>> other changes to get this closer. I hope this time I can actually get > >>>> to the rest soon. > >>> Great, thanks. I did some more small changes, rebased and I am > >>> attaching all > >>> patches again (except the one already merged, of course). > >>> >From 064a470664f8ac78162033602e7621b0d207618e Mon Sep 17 00:00:00 2001 > >>> From: Lumir Balhar <lbalhar@xxxxxxxxxx> > >>> Date: Tue, 10 Jul 2018 09:08:07 +0200 > >>> Subject: [PATCH 4/4] pylibfdt: Change how passing tests are recognized > >>> > >>> When some warning appears in test result, "ok" is still > >>> at the end of the line but without three dots. > >>> > >>> Signed-off-by: Lumir Balhar <lbalhar@xxxxxxxxxx> > >>> --- > >>> tests/run_tests.sh | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/tests/run_tests.sh b/tests/run_tests.sh > >>> index ca3fc86..1fa14ff 100755 > >>> --- a/tests/run_tests.sh > >>> +++ b/tests/run_tests.sh > >>> @@ -940,7 +940,7 @@ pylibfdt_tests () { > >>> # and the summary line for total tests (e.g. 'Ran 17 tests in > >>> 0.002s'). > >>> # We could add pass + fail to get total tests, but this > >>> provides a useful > >>> # sanity check. > >>> - pass_count=$(grep "\.\.\. ok$" $TMP | wc -l) > >>> + pass_count=$(grep "ok$" $TMP | wc -l) > >>> fail_count=$(grep "^ERROR: " $TMP | wc -l) > >>> total_tests=$(sed -n 's/^Ran \([0-9]*\) tests.*$/\1/p' $TMP) > >>> cat $TMP > >>> >From 8a19d132ca0db7e10ec816a39ae660f728df9e9d Mon Sep 17 00:00:00 2001 > >>> From: Petr Viktorin <pviktori@xxxxxxxxxx> > >>> Date: Mon, 9 Jul 2018 14:59:30 +0200 > >>> Subject: [PATCH 3/4] pylibfdt: Test fdt.setprop take bytes on Python > >>> 3, add > >>> error handling > >>> > >>> Signed-off-by: Petr Viktorin <pviktori@xxxxxxxxxx> > >>> --- > >>> pylibfdt/libfdt.i | 4 ++++ > >>> tests/pylibfdt_tests.py | 12 +++++++----- > >>> 2 files changed, 11 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i > >>> index 084bc5b..4f14403 100644 > >>> --- a/pylibfdt/libfdt.i > >>> +++ b/pylibfdt/libfdt.i > >>> @@ -1084,6 +1084,10 @@ typedef uint32_t fdt32_t; > >>> /* typemap used for fdt_setprop() */ > >>> %typemap(in) (const void *val) { > >>> %#if PY_VERSION_HEX >= 0x03000000 > >>> + if (!PyBytes_Check($input)) { > >>> + SWIG_exception_fail(SWIG_TypeError, "bytes expected in > >>> method '" "$symname" > >>> + "', argument " "$argnum"" of type '" "$type""'"); > >>> + } > >>> $1 = PyBytes_AsString($input); > >>> %#else > >>> $1 = PyString_AsString($input); /* char *str */ > >>> diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py > >>> index 4761c52..e6c13ff 100644 > >>> --- a/tests/pylibfdt_tests.py > >>> +++ b/tests/pylibfdt_tests.py > >>> @@ -82,6 +82,8 @@ TEST_VALUE64_1 = (TEST_VALUE64_1H << 32) | > >>> TEST_VALUE64_1L > >>> PHANDLE_1 = 0x2000 > >>> PHANDLE_2 = 0x2001 > >>> +TEST_BYTES_1 = b'hello world' > >>> + > >>> TEST_STRING_1 = 'hello world' > >>> TEST_STRING_2 = 'hi world' > >>> TEST_STRING_3 = u'unicode \u01d3' > >>> @@ -443,21 +445,21 @@ class PyLibfdtBasicTests(unittest.TestCase): > >>> def testSetProp(self): > >>> """Test that we can update and create properties""" > >>> node = self.fdt.path_offset('/subnode@1') > >>> - self.fdt.setprop(node, 'compatible', TEST_STRING_1) > >>> - self.assertEquals(TEST_STRING_1, self.fdt.getprop(node, > >>> 'compatible')) > >>> + self.fdt.setprop(node, 'compatible', TEST_BYTES_1) > >>> + self.assertEquals(TEST_BYTES_1, self.fdt.getprop(node, > >>> 'compatible')) > >>> # Check that this property is missing, and that we don't > >>> have space to > >>> # add it > >>> self.assertEquals(-libfdt.NOTFOUND, > >>> self.fdt.getprop(node, 'missing', > >>> QUIET_NOTFOUND)) > >>> self.assertEquals(-libfdt.NOSPACE, > >>> - self.fdt.setprop(node, 'missing', > >>> TEST_STRING_1, > >>> + self.fdt.setprop(node, 'missing', > >>> TEST_BYTES_1, > >>> quiet=(libfdt.NOSPACE,))) > >>> # Expand the device tree so we now have room > >>> self.fdt.resize(self.fdt.totalsize() + 50) > >>> - self.fdt.setprop(node, 'missing', TEST_STRING_1) > >>> - self.assertEquals(TEST_STRING_1, self.fdt.getprop(node, > >>> 'missing')) > >>> + self.fdt.setprop(node, 'missing', TEST_BYTES_1) > >>> + self.assertEquals(TEST_BYTES_1, self.fdt.getprop(node, > >>> 'missing')) > >>> def testSetPropU32(self): > >>> """Test that we can update and create integer properties""" > >>> >From c96c95413f0432fe1bf36796d5c8027b36ac5b93 Mon Sep 17 00:00:00 2001 > >>> From: Lumir Balhar <lbalhar@xxxxxxxxxx> > >>> Date: Mon, 9 Jul 2018 12:41:13 +0200 > >>> Subject: [PATCH 2/4] pylibfdt: check_err accepts only integer as a > >>> first > >>> argument. > >>> > >>> A list passed as an argument to check_err() means that > >>> there is no error code to check and therefore it should > >>> be returned back. > >>> > >>> Signed-off-by: Lumir Balhar <lbalhar@xxxxxxxxxx> > >>> --- > >>> pylibfdt/libfdt.i | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i > >>> index 6f1f1dc..084bc5b 100644 > >>> --- a/pylibfdt/libfdt.i > >>> +++ b/pylibfdt/libfdt.i > >>> @@ -137,7 +137,7 @@ def check_err(val, quiet=()): > >>> Raises > >>> FdtException if val < 0 > >>> """ > >>> - if val < 0: > >>> + if isinstance(val, int) and val < 0: > >>> if -val not in quiet: > >>> raise FdtException(val) > >>> return val > >>> >From 751a34fee080c9aaca6033aad9501a849a1ab8a7 Mon Sep 17 00:00:00 2001 > >>> From: Lumir Balhar <lbalhar@xxxxxxxxxx> > >>> Date: Mon, 9 Jul 2018 12:40:09 +0200 > >>> Subject: [PATCH 1/4] pylibfdt: Proper handling of bytes/unicode > >>> strings and > >>> octal literals > >>> > >>> Signed-off-by: Lumir Balhar <lbalhar@xxxxxxxxxx> > >>> --- > >>> pylibfdt/libfdt.i | 14 +++++++++++--- > >>> tests/pylibfdt_tests.py | 20 ++++++++++---------- > >>> 2 files changed, 21 insertions(+), 13 deletions(-) > >>> > >>> diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i > >>> index 462b5b0..6f1f1dc 100644 > >>> --- a/pylibfdt/libfdt.i > >>> +++ b/pylibfdt/libfdt.i > >>> @@ -669,7 +669,7 @@ class Fdt(FdtRo): > >>> Raises: > >>> FdtException if no parent found or other error occurs > >>> """ > >>> - val = val.encode('utf-8') + '\0' > >>> + val = val.encode('utf-8') + b'\0' > >>> return check_err(fdt_setprop(self._fdt, nodeoffset, > >>> prop_name, > >>> val, len(val)), quiet) > >>> @@ -1074,12 +1074,20 @@ typedef uint32_t fdt32_t; > >>> if (!$1) > >>> $result = Py_None; > >>> else > >>> - $result = Py_BuildValue("s#", $1, *arg4); > >>> + %#if PY_VERSION_HEX >= 0x03000000 > >>> + $result = Py_BuildValue("y#", $1, *arg4); > >>> + %#else > >>> + $result = Py_BuildValue("s#", $1, *arg4); > >>> + %#endif > >>> } > >>> /* typemap used for fdt_setprop() */ > >>> %typemap(in) (const void *val) { > >>> - $1 = PyString_AsString($input); /* char *str */ > >>> + %#if PY_VERSION_HEX >= 0x03000000 > >>> + $1 = PyBytes_AsString($input); > >>> + %#else > >>> + $1 = PyString_AsString($input); /* char *str */ > >>> + %#endif > >>> } > >>> /* typemaps used for fdt_next_node() */ > >>> diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py > >>> index 34c680d..4761c52 100644 > >>> --- a/tests/pylibfdt_tests.py > >>> +++ b/tests/pylibfdt_tests.py > >>> @@ -84,7 +84,7 @@ PHANDLE_2 = 0x2001 > >>> TEST_STRING_1 = 'hello world' > >>> TEST_STRING_2 = 'hi world' > >>> -TEST_STRING_3 = u'unicode ' + unichr(467) > >>> +TEST_STRING_3 = u'unicode \u01d3' > >>> def get_err(err_code): > >>> @@ -107,7 +107,7 @@ def _ReadFdt(fname): > >>> Returns: > >>> Fdt bytearray suitable for passing to libfdt functions > >>> """ > >>> - return libfdt.Fdt(open(fname).read()) > >>> + return libfdt.Fdt(open(fname, mode='rb').read()) > >>> class PyLibfdtBasicTests(unittest.TestCase): > >>> """Test class for basic pylibfdt access functions > >>> @@ -164,7 +164,7 @@ class PyLibfdtBasicTests(unittest.TestCase): > >>> def testBadFdt(self): > >>> """Check that a filename provided accidentally is not > >>> accepted""" > >>> with self.assertRaises(FdtException) as e: > >>> - fdt = libfdt.Fdt('a string') > >>> + fdt = libfdt.Fdt(b'a string') > >>> self.assertEquals(e.exception.err, -libfdt.BADMAGIC) > >>> def testSubnodeOffset(self): > >>> @@ -239,7 +239,7 @@ class PyLibfdtBasicTests(unittest.TestCase): > >>> poffset = self.fdt.first_property_offset(root) > >>> prop = self.fdt.get_property_by_offset(poffset) > >>> self.assertEquals(prop.name, 'compatible') > >>> - self.assertEquals(prop, 'test_tree1\0') > >>> + self.assertEquals(prop, b'test_tree1\0') > >>> with self.assertRaises(FdtException) as e: > >>> self.fdt.get_property_by_offset(-2) > >>> @@ -252,7 +252,7 @@ class PyLibfdtBasicTests(unittest.TestCase): > >>> """Check that we can read the contents of a property by > >>> name""" > >>> root = self.fdt.path_offset('/') > >>> value = self.fdt.getprop(root, "compatible") > >>> - self.assertEquals(value, 'test_tree1\0') > >>> + self.assertEquals(value, b'test_tree1\0') > >>> self.assertEquals(-libfdt.NOTFOUND, self.fdt.getprop(root, > >>> 'missing', > >>> QUIET_NOTFOUND)) > >>> @@ -262,7 +262,7 @@ class PyLibfdtBasicTests(unittest.TestCase): > >>> node = self.fdt.path_offset('/subnode@1/subsubnode') > >>> value = self.fdt.getprop(node, "compatible") > >>> - self.assertEquals(value, 'subsubnode1\0subsubnode\0') > >>> + self.assertEquals(value, b'subsubnode1\0subsubnode\0') > >>> def testStrError(self): > >>> """Check that we can get an error string""" > >>> @@ -591,7 +591,7 @@ class PyLibfdtSwTests(unittest.TestCase): > >>> # Make sure we can read from the tree too > >>> node = sw.path_offset('/subnode@1') > >>> - self.assertEqual('subnode1' + chr(0), sw.getprop(node, > >>> 'compatible')) > >>> + self.assertEqual(b'subnode1\0', sw.getprop(node, > >>> 'compatible')) > >>> # Make sure we did at least two resizes > >>> self.assertTrue(len(fdt.as_bytearray()) > FdtSw.INC_SIZE * 2) > >>> @@ -609,15 +609,15 @@ class PyLibfdtRoTests(unittest.TestCase): > >>> def setUp(self): > >>> """Read in the device tree we use for testing""" > >>> - self.fdt = libfdt.FdtRo(open('test_tree1.dtb').read()) > >>> + self.fdt = libfdt.FdtRo(open('test_tree1.dtb', > >>> mode='rb').read()) > >>> def testAccess(self): > >>> """Basic sanity check for the FdtRo class""" > >>> node = self.fdt.path_offset('/subnode@1') > >>> - self.assertEqual('subnode1' + chr(0), > >>> + self.assertEqual(b'subnode1\0', > >>> self.fdt.getprop(node, 'compatible')) > >>> node = self.fdt.first_subnode(node) > >>> - self.assertEqual('this is a placeholder string\0string2\0', > >>> + self.assertEqual(b'this is a placeholder string\0string2\0', > >>> self.fdt.getprop(node, 'placeholder')) > >>
