diff mbox

[1/1] i2c-tools: add support to build python extension

Message ID 1428259675-7116-1-git-send-email-ryanbarnett3@gmail.com
State Changes Requested
Headers show

Commit Message

Ryan Barnett April 5, 2015, 6:47 p.m. UTC
Add a config option to build the python bindings for i2c-tools -
py-smbus. The steps for building the python bindings is the same as
the distutil steps that are a part of the python infrastructure.

Signed-off-by: Ryan Barnett <ryanbarnett3@gmail.com>
CC: Tjeerd Pinkert <t.j.pinkert@vu.nl>
CC: Zoltan Gyarmati <mr.zoltan.gyarmati@gmail.com>
---
 package/i2c-tools/Config.in    | 13 +++++++++++++
 package/i2c-tools/i2c-tools.mk | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

Comments

Thomas Petazzoni April 5, 2015, 7 p.m. UTC | #1
Dear Ryan Barnett,

On Sun,  5 Apr 2015 13:47:55 -0500, Ryan Barnett wrote:
> Add a config option to build the python bindings for i2c-tools -
> py-smbus. The steps for building the python bindings is the same as
> the distutil steps that are a part of the python infrastructure.

Yeah, it's a bit sad that we can't re-use the Python package
infrastructure is.

I think we discussed this during the Buildroot Developers Meeting,
though I can't find any reference to this. Cc'ing Arnout.

So essentially, we have two choices:
 
 * Have separate packages for the C/C++ library and the Python
   bindings. Like is done today for protobuf + python-protobuf. One
   problem is how to share the VERSION / SITE / SOURCE variables. The
   other problem is that people don't necessarily notice when bumping a
   package version. But the plus side is that we can re-use the python
   package infrastructure.

 * Use one single package. Makes it trivial to share VERSION / SITE /
   SOURCE, no possible omission when bumping. But the down-side is that
   we cannot re-use the python package infra.

Maybe we should take the second approach, but adjust a bit the python
package infra to make it easier for packages not using the infra to use
some of its variables?

> diff --git a/package/i2c-tools/i2c-tools.mk b/package/i2c-tools/i2c-tools.mk
> index 0115e22..f363505 100644
> --- a/package/i2c-tools/i2c-tools.mk
> +++ b/package/i2c-tools/i2c-tools.mk
> @@ -21,4 +21,37 @@ define I2C_TOOLS_INSTALL_TARGET_CMDS
>  	done
>  endef
>  
> +# BASE_ENV taken from PKG_PYTHON_DISTUTILS_ENV in package/pkg-python.mk
> +I2C_TOOLS_PYTHON_BASE_ENV = PATH=$(BR_PATH) \
> +	CC="$(TARGET_CC)" \
> +	CFLAGS="$(TARGET_CFLAGS) -I../include" \
> +	LDFLAGS="$(TARGET_LDFLAGS)" \
> +	LDSHARED="$(TARGET_CROSS)gcc -shared" \
> +	PYTHONPATH="$(if $(BR2_PACKAGE_PYTHON3),$(PYTHON3_PATH),$(PYTHON_PATH))" \
> +	_python_sysroot=$(STAGING_DIR) \
> +	_python_prefix=/usr \
> +	_python_exec_prefix=/usr

What about:

I2C_TOOLS_PYTHON_BASE_ENV = \
	$(PKG_PYTHON_DISTUTILS_ENV) \
	CFLAGS="$(TARGET_CFLAGS) -I../include"

this way we re-use a part of the Python package infra.

> +# Build/install steps mirror the distutil python package type in the python package
> +# infrastructure
> +ifeq ($(BR2_PACKAGE_I2C_TOOLS_PYSMBUS),y)
> +I2C_TOOLS_DEPENDENCIES += $(if $(BR2_PACKAGE_PYTHON3),host-python3 python3,host-python python)

I know we're doing that in pkg-python.mk, but I'm not sure why. I
believe something like:

I2C_TOOLS_DEPENDENCIES += $(if $(BR2_PACKAGE_PYTHON3),python3,python)

is sufficient, since pythonX depends on host-pythonX.

> +define I2C_TOOLS_BUILD_PYSMBUS
> +	(cd $(@D)/py-smbus;  \
> +	$(I2C_TOOLS_PYTHON_BASE_ENV) \
> +		$(HOST_DIR)/usr/bin/python setup.py build \
> +		--executable=/usr/bin/python)

Maybe use PKG_PYTHON_DISTUTILS_BUILD_OPTS

> +endef
> +I2C_TOOLS_POST_BUILD_HOOKS += I2C_TOOLS_BUILD_PYSMBUS
> +
> +define I2C_TOOLS_INSTALL_PYSMBUS
> +	(cd $(@D)/py-smbus; \
> +	$(I2C_TOOLS_PYTHON_BASE_ENV) \
> +		$(HOST_DIR)/usr/bin/python setup.py install \
> +		--prefix=$(TARGET_DIR)/usr )

And PKG_PYTHON_DISTUTILS_INSTALL_TARGET_OPTS

Thomas
Baruch Siach April 5, 2015, 7:13 p.m. UTC | #2
Hi Ryan,

On Sun, Apr 05, 2015 at 01:47:55PM -0500, Ryan Barnett wrote:
> Add a config option to build the python bindings for i2c-tools -
> py-smbus. The steps for building the python bindings is the same as
> the distutil steps that are a part of the python infrastructure.
> 
> Signed-off-by: Ryan Barnett <ryanbarnett3@gmail.com>
> CC: Tjeerd Pinkert <t.j.pinkert@vu.nl>
> CC: Zoltan Gyarmati <mr.zoltan.gyarmati@gmail.com>
> ---
>  package/i2c-tools/Config.in    | 13 +++++++++++++
>  package/i2c-tools/i2c-tools.mk | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/package/i2c-tools/Config.in b/package/i2c-tools/Config.in
> index e83dbd6..2e537cb 100644
> --- a/package/i2c-tools/Config.in
> +++ b/package/i2c-tools/Config.in
> @@ -8,3 +8,16 @@ config BR2_PACKAGE_I2C_TOOLS
>  	  EEPROM decoding scripts, and more.
>  
>  	  http://www.lm-sensors.org/wiki/I2CTools
> +
> +if BR2_PACKAGE_I2C_TOOLS
> +
> +config BR2_PACKAGE_I2C_TOOLS_PYSMBUS

I don't think another config option is necessary. The size of i2c-tools python 
binding is negligible when compared to the size of python itself.

> +	bool "py-smbus"
> +	depends on BR2_PACKAGE_PYTHON

This seems to indicate that only python2 is supported ...

[...]

> +ifeq ($(BR2_PACKAGE_I2C_TOOLS_PYSMBUS),y)
> +I2C_TOOLS_DEPENDENCIES += $(if $(BR2_PACKAGE_PYTHON3),host-python3 python3,host-python python)

But this assumes that both python2 and python3 are supported. Which one is 
correct?

Also, there is no need to explicitly list python host variants. Target python 
packages already depend on their corresponding host packages.

baruch
Ryan Barnett April 5, 2015, 7:15 p.m. UTC | #3
Thomas,

On Sun, Apr 5, 2015 at 2:00 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Ryan Barnett,
>
> On Sun,  5 Apr 2015 13:47:55 -0500, Ryan Barnett wrote:
>> Add a config option to build the python bindings for i2c-tools -
>> py-smbus. The steps for building the python bindings is the same as
>> the distutil steps that are a part of the python infrastructure.
>
> Yeah, it's a bit sad that we can't re-use the Python package
> infrastructure is.
>
> I think we discussed this during the Buildroot Developers Meeting,
> though I can't find any reference to this. Cc'ing Arnout.
>
> So essentially, we have two choices:
>
>  * Have separate packages for the C/C++ library and the Python
>    bindings. Like is done today for protobuf + python-protobuf. One
>    problem is how to share the VERSION / SITE / SOURCE variables. The
>    other problem is that people don't necessarily notice when bumping a
>    package version. But the plus side is that we can re-use the python
>    package infrastructure.
>
>  * Use one single package. Makes it trivial to share VERSION / SITE /
>    SOURCE, no possible omission when bumping. But the down-side is that
>    we cannot re-use the python package infra.
>
> Maybe we should take the second approach, but adjust a bit the python
> package infra to make it easier for packages not using the infra to use
> some of its variables?

I too prefer the second approach. I don't have any ideas though on how
to make it easier to take advantage of the python package infra
though. Using the variables from the pkg-python.mk will simplify
things a bit.

>> diff --git a/package/i2c-tools/i2c-tools.mk b/package/i2c-tools/i2c-tools.mk
>> index 0115e22..f363505 100644
>> --- a/package/i2c-tools/i2c-tools.mk
>> +++ b/package/i2c-tools/i2c-tools.mk
>> @@ -21,4 +21,37 @@ define I2C_TOOLS_INSTALL_TARGET_CMDS
>>       done
>>  endef
>>
>> +# BASE_ENV taken from PKG_PYTHON_DISTUTILS_ENV in package/pkg-python.mk
>> +I2C_TOOLS_PYTHON_BASE_ENV = PATH=$(BR_PATH) \
>> +     CC="$(TARGET_CC)" \
>> +     CFLAGS="$(TARGET_CFLAGS) -I../include" \
>> +     LDFLAGS="$(TARGET_LDFLAGS)" \
>> +     LDSHARED="$(TARGET_CROSS)gcc -shared" \
>> +     PYTHONPATH="$(if $(BR2_PACKAGE_PYTHON3),$(PYTHON3_PATH),$(PYTHON_PATH))" \
>> +     _python_sysroot=$(STAGING_DIR) \
>> +     _python_prefix=/usr \
>> +     _python_exec_prefix=/usr
>
> What about:
>
> I2C_TOOLS_PYTHON_BASE_ENV = \
>         $(PKG_PYTHON_DISTUTILS_ENV) \
>         CFLAGS="$(TARGET_CFLAGS) -I../include"
>
> this way we re-use a part of the Python package infra.

Agreed. Will the second definition override the definition from
PKG_PYTHON_DISTUTILS_ENV?

>
>> +# Build/install steps mirror the distutil python package type in the python package
>> +# infrastructure
>> +ifeq ($(BR2_PACKAGE_I2C_TOOLS_PYSMBUS),y)
>> +I2C_TOOLS_DEPENDENCIES += $(if $(BR2_PACKAGE_PYTHON3),host-python3 python3,host-python python)
>
> I know we're doing that in pkg-python.mk, but I'm not sure why. I
> believe something like:
>
> I2C_TOOLS_DEPENDENCIES += $(if $(BR2_PACKAGE_PYTHON3),python3,python)
>
> is sufficient, since pythonX depends on host-pythonX.

Agree will simplify. Should I sent a second patch to do this too in
pkg-python.mk?

>> +define I2C_TOOLS_BUILD_PYSMBUS
>> +     (cd $(@D)/py-smbus;  \
>> +     $(I2C_TOOLS_PYTHON_BASE_ENV) \
>> +             $(HOST_DIR)/usr/bin/python setup.py build \
>> +             --executable=/usr/bin/python)
>
> Maybe use PKG_PYTHON_DISTUTILS_BUILD_OPTS

Agree.

>> +endef
>> +I2C_TOOLS_POST_BUILD_HOOKS += I2C_TOOLS_BUILD_PYSMBUS
>> +
>> +define I2C_TOOLS_INSTALL_PYSMBUS
>> +     (cd $(@D)/py-smbus; \
>> +     $(I2C_TOOLS_PYTHON_BASE_ENV) \
>> +             $(HOST_DIR)/usr/bin/python setup.py install \
>> +             --prefix=$(TARGET_DIR)/usr )
>
> And PKG_PYTHON_DISTUTILS_INSTALL_TARGET_OPTS

Agree.

Will be sending a v2 here shortly.

Thanks,
-Ryan
Ryan Barnett April 5, 2015, 7:22 p.m. UTC | #4
Baruch,

On Sun, Apr 5, 2015 at 2:13 PM, Baruch Siach <baruch@tkos.co.il> wrote:
> Hi Ryan,
>
> On Sun, Apr 05, 2015 at 01:47:55PM -0500, Ryan Barnett wrote:
>> Add a config option to build the python bindings for i2c-tools -
>> py-smbus. The steps for building the python bindings is the same as
>> the distutil steps that are a part of the python infrastructure.
>>
>> Signed-off-by: Ryan Barnett <ryanbarnett3@gmail.com>
>> CC: Tjeerd Pinkert <t.j.pinkert@vu.nl>
>> CC: Zoltan Gyarmati <mr.zoltan.gyarmati@gmail.com>
>> ---
>>  package/i2c-tools/Config.in    | 13 +++++++++++++
>>  package/i2c-tools/i2c-tools.mk | 33 +++++++++++++++++++++++++++++++++
>>  2 files changed, 46 insertions(+)
>>
>> diff --git a/package/i2c-tools/Config.in b/package/i2c-tools/Config.in
>> index e83dbd6..2e537cb 100644
>> --- a/package/i2c-tools/Config.in
>> +++ b/package/i2c-tools/Config.in
>> @@ -8,3 +8,16 @@ config BR2_PACKAGE_I2C_TOOLS
>>         EEPROM decoding scripts, and more.
>>
>>         http://www.lm-sensors.org/wiki/I2CTools
>> +
>> +if BR2_PACKAGE_I2C_TOOLS
>> +
>> +config BR2_PACKAGE_I2C_TOOLS_PYSMBUS
>
> I don't think another config option is necessary. The size of i2c-tools python
> binding is negligible when compared to the size of python itself.

Good point. So do I still keep a comment there about py-smbus not
being enabled unless python is selected? Otherwise someone might not
know that i2c-tools has the python bindings which is why I had the
config option to begin with.

>
>> +     bool "py-smbus"
>> +     depends on BR2_PACKAGE_PYTHON
>
> This seems to indicate that only python2 is supported ...
>
> [...]
>
>> +ifeq ($(BR2_PACKAGE_I2C_TOOLS_PYSMBUS),y)
>> +I2C_TOOLS_DEPENDENCIES += $(if $(BR2_PACKAGE_PYTHON3),host-python3 python3,host-python python)
>
> But this assumes that both python2 and python3 are supported. Which one is
> correct?

I believe both are support so I will need to confirm and update my
config option to support either one.


> Also, there is no need to explicitly list python host variants. Target python
> packages already depend on their corresponding host packages.

Already have implemented based off of Thomas P comments.

Thanks,
-Ryan
Baruch Siach April 5, 2015, 7:35 p.m. UTC | #5
Hi Ryan,

On Sun, Apr 05, 2015 at 02:22:15PM -0500, Ryan Barnett wrote:
> On Sun, Apr 5, 2015 at 2:13 PM, Baruch Siach <baruch@tkos.co.il> wrote:
> > On Sun, Apr 05, 2015 at 01:47:55PM -0500, Ryan Barnett wrote:
> >> +config BR2_PACKAGE_I2C_TOOLS_PYSMBUS
> >
> > I don't think another config option is necessary. The size of i2c-tools python
> > binding is negligible when compared to the size of python itself.
> 
> Good point. So do I still keep a comment there about py-smbus not
> being enabled unless python is selected? Otherwise someone might not
> know that i2c-tools has the python bindings which is why I had the
> config option to begin with.

I think it is reasonable to expect users looking for a python binding to 
enable python itself first.

baruch
diff mbox

Patch

diff --git a/package/i2c-tools/Config.in b/package/i2c-tools/Config.in
index e83dbd6..2e537cb 100644
--- a/package/i2c-tools/Config.in
+++ b/package/i2c-tools/Config.in
@@ -8,3 +8,16 @@  config BR2_PACKAGE_I2C_TOOLS
 	  EEPROM decoding scripts, and more.
 
 	  http://www.lm-sensors.org/wiki/I2CTools
+
+if BR2_PACKAGE_I2C_TOOLS
+
+config BR2_PACKAGE_I2C_TOOLS_PYSMBUS
+	bool "py-smbus"
+	depends on BR2_PACKAGE_PYTHON
+	help
+	  Python bindings to smbus from the i2c-tools package.
+
+comment "i2c-tools py-smbux depends on python"
+	depends on !BR2_PACKAGE_PYTHON
+
+endif
diff --git a/package/i2c-tools/i2c-tools.mk b/package/i2c-tools/i2c-tools.mk
index 0115e22..f363505 100644
--- a/package/i2c-tools/i2c-tools.mk
+++ b/package/i2c-tools/i2c-tools.mk
@@ -21,4 +21,37 @@  define I2C_TOOLS_INSTALL_TARGET_CMDS
 	done
 endef
 
+# BASE_ENV taken from PKG_PYTHON_DISTUTILS_ENV in package/pkg-python.mk
+I2C_TOOLS_PYTHON_BASE_ENV = PATH=$(BR_PATH) \
+	CC="$(TARGET_CC)" \
+	CFLAGS="$(TARGET_CFLAGS) -I../include" \
+	LDFLAGS="$(TARGET_LDFLAGS)" \
+	LDSHARED="$(TARGET_CROSS)gcc -shared" \
+	PYTHONPATH="$(if $(BR2_PACKAGE_PYTHON3),$(PYTHON3_PATH),$(PYTHON_PATH))" \
+	_python_sysroot=$(STAGING_DIR) \
+	_python_prefix=/usr \
+	_python_exec_prefix=/usr
+
+# Build/install steps mirror the distutil python package type in the python package
+# infrastructure
+ifeq ($(BR2_PACKAGE_I2C_TOOLS_PYSMBUS),y)
+I2C_TOOLS_DEPENDENCIES += $(if $(BR2_PACKAGE_PYTHON3),host-python3 python3,host-python python)
+
+define I2C_TOOLS_BUILD_PYSMBUS
+	(cd $(@D)/py-smbus;  \
+	$(I2C_TOOLS_PYTHON_BASE_ENV) \
+		$(HOST_DIR)/usr/bin/python setup.py build \
+		--executable=/usr/bin/python)
+endef
+I2C_TOOLS_POST_BUILD_HOOKS += I2C_TOOLS_BUILD_PYSMBUS
+
+define I2C_TOOLS_INSTALL_PYSMBUS
+	(cd $(@D)/py-smbus; \
+	$(I2C_TOOLS_PYTHON_BASE_ENV) \
+		$(HOST_DIR)/usr/bin/python setup.py install \
+		--prefix=$(TARGET_DIR)/usr )
+endef
+I2C_TOOLS_POST_INSTALL_TARGET_HOOKS += I2C_TOOLS_INSTALL_PYSMBUS
+endif
+
 $(eval $(generic-package))