| Message ID | 1428259675-7116-1-git-send-email-ryanbarnett3@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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
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
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 --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))
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(+)