| Message ID | 20220726063400.2321251-1-james.hilliard1@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] package/mesa3d: set cpp_rtti option | expand |
James, Al;, On 2022-07-26 00:34 -0600, James Hilliard spake thusly: > This needs to be set based on BR2_PACKAGE_LLVM_RTTI being set. BR2_PACKAGE_LLVM_RTTI is a sub-option of llvm, so it somehow means that llvm should be a dependency of mesa3d, which your patch does not add. But we already have a conditional dependency on llvm, so this nex rtti option should be a sub-condition of BR2_PACKAGE_MESA3D_LLVM? Regards, Yann E. MORIN. > Fixes: > - http://autobuild.buildroot.net/results/e2ebc9a73ed421aa6be44fe41bb5224cc12f699d > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > --- > package/mesa3d/mesa3d.mk | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/package/mesa3d/mesa3d.mk b/package/mesa3d/mesa3d.mk > index 96520e2efd..652b5168d2 100644 > --- a/package/mesa3d/mesa3d.mk > +++ b/package/mesa3d/mesa3d.mk > @@ -94,6 +94,12 @@ else > MESA3D_CONF_OPTS += -Dgallium-vc4-neon=disabled > endif > > +ifeq ($(BR2_PACKAGE_LLVM_RTTI),y) > +MESA3D_CONF_OPTS += -Dcpp_rtti=true > +else > +MESA3D_CONF_OPTS += -Dcpp_rtti=false > +endif > + > # Drivers > > #Gallium Drivers > -- > 2.34.1 > > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
On Tue, Jul 26, 2022 at 2:47 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > James, Al;, > > On 2022-07-26 00:34 -0600, James Hilliard spake thusly: > > This needs to be set based on BR2_PACKAGE_LLVM_RTTI being set. > > BR2_PACKAGE_LLVM_RTTI is a sub-option of llvm, so it somehow means > that llvm should be a dependency of mesa3d, which your patch does not > add. It's an optional dependency. > > But we already have a conditional dependency on llvm, so this nex rtti > option should be a sub-condition of BR2_PACKAGE_MESA3D_LLVM? Setting -Dcpp_rtti=false when llvm isn't used is harmless, it seemed clearer to just have the single independent conditional for BR2_PACKAGE_LLVM_RTTI. > > Regards, > Yann E. MORIN. > > > Fixes: > > - http://autobuild.buildroot.net/results/e2ebc9a73ed421aa6be44fe41bb5224cc12f699d > > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > --- > > package/mesa3d/mesa3d.mk | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/package/mesa3d/mesa3d.mk b/package/mesa3d/mesa3d.mk > > index 96520e2efd..652b5168d2 100644 > > --- a/package/mesa3d/mesa3d.mk > > +++ b/package/mesa3d/mesa3d.mk > > @@ -94,6 +94,12 @@ else > > MESA3D_CONF_OPTS += -Dgallium-vc4-neon=disabled > > endif > > > > +ifeq ($(BR2_PACKAGE_LLVM_RTTI),y) > > +MESA3D_CONF_OPTS += -Dcpp_rtti=true > > +else > > +MESA3D_CONF_OPTS += -Dcpp_rtti=false > > +endif > > + > > # Drivers > > > > #Gallium Drivers > > -- > > 2.34.1 > > > > _______________________________________________ > > buildroot mailing list > > buildroot@buildroot.org > > https://lists.buildroot.org/mailman/listinfo/buildroot > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------'
James, All, On 2022-07-26 09:31 -0600, James Hilliard spake thusly: > On Tue, Jul 26, 2022 at 2:47 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > On 2022-07-26 00:34 -0600, James Hilliard spake thusly: > > > This needs to be set based on BR2_PACKAGE_LLVM_RTTI being set. > > BR2_PACKAGE_LLVM_RTTI is a sub-option of llvm, so it somehow means > > that llvm should be a dependency of mesa3d, which your patch does not > > add. > It's an optional dependency. I know. But how does it make sense to add BR2_PACKAGE_LLVM_RTTI without a dependency on llvm? And note that mesa3d does not use the fact that llvm is enabled, to enable its llvm support; one has to explicitly request it with BR2_PACKAGE_MESA3D_LLVM. > > But we already have a conditional dependency on llvm, so this nex rtti > > option should be a sub-condition of BR2_PACKAGE_MESA3D_LLVM? > Setting -Dcpp_rtti=false when llvm isn't used is harmless, it seemed clearer > to just have the single independent conditional for BR2_PACKAGE_LLVM_RTTI. If you have a configuration with: BR2_PACKAGE_LLVM=y BR2_PACKAGE_LLVM_RTTI=y BR2_PACKAGE_MESA3D=y # BR2_PACKAGE_MESA3D_LLVM is unset Then llvm support is not built in mesa3d, yet your code would cause us to pass -Dcpp_rtti=true, which does not seem to make sense. So, I think we want that to move to the existing condition, like so: diff --git a/package/mesa3d/mesa3d.mk b/package/mesa3d/mesa3d.mk index 96520e2efd..980faced31 100644 --- a/package/mesa3d/mesa3d.mk +++ b/package/mesa3d/mesa3d.mk @@ -48,6 +48,11 @@ ifeq ($(BR2_PACKAGE_MESA3D_LLVM),y) MESA3D_DEPENDENCIES += host-llvm llvm MESA3D_MESON_EXTRA_BINARIES += llvm-config='$(STAGING_DIR)/usr/bin/llvm-config' MESA3D_CONF_OPTS += -Dllvm=enabled +ifeq ($(BR2_PACKAGE_LLVM_RTTI),y) +MESA3D_CONF_OPTS += -Dcpp_rtti=true +else +MESA3D_CONF_OPTS += -Dcpp_rtti=false +endif else # Avoid automatic search of llvm-config MESA3D_CONF_OPTS += -Dllvm=disabled Regards, Yann E. MORIN.
On Tue, Jul 26, 2022 at 10:28 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > James, All, > > On 2022-07-26 09:31 -0600, James Hilliard spake thusly: > > On Tue, Jul 26, 2022 at 2:47 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > > On 2022-07-26 00:34 -0600, James Hilliard spake thusly: > > > > This needs to be set based on BR2_PACKAGE_LLVM_RTTI being set. > > > BR2_PACKAGE_LLVM_RTTI is a sub-option of llvm, so it somehow means > > > that llvm should be a dependency of mesa3d, which your patch does not > > > add. > > It's an optional dependency. > > I know. But how does it make sense to add BR2_PACKAGE_LLVM_RTTI without > a dependency on llvm? > > And note that mesa3d does not use the fact that llvm is enabled, to > enable its llvm support; one has to explicitly request it with > BR2_PACKAGE_MESA3D_LLVM. > > > > But we already have a conditional dependency on llvm, so this nex rtti > > > option should be a sub-condition of BR2_PACKAGE_MESA3D_LLVM? > > Setting -Dcpp_rtti=false when llvm isn't used is harmless, it seemed clearer > > to just have the single independent conditional for BR2_PACKAGE_LLVM_RTTI. > > If you have a configuration with: > > BR2_PACKAGE_LLVM=y > BR2_PACKAGE_LLVM_RTTI=y > BR2_PACKAGE_MESA3D=y > # BR2_PACKAGE_MESA3D_LLVM is unset > > Then llvm support is not built in mesa3d, yet your code would cause us > to pass -Dcpp_rtti=true, which does not seem to make sense. I think I did it this way in case llvm gets pulled in as a transient dependency down the line. I mean -Dcpp_rtti=true is still technically accurate when BR2_PACKAGE_LLVM_RTTI=y even if BR2_PACKAGE_MESA3D_LLVM isn't set, it's just unused internally by mesa3d in that case I think. Having it separate from BR2_PACKAGE_MESA3D_LLVM seems to make things less likely to break in the event of llvm getting pulled in as a transient dependency in the future. > > So, I think we want that to move to the existing condition, like so: > > diff --git a/package/mesa3d/mesa3d.mk b/package/mesa3d/mesa3d.mk > index 96520e2efd..980faced31 100644 > --- a/package/mesa3d/mesa3d.mk > +++ b/package/mesa3d/mesa3d.mk > @@ -48,6 +48,11 @@ ifeq ($(BR2_PACKAGE_MESA3D_LLVM),y) > MESA3D_DEPENDENCIES += host-llvm llvm > MESA3D_MESON_EXTRA_BINARIES += llvm-config='$(STAGING_DIR)/usr/bin/llvm-config' > MESA3D_CONF_OPTS += -Dllvm=enabled > +ifeq ($(BR2_PACKAGE_LLVM_RTTI),y) > +MESA3D_CONF_OPTS += -Dcpp_rtti=true > +else > +MESA3D_CONF_OPTS += -Dcpp_rtti=false > +endif > else > # Avoid automatic search of llvm-config > MESA3D_CONF_OPTS += -Dllvm=disabled > > Regards, > Yann E. MORIN. > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------'
On Tue, Jul 26, 2022 at 11:18 AM James Hilliard <james.hilliard1@gmail.com> wrote: > > On Tue, Jul 26, 2022 at 10:28 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > > > James, All, > > > > On 2022-07-26 09:31 -0600, James Hilliard spake thusly: > > > On Tue, Jul 26, 2022 at 2:47 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > > > On 2022-07-26 00:34 -0600, James Hilliard spake thusly: > > > > > This needs to be set based on BR2_PACKAGE_LLVM_RTTI being set. > > > > BR2_PACKAGE_LLVM_RTTI is a sub-option of llvm, so it somehow means > > > > that llvm should be a dependency of mesa3d, which your patch does not > > > > add. > > > It's an optional dependency. > > > > I know. But how does it make sense to add BR2_PACKAGE_LLVM_RTTI without > > a dependency on llvm? > > > > And note that mesa3d does not use the fact that llvm is enabled, to > > enable its llvm support; one has to explicitly request it with > > BR2_PACKAGE_MESA3D_LLVM. > > > > > > But we already have a conditional dependency on llvm, so this nex rtti > > > > option should be a sub-condition of BR2_PACKAGE_MESA3D_LLVM? > > > Setting -Dcpp_rtti=false when llvm isn't used is harmless, it seemed clearer > > > to just have the single independent conditional for BR2_PACKAGE_LLVM_RTTI. > > > > If you have a configuration with: > > > > BR2_PACKAGE_LLVM=y > > BR2_PACKAGE_LLVM_RTTI=y > > BR2_PACKAGE_MESA3D=y > > # BR2_PACKAGE_MESA3D_LLVM is unset > > > > Then llvm support is not built in mesa3d, yet your code would cause us > > to pass -Dcpp_rtti=true, which does not seem to make sense. > > I think I did it this way in case llvm gets pulled in as a transient dependency > down the line. > > I mean -Dcpp_rtti=true is still technically accurate when > BR2_PACKAGE_LLVM_RTTI=y even if BR2_PACKAGE_MESA3D_LLVM isn't > set, it's just unused internally by mesa3d in that case I think. > > Having it separate from BR2_PACKAGE_MESA3D_LLVM seems to make > things less likely to break in the event of llvm getting pulled in as > a transient > dependency in the future. Sent a v2 with it under BR2_PACKAGE_MESA3D_LLVM if that's preferable: https://patchwork.ozlabs.org/project/buildroot/patch/20220914230334.4001572-1-james.hilliard1@gmail.com/ > > > > > So, I think we want that to move to the existing condition, like so: > > > > diff --git a/package/mesa3d/mesa3d.mk b/package/mesa3d/mesa3d.mk > > index 96520e2efd..980faced31 100644 > > --- a/package/mesa3d/mesa3d.mk > > +++ b/package/mesa3d/mesa3d.mk > > @@ -48,6 +48,11 @@ ifeq ($(BR2_PACKAGE_MESA3D_LLVM),y) > > MESA3D_DEPENDENCIES += host-llvm llvm > > MESA3D_MESON_EXTRA_BINARIES += llvm-config='$(STAGING_DIR)/usr/bin/llvm-config' > > MESA3D_CONF_OPTS += -Dllvm=enabled > > +ifeq ($(BR2_PACKAGE_LLVM_RTTI),y) > > +MESA3D_CONF_OPTS += -Dcpp_rtti=true > > +else > > +MESA3D_CONF_OPTS += -Dcpp_rtti=false > > +endif > > else > > # Avoid automatic search of llvm-config > > MESA3D_CONF_OPTS += -Dllvm=disabled > > > > Regards, > > Yann E. MORIN. > > > > -- > > .-----------------.--------------------.------------------.--------------------. > > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | > > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > > '------------------------------^-------^------------------^--------------------'
diff --git a/package/mesa3d/mesa3d.mk b/package/mesa3d/mesa3d.mk index 96520e2efd..652b5168d2 100644 --- a/package/mesa3d/mesa3d.mk +++ b/package/mesa3d/mesa3d.mk @@ -94,6 +94,12 @@ else MESA3D_CONF_OPTS += -Dgallium-vc4-neon=disabled endif +ifeq ($(BR2_PACKAGE_LLVM_RTTI),y) +MESA3D_CONF_OPTS += -Dcpp_rtti=true +else +MESA3D_CONF_OPTS += -Dcpp_rtti=false +endif + # Drivers #Gallium Drivers
This needs to be set based on BR2_PACKAGE_LLVM_RTTI being set. Fixes: - http://autobuild.buildroot.net/results/e2ebc9a73ed421aa6be44fe41bb5224cc12f699d Signed-off-by: James Hilliard <james.hilliard1@gmail.com> --- package/mesa3d/mesa3d.mk | 6 ++++++ 1 file changed, 6 insertions(+)