| Message ID | 20191028111556.245007-1-titouan.christophe@railnova.eu |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/1] package/gst1-validate: allow to use host-python3 and target python3 | expand |
On 28/10/2019 12:15, Titouan Christophe wrote: > Signed-off-by: Titouan Christophe <titouan.christophe@railnova.eu> > --- > package/gstreamer1/gst1-validate/Config.in | 6 ++++-- > package/gstreamer1/gst1-validate/gst1-validate.mk | 8 ++++++-- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/package/gstreamer1/gst1-validate/Config.in b/package/gstreamer1/gst1-validate/Config.in > index 2022d38d99..63bce613ed 100644 > --- a/package/gstreamer1/gst1-validate/Config.in > +++ b/package/gstreamer1/gst1-validate/Config.in > @@ -1,9 +1,10 @@ > config BR2_PACKAGE_GST1_VALIDATE > bool "gst1-validate" > - depends on BR2_PACKAGE_PYTHON > + depends on BR2_PACKAGE_PYTHON || BR2_PACKAGE_PYTHON3 Not directly related to this patch, but it would be better to use a select instead of a depends here. 'depends' is for packages that are really Python-related. 'select' is for when it's a package that just happens to be implemented in python, like e.g. dstat. The boundary is sometimes a bit thin, but I think in this case it's clearly the latter. But anyway, that's a separate patch. > select BR2_PACKAGE_GST1_PLUGINS_BASE > select BR2_PACKAGE_JSON_GLIB > - select BR2_PACKAGE_PYTHON_PYEXPAT > + select BR2_PACKAGE_PYTHON_PYEXPAT if BR2_PACKAGE_PYTHON > + select BR2_PACKAGE_PYTHON3_PYEXPAT if BR2_PACKAGE_PYTHON3 There is no python3-pyexpat package. Target packages will automatically use the python3 version if python3 is selected for the target. It is only when a python3-only host package depends on some module that we need those python3-foo packages. > # cairo is autodetected but needs PNG support > select BR2_PACKAGE_CAIRO_PNG if BR2_PACKAGE_CAIRO > help > @@ -15,3 +16,4 @@ config BR2_PACKAGE_GST1_VALIDATE > > comment "gst1-validate depends on python" > depends on !BR2_PACKAGE_PYTHON > + depends on !BR2_PACKAGE_PYTHON3 Although correct, we typically write this as depends on !(BR2_PACKAGE_PYTHON || BR2_PACKAGE_PYTHON3) Regards, Arnout > diff --git a/package/gstreamer1/gst1-validate/gst1-validate.mk b/package/gstreamer1/gst1-validate/gst1-validate.mk > index da38aeb497..e9522e1568 100644 > --- a/package/gstreamer1/gst1-validate/gst1-validate.mk > +++ b/package/gstreamer1/gst1-validate/gst1-validate.mk > @@ -14,10 +14,14 @@ GST1_VALIDATE_DEPENDENCIES = \ > gstreamer1 \ > gst1-plugins-base \ > json-glib \ > - host-python \ > - python \ > $(if $(BR2_PACKAGE_CAIRO),cairo) > > +ifeq ($(BR2_PACKAGE_PYTHON3),y) > +GST1_VALIDATE_DEPENDENCIES += host-python3 python3 > +else > +GST1_VALIDATE_DEPENDENCIES += host-python python > +endif > + > GST1_VALIDATE_CONF_OPTS += --disable-sphinx-doc > > $(eval $(autotools-package)) >
Good evening Arnout, On 10/28/19 11:35 PM, Arnout Vandecappelle wrote: > > > On 28/10/2019 12:15, Titouan Christophe wrote: >> +++ b/package/gstreamer1/gst1-validate/Config.in >> @@ -1,9 +1,10 @@ >> config BR2_PACKAGE_GST1_VALIDATE >> bool "gst1-validate" >> - depends on BR2_PACKAGE_PYTHON >> + depends on BR2_PACKAGE_PYTHON || BR2_PACKAGE_PYTHON3 > > Not directly related to this patch, but it would be better to use a select > instead of a depends here. 'depends' is for packages that are really > Python-related. 'select' is for when it's a package that just happens to be > implemented in python, like e.g. dstat. The boundary is sometimes a bit thin, > but I think in this case it's clearly the latter. But anyway, that's a separate > patch. Ok > >> select BR2_PACKAGE_GST1_PLUGINS_BASE >> select BR2_PACKAGE_JSON_GLIB >> - select BR2_PACKAGE_PYTHON_PYEXPAT >> + select BR2_PACKAGE_PYTHON_PYEXPAT if BR2_PACKAGE_PYTHON >> + select BR2_PACKAGE_PYTHON3_PYEXPAT if BR2_PACKAGE_PYTHON3 > > There is no python3-pyexpat package. Target packages will automatically use the > python3 version if python3 is selected for the target. It is only when a > python3-only host package depends on some module that we need those python3-foo > packages. Then, there is something I probably do not understand well: $ git grep BR2_PACKAGE_PYTHON3_PYEXPAT | wc -l 20 > >> # cairo is autodetected but needs PNG support >> select BR2_PACKAGE_CAIRO_PNG if BR2_PACKAGE_CAIRO >> help >> @@ -15,3 +16,4 @@ config BR2_PACKAGE_GST1_VALIDATE >> >> comment "gst1-validate depends on python" >> depends on !BR2_PACKAGE_PYTHON >> + depends on !BR2_PACKAGE_PYTHON3 > > Although correct, we typically write this as > > depends on !(BR2_PACKAGE_PYTHON || BR2_PACKAGE_PYTHON3) I already submitted some accepted patche(s?) that had the 2 "depends on" form, for instance supervisor. Is there a style guide somewhere I could follow ? (IMHO, the 2 lines form is prettier and easier to understand for humans and diffs, but this is definitely a matter of taste </my2cents>) > > Regards, > Arnout Best regards, Titouan
On 29/10/2019 00:19, Titouan Christophe wrote: > Good evening Arnout, > > > On 10/28/19 11:35 PM, Arnout Vandecappelle wrote: >> >> >> On 28/10/2019 12:15, Titouan Christophe wrote: >>> +++ b/package/gstreamer1/gst1-validate/Config.in >>> @@ -1,9 +1,10 @@ >>> config BR2_PACKAGE_GST1_VALIDATE >>> bool "gst1-validate" >>> - depends on BR2_PACKAGE_PYTHON >>> + depends on BR2_PACKAGE_PYTHON || BR2_PACKAGE_PYTHON3 >> >> Not directly related to this patch, but it would be better to use a select >> instead of a depends here. 'depends' is for packages that are really >> Python-related. 'select' is for when it's a package that just happens to be >> implemented in python, like e.g. dstat. The boundary is sometimes a bit thin, >> but I think in this case it's clearly the latter. But anyway, that's a separate >> patch. > > Ok > >> >>> select BR2_PACKAGE_GST1_PLUGINS_BASE >>> select BR2_PACKAGE_JSON_GLIB >>> - select BR2_PACKAGE_PYTHON_PYEXPAT >>> + select BR2_PACKAGE_PYTHON_PYEXPAT if BR2_PACKAGE_PYTHON >>> + select BR2_PACKAGE_PYTHON3_PYEXPAT if BR2_PACKAGE_PYTHON3 >> >> There is no python3-pyexpat package. Target packages will automatically use the >> python3 version if python3 is selected for the target. It is only when a >> python3-only host package depends on some module that we need those python3-foo >> packages. > > Then, there is something I probably do not understand well: > > $ git grep BR2_PACKAGE_PYTHON3_PYEXPAT | wc -l > 20 That is my stupidity. I didn't find a python3-pyexpat package. But that's because it's not a separate package, but rather a suboption of python/python3. So your patch was perfectly fine. >>> # cairo is autodetected but needs PNG support >>> select BR2_PACKAGE_CAIRO_PNG if BR2_PACKAGE_CAIRO >>> help >>> @@ -15,3 +16,4 @@ config BR2_PACKAGE_GST1_VALIDATE >>> comment "gst1-validate depends on python" >>> depends on !BR2_PACKAGE_PYTHON >>> + depends on !BR2_PACKAGE_PYTHON3 >> >> Although correct, we typically write this as >> >> depends on !(BR2_PACKAGE_PYTHON || BR2_PACKAGE_PYTHON3) > > I already submitted some accepted patche(s?) that had the 2 "depends on" form, > for instance supervisor. Is there a style guide somewhere I could follow ? > > (IMHO, the 2 lines form is prettier and easier to understand for humans and > diffs, but this is definitely a matter of taste </my2cents>) In matters of taste we aim for consistency. But that makes my proposal wrong as well, because in two places we have: depends on !BR2_PACKAGE_PYTHON && !BR2_PACKAGE_PYTHON3 (which IMO is the worst option: it's less readable than the two lines that you have, and it's not an exact negation of the dependency line in the package itself.) Since your patch was perfectly OK, I've now applied to master, thanks. Regards, Arnout
diff --git a/package/gstreamer1/gst1-validate/Config.in b/package/gstreamer1/gst1-validate/Config.in index 2022d38d99..63bce613ed 100644 --- a/package/gstreamer1/gst1-validate/Config.in +++ b/package/gstreamer1/gst1-validate/Config.in @@ -1,9 +1,10 @@ config BR2_PACKAGE_GST1_VALIDATE bool "gst1-validate" - depends on BR2_PACKAGE_PYTHON + depends on BR2_PACKAGE_PYTHON || BR2_PACKAGE_PYTHON3 select BR2_PACKAGE_GST1_PLUGINS_BASE select BR2_PACKAGE_JSON_GLIB - select BR2_PACKAGE_PYTHON_PYEXPAT + select BR2_PACKAGE_PYTHON_PYEXPAT if BR2_PACKAGE_PYTHON + select BR2_PACKAGE_PYTHON3_PYEXPAT if BR2_PACKAGE_PYTHON3 # cairo is autodetected but needs PNG support select BR2_PACKAGE_CAIRO_PNG if BR2_PACKAGE_CAIRO help @@ -15,3 +16,4 @@ config BR2_PACKAGE_GST1_VALIDATE comment "gst1-validate depends on python" depends on !BR2_PACKAGE_PYTHON + depends on !BR2_PACKAGE_PYTHON3 diff --git a/package/gstreamer1/gst1-validate/gst1-validate.mk b/package/gstreamer1/gst1-validate/gst1-validate.mk index da38aeb497..e9522e1568 100644 --- a/package/gstreamer1/gst1-validate/gst1-validate.mk +++ b/package/gstreamer1/gst1-validate/gst1-validate.mk @@ -14,10 +14,14 @@ GST1_VALIDATE_DEPENDENCIES = \ gstreamer1 \ gst1-plugins-base \ json-glib \ - host-python \ - python \ $(if $(BR2_PACKAGE_CAIRO),cairo) +ifeq ($(BR2_PACKAGE_PYTHON3),y) +GST1_VALIDATE_DEPENDENCIES += host-python3 python3 +else +GST1_VALIDATE_DEPENDENCIES += host-python python +endif + GST1_VALIDATE_CONF_OPTS += --disable-sphinx-doc $(eval $(autotools-package))
Signed-off-by: Titouan Christophe <titouan.christophe@railnova.eu> --- package/gstreamer1/gst1-validate/Config.in | 6 ++++-- package/gstreamer1/gst1-validate/gst1-validate.mk | 8 ++++++-- 2 files changed, 10 insertions(+), 4 deletions(-)