| Message ID | 20201027174046.17157-2-buildroot@heine.tech |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | fix libabseil-cpp build on older gcc versions | expand |
On Tue, 27 Oct 2020 18:40:44 +0100 Michael Nosthoff via buildroot <buildroot@busybox.net> wrote: > From: Michael Nosthoff <buildroot@heine.tech> > > For gcc < 6 the c++ default is 98. Abseil needs at least 11 > but doesn't enforce this anymore. So we need to do this. > > Signed-off-by: Michael Nosthoff <buildroot@heine.tech> Is this fixing a build issue? What was the thing that made you write this patch? > +# abseil needs c++ >= 11 but doesn't set this anymore, > +# before gcc6 the default is 98. > +ifeq ($(BR2_GCC_AT_LEAST_6_0),) > +LIBABSEIL_CPP_CONF_OPTS += -DCMAKE_CXX_STANDARD=11 > +endif > + > +ifeq ($(BR2_HOST_GCC_AT_LEAST_6_0),) > +HOST_LIBABSEIL_CPP_CONF_OPTS += -DCMAKE_CXX_STANDARD=11 > +endif Can we do it unconditionally ? Thanks, Thomas
Hi, On Thursday, October 29, 2020 23:36 CET, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > On Tue, 27 Oct 2020 18:40:44 +0100 > Michael Nosthoff via buildroot <buildroot@busybox.net> wrote: > > > From: Michael Nosthoff <buildroot@heine.tech> > > > > For gcc < 6 the c++ default is 98. Abseil needs at least 11 > > but doesn't enforce this anymore. So we need to do this. > > > > Signed-off-by: Michael Nosthoff <buildroot@heine.tech> > > Is this fixing a build issue? What was the thing that made you write > this patch? > I had the links to the broken autobuilders in the Cover letter. Should I put them in the commit instead? > > +# abseil needs c++ >= 11 but doesn't set this anymore, > > +# before gcc6 the default is 98. > > +ifeq ($(BR2_GCC_AT_LEAST_6_0),) > > +LIBABSEIL_CPP_CONF_OPTS += -DCMAKE_CXX_STANDARD=11 > > +endif > > + > > +ifeq ($(BR2_HOST_GCC_AT_LEAST_6_0),) > > +HOST_LIBABSEIL_CPP_CONF_OPTS += -DCMAKE_CXX_STANDARD=11 > > +endif > > Can we do it unconditionally ? Sure, that was the fallback before (In the CMakeLists). And grpc also just sets C++11 as fallback. I just thought that, as abseil is all about "bringing newer C++ features to older C++ versions", it would make sense to build it with C++14/17 when it's the compilers default. But as grpc is also built with "only" C++11 this might not be the best idea in the first place. Regards, Michael
On Fri, 30 Oct 2020 08:44:06 +0100 "Michael Nosthoff" <buildroot@heine.tech> wrote: > > Is this fixing a build issue? What was the thing that made you write > > this patch? > > I had the links to the broken autobuilders in the Cover letter. Should I put them in the commit instead? Sorry, I had missed the cover letters, since I'm mainly looking at patches through patchwork. And yes, the link to the autobuilder failure should be in the commit log, like this: Fixes: http://autobuild.buildroot.org/... > > > +# abseil needs c++ >= 11 but doesn't set this anymore, > > > +# before gcc6 the default is 98. > > > +ifeq ($(BR2_GCC_AT_LEAST_6_0),) > > > +LIBABSEIL_CPP_CONF_OPTS += -DCMAKE_CXX_STANDARD=11 > > > +endif > > > + > > > +ifeq ($(BR2_HOST_GCC_AT_LEAST_6_0),) > > > +HOST_LIBABSEIL_CPP_CONF_OPTS += -DCMAKE_CXX_STANDARD=11 > > > +endif > > > > Can we do it unconditionally ? > > Sure, that was the fallback before (In the CMakeLists). And grpc also just sets C++11 as fallback. > > I just thought that, as abseil is all about "bringing newer C++ > features to older C++ versions", it would make sense to build it with > C++14/17 when it's the compilers default. But as grpc is also built > with "only" C++11 this might not be the best idea in the first place. In fact, let's turn this around: if abseil uses C++11 features, then it should by itself pass -std=c++11 to the compiler, by setting the appropriate CXX_STANDARD in its CMakeLists.txt. There is no reason Buildroot should have to help here. So isn't an abseil CMakeLists.txt fix (submitted upstream) more appropriate here ? Thanks! Thomas
Hi, On Friday, October 30, 2020 09:15 CET, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > In fact, let's turn this around: if abseil uses C++11 features, then it > should by itself pass -std=c++11 to the compiler, by setting the > appropriate CXX_STANDARD in its CMakeLists.txt. There is no reason > Buildroot should have to help here. So isn't an abseil CMakeLists.txt > fix (submitted upstream) more appropriate here ? > Actually this was in abseil and they removed it intentionally in [0]. So I don't think they might have interest in picking up a patch that would reintroduce that... Regards, Michael [0] https://github.com/abseil/abseil-cpp/commit/c6954897f7ece5011f0126db9117361dc1a6ff36#
On Fri, 30 Oct 2020 09:47:42 +0100 "Michael Nosthoff" <buildroot@heine.tech> wrote: > Actually this was in abseil and they removed it intentionally in [0]. > So I don't think they might have interest in picking up a patch that would reintroduce that... Meh, ok. Then, so be it, your patch is the best we can do I guess. Thomas
diff --git a/package/libabseil-cpp/libabseil-cpp.mk b/package/libabseil-cpp/libabseil-cpp.mk index dfcf892f65..8490b6d948 100644 --- a/package/libabseil-cpp/libabseil-cpp.mk +++ b/package/libabseil-cpp/libabseil-cpp.mk @@ -20,5 +20,15 @@ HOST_LIBABSEIL_CPP_CONF_OPTS = \ -DABSL_USE_GOOGLETEST_HEAD=OFF \ -DABSL_RUN_TESTS=OFF +# abseil needs c++ >= 11 but doesn't set this anymore, +# before gcc6 the default is 98. +ifeq ($(BR2_GCC_AT_LEAST_6_0),) +LIBABSEIL_CPP_CONF_OPTS += -DCMAKE_CXX_STANDARD=11 +endif + +ifeq ($(BR2_HOST_GCC_AT_LEAST_6_0),) +HOST_LIBABSEIL_CPP_CONF_OPTS += -DCMAKE_CXX_STANDARD=11 +endif + $(eval $(cmake-package)) $(eval $(host-cmake-package))