| Message ID | 20210526000855.17501-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
.Hi Laurent, thank you for the patch On Wed, May 26, 2021 at 9:09 AM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > On gcc versions older than 9, the file system library, used by the > Android camera HAL configuration file parser, is implemented in a > separate static library. Furthermore, on gcc 7, it's provided in the > std::experimental namespace. This breaks compilation of the HAL on gcc > 7, and linking on gcc 8. > > Fix the compilation issue by conditionally including > <experimental/filesystem> and creating a namespace alias in std, and the > link issue by linking to libstdc++fs on gcc versions older than 9. > > The inclusion of <experimental/filesystem> is a bit of a hack, and when > we'll start using the file system library in another compilation unit, > we should then move all this to an internal helper to abstract the > compiler version. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> --- > meson.build | 8 ++++++++ > src/android/camera_hal_config.cpp | 7 +++++++ > 2 files changed, 15 insertions(+) > > diff --git a/meson.build b/meson.build > index 6626fa7ed154..2e7dffb70acc 100644 > --- a/meson.build > +++ b/meson.build > @@ -76,6 +76,14 @@ if cc.get_id() == 'gcc' > error('gcc version is too old, libcamera requires 7.0 or newer') > endif > > + # On gcc 7 and 8, the file system library is provided in a separate > static > + # library. > + if cc.version().version_compare('<9') > + cpp_arguments += [ > + '-lstdc++fs', > + ] > + endif > + > # gcc 7.1 introduced processor-specific ABI breakages related to > parameter > # passing on ARM platforms. This generates a large number of messages > # during compilation with gcc >=7.1. Silence them. > diff --git a/src/android/camera_hal_config.cpp > b/src/android/camera_hal_config.cpp > index d15df2e30c2c..f33ba26935da 100644 > --- a/src/android/camera_hal_config.cpp > +++ b/src/android/camera_hal_config.cpp > @@ -6,7 +6,14 @@ > */ > #include "camera_hal_config.h" > > +#if defined(_GLIBCXX_RELEASE) && _GLIBCXX_RELEASE < 8 > +#include <experimental/filesystem> > +namespace std { > +namespace filesystem = std::experimental::filesystem; > +} > +#else > #include <filesystem> > +#endif > #include <stdio.h> > #include <stdlib.h> > #include <string> > -- > Regards, > > Laurent Pinchart > >
Hi Laurent,
thanks for addressing this.
On Wed, May 26, 2021 at 03:08:53AM +0300, Laurent Pinchart wrote:
> Hello,
>
> This patch series fixes a compilation breakage of the camera HAL with
> gcc 7 and 8 (technically a link breakage with gcc 8), introduced by
> usage of the file system library.
>
> The first patch is a small refactoring, the second patch includes the
> real fix, details are included there.
>
> The compilation fix is a bit of a hack. If we need to use the file
> system library in more places (and I think we will), we should either
> drop support for gcc 7 completely, or abstract file system operations in
> helper classes. Any opinion ?
I think that's the real question. I had a look around, mostly on the
embedded side of the moon, and I haven't found any big clue that gcc7
is still a requirement. In particular I had a look at toolchains
released by bootlin and linaro, and they ship 9 or 10 for all arch,
except bootlin's one for blackfin which is stuck to 6 or 7.
Buildroot has deprecated gcc versions below 8
I can't really find it out for yocto though :(
True that gcc 7.1 has been released only 4 years ago, and 7.5 has
seen a release in 2019, so it's no ancient at all
For completeness, the linux distro panorama looks quite different
though:
Debian 9 stretch still ships 6.3.0 and has moved to 8.3. in Buster.
Fedora has moved to gcc 8 since release 28
Ubuntu 18.04 ships gcc 7.3.0
The Ubuntu LTS has still 1 year before EOL, Debian 9 is not that
ancient (and requires gcc 6), same for fedora 28.
All in all I feel like it might be a bit early to drop support for gcc 7?
Thanks
j
>
> Laurent Pinchart (2):
> android: camera_hal_config: Move include <filesystem> to .cpp file
> android: Fix file system library usage on gcc 7 and 8
>
> meson.build | 8 ++++++++
> src/android/camera_hal_config.cpp | 8 ++++++++
> src/android/camera_hal_config.h | 1 -
> 3 files changed, 16 insertions(+), 1 deletion(-)
>
> --
> Regards,
>
> Laurent Pinchart
>
Hi Laurent, On Wed, May 26, 2021 at 03:08:55AM +0300, Laurent Pinchart wrote: > On gcc versions older than 9, the file system library, used by the > Android camera HAL configuration file parser, is implemented in a > separate static library. Furthermore, on gcc 7, it's provided in the > std::experimental namespace. This breaks compilation of the HAL on gcc > 7, and linking on gcc 8. > > Fix the compilation issue by conditionally including > <experimental/filesystem> and creating a namespace alias in std, and the > link issue by linking to libstdc++fs on gcc versions older than 9. > > The inclusion of <experimental/filesystem> is a bit of a hack, and when > we'll start using the file system library in another compilation unit, > we should then move all this to an internal helper to abstract the > compiler version. Unfortunate, but there are not many ways around it Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > meson.build | 8 ++++++++ > src/android/camera_hal_config.cpp | 7 +++++++ > 2 files changed, 15 insertions(+) > > diff --git a/meson.build b/meson.build > index 6626fa7ed154..2e7dffb70acc 100644 > --- a/meson.build > +++ b/meson.build > @@ -76,6 +76,14 @@ if cc.get_id() == 'gcc' > error('gcc version is too old, libcamera requires 7.0 or newer') > endif > > + # On gcc 7 and 8, the file system library is provided in a separate static > + # library. > + if cc.version().version_compare('<9') > + cpp_arguments += [ > + '-lstdc++fs', > + ] > + endif > + > # gcc 7.1 introduced processor-specific ABI breakages related to parameter > # passing on ARM platforms. This generates a large number of messages > # during compilation with gcc >=7.1. Silence them. > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp > index d15df2e30c2c..f33ba26935da 100644 > --- a/src/android/camera_hal_config.cpp > +++ b/src/android/camera_hal_config.cpp > @@ -6,7 +6,14 @@ > */ > #include "camera_hal_config.h" > > +#if defined(_GLIBCXX_RELEASE) && _GLIBCXX_RELEASE < 8 > +#include <experimental/filesystem> > +namespace std { > +namespace filesystem = std::experimental::filesystem; > +} > +#else > #include <filesystem> > +#endif > #include <stdio.h> > #include <stdlib.h> > #include <string> > -- > Regards, > > Laurent Pinchart >
Hi Laurent, Thanks for your work. On 2021-05-26 03:08:55 +0300, Laurent Pinchart wrote: > On gcc versions older than 9, the file system library, used by the > Android camera HAL configuration file parser, is implemented in a > separate static library. Furthermore, on gcc 7, it's provided in the > std::experimental namespace. This breaks compilation of the HAL on gcc > 7, and linking on gcc 8. > > Fix the compilation issue by conditionally including > <experimental/filesystem> and creating a namespace alias in std, and the > link issue by linking to libstdc++fs on gcc versions older than 9. > > The inclusion of <experimental/filesystem> is a bit of a hack, and when > we'll start using the file system library in another compilation unit, > we should then move all this to an internal helper to abstract the > compiler version. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> This solves my issues, thanks! Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > meson.build | 8 ++++++++ > src/android/camera_hal_config.cpp | 7 +++++++ > 2 files changed, 15 insertions(+) > > diff --git a/meson.build b/meson.build > index 6626fa7ed154..2e7dffb70acc 100644 > --- a/meson.build > +++ b/meson.build > @@ -76,6 +76,14 @@ if cc.get_id() == 'gcc' > error('gcc version is too old, libcamera requires 7.0 or newer') > endif > > + # On gcc 7 and 8, the file system library is provided in a separate static > + # library. > + if cc.version().version_compare('<9') > + cpp_arguments += [ > + '-lstdc++fs', > + ] > + endif > + > # gcc 7.1 introduced processor-specific ABI breakages related to parameter > # passing on ARM platforms. This generates a large number of messages > # during compilation with gcc >=7.1. Silence them. > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp > index d15df2e30c2c..f33ba26935da 100644 > --- a/src/android/camera_hal_config.cpp > +++ b/src/android/camera_hal_config.cpp > @@ -6,7 +6,14 @@ > */ > #include "camera_hal_config.h" > > +#if defined(_GLIBCXX_RELEASE) && _GLIBCXX_RELEASE < 8 > +#include <experimental/filesystem> > +namespace std { > +namespace filesystem = std::experimental::filesystem; > +} > +#else > #include <filesystem> > +#endif > #include <stdio.h> > #include <stdlib.h> > #include <string> > -- > Regards, > > Laurent Pinchart >