[Buildroot] [PATCH 1/2] package/bmap-tools: new package

[Buildroot] [PATCH 1/2] package/bmap-tools: new package

[Nicolas Carrier]

Signed-off-by: Nicolas Carrier <nicolas.carrier@orolia.com>
---
 DEVELOPERS                         |  3 +++
 package/Config.in                  |  1 +
 package/bmap-tools/Config.in       | 13 +++++++++++++
 package/bmap-tools/bmap-tools.hash |  2 ++
 package/bmap-tools/bmap-tools.mk   | 15 +++++++++++++++
 5 files changed, 34 insertions(+)
 create mode 100644 package/bmap-tools/Config.in
 create mode 100644 package/bmap-tools/bmap-tools.hash
 create mode 100644 package/bmap-tools/bmap-tools.mk

diff --git a/DEVELOPERS b/DEVELOPERS
index b9e6881419..6fe457ac47 100644
--- a/DEVELOPERS
+++ b/DEVELOPERS
@@ -1725,6 +1725,9 @@ F:	package/libevdev/
 N:	Nicola Di Lieto <nicola.dilieto@gmail.com>
 F:	package/uacme/
 
+N:	Nicolas Carrier <nicolas.carrier@orolia.com>
+F:	package/bmap-tools/
+
 N:	Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
 F:	package/libgit2/
 
diff --git a/package/Config.in b/package/Config.in
index 901c25fe02..951342042a 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -184,6 +184,7 @@ menu "Filesystem and flash utilities"
 	source "package/aufs/Config.in"
 	source "package/aufs-util/Config.in"
 	source "package/autofs/Config.in"
+	source "package/bmap-tools/Config.in"
 	source "package/btrfs-progs/Config.in"
 	source "package/cifs-utils/Config.in"
 	source "package/cpio/Config.in"
diff --git a/package/bmap-tools/Config.in b/package/bmap-tools/Config.in
new file mode 100644
index 0000000000..63301f2558
--- /dev/null
+++ b/package/bmap-tools/Config.in
@@ -0,0 +1,13 @@
+config BR2_PACKAGE_BMAP_TOOLS
+	bool "bmap-tools"
+	depends on BR2_PACKAGE_PYTHON3 || \
+		BR2_PACKAGE_PYTHON
+	select BR2_PACKAGE_PYTHON_SIX # runtime
+	select BR2_PACKAGE_PYTHON_SETUPTOOLS
+	help
+	  Tool to flash image files to block devices using the block map
+	  bmaptool is a generic tool for creating the block map (bmap)
+	  for a file, and copying files using the block map. The idea is
+	  that large file containing unused blocks, like raw system
+	  image files, can be copied or flashed a lot faster with
+	  bmaptool than with traditional tools like "dd" or "cp".
diff --git a/package/bmap-tools/bmap-tools.hash b/package/bmap-tools/bmap-tools.hash
new file mode 100644
index 0000000000..ad56b98c85
--- /dev/null
+++ b/package/bmap-tools/bmap-tools.hash
@@ -0,0 +1,2 @@
+sha256  d410e2d97192d0fc2f88ef160a0bb6ed83fce99da97a606d7f6890cc654ec594  bmap-tools-v3.5.tar.gz
+sha256  8177f97513213526df2cf6184d8ff986c675afb514d4e68a404010521b880643  COPYING
diff --git a/package/bmap-tools/bmap-tools.mk b/package/bmap-tools/bmap-tools.mk
new file mode 100644
index 0000000000..c86f545ab0
--- /dev/null
+++ b/package/bmap-tools/bmap-tools.mk
@@ -0,0 +1,15 @@
+################################################################################
+#
+# bmap-tools
+#
+################################################################################
+
+BMAP_TOOLS_VERSION = v3.5
+BMAP_TOOLS_SITE = $(call github,intel,bmap-tools,$(BMAP_TOOLS_VERSION))
+BMAP_TOOLS_LICENSE = GPLv2
+BMAP_TOOLS_LICENSE_FILES = COPYING
+BMAP_TOOLS_SETUP_TYPE = setuptools
+BMAP_TOOLS_DEPENDENCIES = \
+	python-setuptools
+
+$(eval $(python-package))
-- 
2.20.1

[Buildroot] [PATCH 2/2] support/testing/tests/package/: runtime test

[Nicolas Carrier]

This patch implements a simple test in which a dummy file system image
is created, then `bmaptool create` and `bmaptool copy` are used to copy
it to another file.

Signed-off-by: Nicolas Carrier <nicolas.carrier@orolia.com>
---
 .../tests/package/sample_bmap_tools.sh        | 15 ++++++
 .../testing/tests/package/test_bmap_tools.py  | 54 +++++++++++++++++++
 2 files changed, 69 insertions(+)
 create mode 100755 support/testing/tests/package/sample_bmap_tools.sh
 create mode 100644 support/testing/tests/package/test_bmap_tools.py

diff --git a/support/testing/tests/package/sample_bmap_tools.sh b/support/testing/tests/package/sample_bmap_tools.sh
new file mode 100755
index 0000000000..7c90368c17
--- /dev/null
+++ b/support/testing/tests/package/sample_bmap_tools.sh
@@ -0,0 +1,15 @@
+#!/bin/sh
+# simple test which creates a dummy file system image, then use bmaptool create
+# and bmaptool copy to copy it to another file
+
+set -xeu
+
+# create the necessary test files
+dd if=/dev/zero of=disk.img bs=2M count=1
+mkfs.ext4 disk.img
+fallocate -d disk.img
+dd if=/dev/zero of=copy.img bs=2M count=1
+
+# do a test copy of the file system image
+bmaptool create -o disk.img.bmap disk.img
+bmaptool copy disk.img copy.img
diff --git a/support/testing/tests/package/test_bmap_tools.py b/support/testing/tests/package/test_bmap_tools.py
new file mode 100644
index 0000000000..02bd4634d4
--- /dev/null
+++ b/support/testing/tests/package/test_bmap_tools.py
@@ -0,0 +1,54 @@
+import os
+import sys
+import infra
+
+from infra.basetest import BRTest
+from abc import ABC, abstractproperty
+
+class AbstractBmapToolsTest(BRTest, ABC):
+    __test__ = False
+    sample_script = "tests/package/sample_bmap_tools.sh"
+
+    copy_script = 'tests/package/copy-sample-script-to-target.sh'
+    config = f'''
+        {infra.basetest.BASIC_TOOLCHAIN_CONFIG}
+        BR2_TARGET_ROOTFS_CPIO=y
+        BR2_PACKAGE_BMAP_TOOLS=y
+        BR2_ROOTFS_POST_BUILD_SCRIPT="{infra.filepath(copy_script)}"
+        BR2_ROOTFS_POST_SCRIPT_ARGS="{infra.filepath(sample_script)}"
+        # BR2_TARGET_ROOTFS_TAR is not set
+        BR2_PACKAGE_UTIL_LINUX=y
+        BR2_PACKAGE_UTIL_LINUX_FALLOCATE=y
+        BR2_PACKAGE_E2FSPROGS=y
+        BR2_PACKAGE_UTIL_LINUX_LIBUUID=y
+        '''
+
+    def __init__(self, names):
+        super(AbstractBmapToolsTest, self).__init__(names)
+        self.config += f"BR2_PACKAGE_PYTHON{self.python_version}=y"
+
+    @abstractproperty
+    def python_version(self):
+        pass
+
+    def login(self):
+        cpio_file = os.path.join(self.builddir, "images", "rootfs.cpio")
+        self.emulator.boot(arch="armv5",
+                           kernel="builtin",
+                           options=["-initrd", cpio_file])
+        self.emulator.login()
+
+    def test_run(self):
+        print(self.config, file=sys.stderr)
+        self.login()
+        cmd = f"/root/{os.path.basename(self.sample_script)}"
+        _, exit_code = self.emulator.run(cmd, timeout=10)
+        self.assertEqual(exit_code, 0)
+
+class TestPy2BmapTools(AbstractBmapToolsTest):
+    __test__ = True
+    python_version = ""
+
+class TestPy3BmapTools(AbstractBmapToolsTest):
+    __test__ = True
+    python_version = "3"
-- 
2.20.1

[Buildroot] [PATCH 1/2] package/bmap-tools: new package

[Thomas Petazzoni]

Hello Nicolas,

On Sat, 26 Oct 2019 14:41:18 +0000
Nicolas Carrier <nicolas.carrier@orolia.com> wrote:

> diff --git a/package/bmap-tools/Config.in b/package/bmap-tools/Config.in
> new file mode 100644
> index 0000000000..63301f2558
> --- /dev/null
> +++ b/package/bmap-tools/Config.in
> @@ -0,0 +1,13 @@
> +config BR2_PACKAGE_BMAP_TOOLS
> +	bool "bmap-tools"
> +	depends on BR2_PACKAGE_PYTHON3 || \
> +		BR2_PACKAGE_PYTHON

For some packages like this, we also do a simple select, because it's
not obvious you need Python.

> +	select BR2_PACKAGE_PYTHON_SIX # runtime
> +	select BR2_PACKAGE_PYTHON_SETUPTOOLS

Do you really need setuptools on the target? This seems odd.


> +BMAP_TOOLS_VERSION = v3.5

The "v" should be in BMAP_TOOLS_SITE, not BMAP_TOOLS_VERSION.

> +BMAP_TOOLS_SITE = $(call github,intel,bmap-tools,$(BMAP_TOOLS_VERSION))
> +BMAP_TOOLS_LICENSE = GPLv2

Should be GPL-2.0

> +BMAP_TOOLS_LICENSE_FILES = COPYING
> +BMAP_TOOLS_SETUP_TYPE = setuptools
> +BMAP_TOOLS_DEPENDENCIES = \
> +	python-setuptools

Why do you need python-setuptools on the target, and be a build time
dependency ? BMAP_TOOLS_SETUP_TYPE = setuptools already says that this
package needs setuptools on the host for its build/installation.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/2] package/bmap-tools: new package

[Thomas Petazzoni]

On Sat, 26 Oct 2019 14:41:18 +0000
Nicolas Carrier <nicolas.carrier@orolia.com> wrote:

> +	  Tool to flash image files to block devices using the block map
> +	  bmaptool is a generic tool for creating the block map (bmap)
> +	  for a file, and copying files using the block map. The idea is
> +	  that large file containing unused blocks, like raw system
> +	  image files, can be copied or flashed a lot faster with
> +	  bmaptool than with traditional tools like "dd" or "cp".

Another question is that you're packaging this for the target, but it
sounds like a tool that would be mainly useful on the host to
prepare/flash a SD card. Do you have a use case for having this tool on
the target ?

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 2/2] support/testing/tests/package/: runtime test

[Thomas Petazzoni]

Hello Nicolas,

On Sat, 26 Oct 2019 14:41:21 +0000
Nicolas Carrier <nicolas.carrier@orolia.com> wrote:

> This patch implements a simple test in which a dummy file system image
> is created, then `bmaptool create` and `bmaptool copy` are used to copy
> it to another file.
> 
> Signed-off-by: Nicolas Carrier <nicolas.carrier@orolia.com>

The title should be: support/testing/tests/package/test_bmap_tools: new test

Also, please add an entry in the DEVELOPERS file for those two new
files you are doing. This will make sure you get notified when there is
a failure of your test in our CI.

> diff --git a/support/testing/tests/package/test_bmap_tools.py b/support/testing/tests/package/test_bmap_tools.py
> new file mode 100644
> index 0000000000..02bd4634d4
> --- /dev/null
> +++ b/support/testing/tests/package/test_bmap_tools.py
> @@ -0,0 +1,54 @@
> +import os
> +import sys
> +import infra
> +
> +from infra.basetest import BRTest
> +from abc import ABC, abstractproperty

I don't think we really need this, it's not used anywhere else.

> +
> +class AbstractBmapToolsTest(BRTest, ABC):

Name this just:

class TestBmapTools(infra.basetest.BRTest)

> +    __test__ = False
> +    sample_script = "tests/package/sample_bmap_tools.sh"
> +

Maybe this empty line is not really needed.

BTW, did you run flake8 on your Python code to make sure it is
compliant with Buildroot's Python coding style ?

> +    copy_script = 'tests/package/copy-sample-script-to-target.sh'
> +    config = f'''
> +        {infra.basetest.BASIC_TOOLCHAIN_CONFIG}

I didn't know about this syntax. I don't think we're using it
elsewhere, but that's OK for me, this syntax is quite nice.

> +        BR2_TARGET_ROOTFS_CPIO=y
> +        BR2_PACKAGE_BMAP_TOOLS=y
> +        BR2_ROOTFS_POST_BUILD_SCRIPT="{infra.filepath(copy_script)}"
> +        BR2_ROOTFS_POST_SCRIPT_ARGS="{infra.filepath(sample_script)}"
> +        # BR2_TARGET_ROOTFS_TAR is not set
> +        BR2_PACKAGE_UTIL_LINUX=y
> +        BR2_PACKAGE_UTIL_LINUX_FALLOCATE=y
> +        BR2_PACKAGE_E2FSPROGS=y
> +        BR2_PACKAGE_UTIL_LINUX_LIBUUID=y
> +        '''
> +
> +    def __init__(self, names):
> +        super(AbstractBmapToolsTest, self).__init__(names)
> +        self.config += f"BR2_PACKAGE_PYTHON{self.python_version}=y"

I don't think you need this.

> +    @abstractproperty
> +    def python_version(self):
> +        pass

or this.

> +
> +    def login(self):
> +        cpio_file = os.path.join(self.builddir, "images", "rootfs.cpio")
> +        self.emulator.boot(arch="armv5",
> +                           kernel="builtin",
> +                           options=["-initrd", cpio_file])
> +        self.emulator.login()
> +
> +    def test_run(self):
> +        print(self.config, file=sys.stderr)
> +        self.login()
> +        cmd = f"/root/{os.path.basename(self.sample_script)}"
> +        _, exit_code = self.emulator.run(cmd, timeout=10)
> +        self.assertEqual(exit_code, 0)
> +
> +class TestPy2BmapTools(AbstractBmapToolsTest):
> +    __test__ = True
> +    python_version = ""

Just do this:

class TestBmapToolsPython2(TestBmapTools):
	__test__ = True
	config = TestBmapTools.config + \
	"""
	BR2_PACKAGE_PYTHON=y
	"""

and ditto for Python 3.x of course. See test_python_treq.py for an
example of how we are doing it.

I understand what you're proposing is a different way of doing the same
thing, we really value consistency across the code base. So we want a
given thing to always be done the same way.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/2] package/bmap-tools: new package

[Nicolas Carrier]

On Sat, 2019-10-26 at 19:30 +0200, Thomas Petazzoni wrote:
> ...
> 
> Do you really need setuptools on the target? This seems odd.
> 

Apparently yes, otherwise I have this failure when running the test:
Traceback (most recent call last):
  File "/usr/bin/bmaptool", line 6, in <module>
    from pkg_resources import load_entry_point
ModuleNotFoundError: No module named 'pkg_resources'

And a bit of searching on SO showed that having setuptools was the
missing package.

> 
> > +BMAP_TOOLS_VERSION = v3.5
> 
> The "v" should be in BMAP_TOOLS_SITE, not BMAP_TOOLS_VERSION.

Ok, will fix in v3.

> 
> > +BMAP_TOOLS_SITE = $(call github,intel,bmap-
> > tools,$(BMAP_TOOLS_VERSION))
> > +BMAP_TOOLS_LICENSE = GPLv2
> 
> Should be GPL-2.0

Ok, will fix in v3.

> 
> > +BMAP_TOOLS_LICENSE_FILES = COPYING
> > +BMAP_TOOLS_SETUP_TYPE = setuptools
> > +BMAP_TOOLS_DEPENDENCIES = \
> > +     python-setuptools
> 
> Why do you need python-setuptools on the target, and be a build time
> dependency ? BMAP_TOOLS_SETUP_TYPE = setuptools already says that
> this
> package needs setuptools on the host for its build/installation.

As said before, it appears to be a runtime dependency too.

> 
> Thanks,
> 
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> ATTENTION: This email came from an external source.
> Do not open attachments or click on links from unknown senders or
> unexpected emails.

[Buildroot] [PATCH 1/2] package/bmap-tools: new package

[Nicolas Carrier]

On Sat, 2019-10-26 at 19:32 +0200, Thomas Petazzoni wrote:
> On Sat, 26 Oct 2019 14:41:18 +0000
> Nicolas Carrier <nicolas.carrier@orolia.com> wrote:
> 
> > +       Tool to flash image files to block devices using the block
> > map
> > +       bmaptool is a generic tool for creating the block map
> > (bmap)
> > +       for a file, and copying files using the block map. The idea
> > is
> > +       that large file containing unused blocks, like raw system
> > +       image files, can be copied or flashed a lot faster with
> > +       bmaptool than with traditional tools like "dd" or "cp".
> 
> Another question is that you're packaging this for the target, but it
> sounds like a tool that would be mainly useful on the host to
> prepare/flash a SD card. Do you have a use case for having this tool
> on
> the target ?

Yes, I use it to speed up the flashing of the EMMC too, it's part of
our update or bootstrap procedures.

> 
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> ATTENTION: This email came from an external source.
> Do not open attachments or click on links from unknown senders or
> unexpected emails.

[Buildroot] [PATCH 2/2] support/testing/tests/package/: runtime test

[Arnout Vandecappelle]

On 26/10/2019 19:41, Thomas Petazzoni wrote:
> Hello Nicolas,
> 
> On Sat, 26 Oct 2019 14:41:21 +0000
> Nicolas Carrier <nicolas.carrier@orolia.com> wrote:
> 
>> This patch implements a simple test in which a dummy file system image
>> is created, then `bmaptool create` and `bmaptool copy` are used to copy
>> it to another file.
>>
>> Signed-off-by: Nicolas Carrier <nicolas.carrier@orolia.com>
> 
> The title should be: support/testing/tests/package/test_bmap_tools: new test

 Not that I have the power to veto, but I veto this :-) That's a ridiculously
long title. I think it should be "support/testing: add bmap_tools test". That is
BTW what 90% of the titles of patches in support/testing are now.

> 
> Also, please add an entry in the DEVELOPERS file for those two new
> files you are doing. This will make sure you get notified when there is
> a failure of your test in our CI.
> 
>> diff --git a/support/testing/tests/package/test_bmap_tools.py b/support/testing/tests/package/test_bmap_tools.py
>> new file mode 100644
>> index 0000000000..02bd4634d4
>> --- /dev/null
>> +++ b/support/testing/tests/package/test_bmap_tools.py
>> @@ -0,0 +1,54 @@
>> +import os
>> +import sys
>> +import infra
>> +
>> +from infra.basetest import BRTest
>> +from abc import ABC, abstractproperty
> 
> I don't think we really need this, it's not used anywhere else.

 If we're using ABC, then BRTest should definitely be an abstract base class.
But indeed, for consistency, it would be better to stick to the patterns we are
using now, or convert things globally.

> 
>> +
>> +class AbstractBmapToolsTest(BRTest, ABC):
> 
> Name this just:
> 
> class TestBmapTools(infra.basetest.BRTest)
> 
>> +    __test__ = False
>> +    sample_script = "tests/package/sample_bmap_tools.sh"
>> +
> 
> Maybe this empty line is not really needed.
> 
> BTW, did you run flake8 on your Python code to make sure it is
> compliant with Buildroot's Python coding style ?
> 
>> +    copy_script = 'tests/package/copy-sample-script-to-target.sh'
>> +    config = f'''
>> +        {infra.basetest.BASIC_TOOLCHAIN_CONFIG}
> 
> I didn't know about this syntax. I don't think we're using it
> elsewhere, but that's OK for me, this syntax is quite nice.

 f-strings were introduced in Python 3.5. I'm a big fan of f-string too, but
then we should document that we require Python 3.5 for running the tests.

> 
>> +        BR2_TARGET_ROOTFS_CPIO=y
>> +        BR2_PACKAGE_BMAP_TOOLS=y
>> +        BR2_ROOTFS_POST_BUILD_SCRIPT="{infra.filepath(copy_script)}"
>> +        BR2_ROOTFS_POST_SCRIPT_ARGS="{infra.filepath(sample_script)}"
>> +        # BR2_TARGET_ROOTFS_TAR is not set
>> +        BR2_PACKAGE_UTIL_LINUX=y
>> +        BR2_PACKAGE_UTIL_LINUX_FALLOCATE=y
>> +        BR2_PACKAGE_E2FSPROGS=y
>> +        BR2_PACKAGE_UTIL_LINUX_LIBUUID=y
>> +        '''
>> +
>> +    def __init__(self, names):
>> +        super(AbstractBmapToolsTest, self).__init__(names)
>> +        self.config += f"BR2_PACKAGE_PYTHON{self.python_version}=y"
> 
> I don't think you need this.

 I think to deal with variations like this, it would be better to have support
for formatting the config string. I.e., before writing the config file, we pass
it through format() with a subclass-provided dict as argument.

 But again, that's a bigger change. For now, you can just copy the self.config
+= to the different subclasses, it's just a tiny increase in the number of lines.

> 
>> +    @abstractproperty
>> +    def python_version(self):
>> +        pass
> 
> or this.
> 
>> +
>> +    def login(self):
>> +        cpio_file = os.path.join(self.builddir, "images", "rootfs.cpio")
>> +        self.emulator.boot(arch="armv5",
>> +                           kernel="builtin",
>> +                           options=["-initrd", cpio_file])
>> +        self.emulator.login()
>> +
>> +    def test_run(self):
>> +        print(self.config, file=sys.stderr)
>> +        self.login()
>> +        cmd = f"/root/{os.path.basename(self.sample_script)}"
>> +        _, exit_code = self.emulator.run(cmd, timeout=10)
>> +        self.assertEqual(exit_code, 0)
>> +
>> +class TestPy2BmapTools(AbstractBmapToolsTest):
>> +    __test__ = True
>> +    python_version = ""
> 
> Just do this:
> 
> class TestBmapToolsPython2(TestBmapTools):
> 	__test__ = True
> 	config = TestBmapTools.config + \
> 	"""
> 	BR2_PACKAGE_PYTHON=y
> 	"""
> 
> and ditto for Python 3.x of course. See test_python_treq.py for an
> example of how we are doing it.
> 
> I understand what you're proposing is a different way of doing the same
> thing, we really value consistency across the code base. So we want a
> given thing to always be done the same way.

 +1

 Regards,
 Arnout

> 
> Best regards,
> 
> Thomas
>