| Message ID | 20241001102810.479285-3-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, Thank you for the patch. On Tue, Oct 01, 2024 at 12:27:54PM +0200, Milan Zamazal wrote: > Global configuration, which uses the YAML parser, will be in base. Thus > YAML parser must be moved to base too. The global configuration file seems quite specific to libcamera to me. Why do you think it should be in base ? Is that maybe because we haven't really defined what base is ? > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/base/meson.build | 14 ++++++++++++++ > src/libcamera/{ => base}/yaml_parser.cpp | 0 > src/libcamera/meson.build | 14 -------------- > 3 files changed, 14 insertions(+), 14 deletions(-) > rename src/libcamera/{ => base}/yaml_parser.cpp (100%) > > diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build > index a742dfdfe..94843eb95 100644 > --- a/src/libcamera/base/meson.build > +++ b/src/libcamera/base/meson.build > @@ -24,10 +24,12 @@ libcamera_base_internal_sources = files([ > 'thread.cpp', > 'timer.cpp', > 'utils.cpp', > + 'yaml_parser.cpp', > ]) > > libdw = dependency('libdw', required : false) > libunwind = dependency('libunwind', required : false) > +libyaml = dependency('yaml-0.1', required : false) > > if cc.has_header_symbol('execinfo.h', 'backtrace') > config_h.set('HAVE_BACKTRACE', 1) > @@ -41,11 +43,23 @@ if libunwind.found() > config_h.set('HAVE_UNWIND', 1) > endif > > +# Fallback to a subproject if libyaml isn't found, as it's not packaged in AOSP. > +if not libyaml.found() > + cmake = import('cmake') > + > + libyaml_vars = cmake.subproject_options() > + libyaml_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE': 'ON'}) > + libyaml_vars.append_compile_args('c', '-Wno-unused-value') > + libyaml_wrap = cmake.subproject('libyaml', options : libyaml_vars) > + libyaml = libyaml_wrap.dependency('yaml') > +endif > + > libcamera_base_deps = [ > libatomic, > libdw, > libthreads, > libunwind, > + libyaml, > ] > > # Internal components must use the libcamera_base_private dependency to enable > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/base/yaml_parser.cpp > similarity index 100% > rename from src/libcamera/yaml_parser.cpp > rename to src/libcamera/base/yaml_parser.cpp > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > index aa9ab0291..736cc496f 100644 > --- a/src/libcamera/meson.build > +++ b/src/libcamera/meson.build > @@ -51,7 +51,6 @@ libcamera_internal_sources = files([ > 'v4l2_pixelformat.cpp', > 'v4l2_subdevice.cpp', > 'v4l2_videodevice.cpp', > - 'yaml_parser.cpp', > ]) > > includes = [ > @@ -79,7 +78,6 @@ if not cc.has_function('dlopen') > libdl = cc.find_library('dl') > endif > libudev = dependency('libudev', required : get_option('udev')) > -libyaml = dependency('yaml-0.1', required : false) > > # Use one of gnutls or libcrypto (provided by OpenSSL), trying gnutls first. > libcrypto = dependency('gnutls', required : false) > @@ -115,17 +113,6 @@ if libudev.found() > ]) > endif > > -# Fallback to a subproject if libyaml isn't found, as it's not packaged in AOSP. > -if not libyaml.found() > - cmake = import('cmake') > - > - libyaml_vars = cmake.subproject_options() > - libyaml_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE': 'ON'}) > - libyaml_vars.append_compile_args('c', '-Wno-unused-value') > - libyaml_wrap = cmake.subproject('libyaml', options : libyaml_vars) > - libyaml = libyaml_wrap.dependency('yaml') > -endif > - > control_sources = [] > > controls_mode_files = { > @@ -185,7 +172,6 @@ libcamera_deps += [ > libdl, > liblttng, > libudev, > - libyaml, > ] > > # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.
Hi Laurent, Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > Thank you for the patch. > > On Tue, Oct 01, 2024 at 12:27:54PM +0200, Milan Zamazal wrote: >> Global configuration, which uses the YAML parser, will be in base. Thus >> YAML parser must be moved to base too. > > The global configuration file seems quite specific to libcamera to me. > Why do you think it should be in base ? Because logging is in base and uses the configuration. If logging configuration was omitted from the global configuration, the global configuration could be put elsewhere and also the annoying chicken-egg problem when reading and parsing the configuration while possibly logging about it would be avoided. But from the user's point of view I think logging should be configurable in the file. > Is that maybe because we haven't really defined what base is ? > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/base/meson.build | 14 ++++++++++++++ >> src/libcamera/{ => base}/yaml_parser.cpp | 0 >> src/libcamera/meson.build | 14 -------------- >> 3 files changed, 14 insertions(+), 14 deletions(-) >> rename src/libcamera/{ => base}/yaml_parser.cpp (100%) >> >> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build >> index a742dfdfe..94843eb95 100644 >> --- a/src/libcamera/base/meson.build >> +++ b/src/libcamera/base/meson.build >> @@ -24,10 +24,12 @@ libcamera_base_internal_sources = files([ >> 'thread.cpp', >> 'timer.cpp', >> 'utils.cpp', >> + 'yaml_parser.cpp', >> ]) >> >> libdw = dependency('libdw', required : false) >> libunwind = dependency('libunwind', required : false) >> +libyaml = dependency('yaml-0.1', required : false) >> >> if cc.has_header_symbol('execinfo.h', 'backtrace') >> config_h.set('HAVE_BACKTRACE', 1) >> @@ -41,11 +43,23 @@ if libunwind.found() >> config_h.set('HAVE_UNWIND', 1) >> endif >> >> +# Fallback to a subproject if libyaml isn't found, as it's not packaged in AOSP. >> +if not libyaml.found() >> + cmake = import('cmake') >> + >> + libyaml_vars = cmake.subproject_options() >> + libyaml_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE': 'ON'}) >> + libyaml_vars.append_compile_args('c', '-Wno-unused-value') >> + libyaml_wrap = cmake.subproject('libyaml', options : libyaml_vars) >> + libyaml = libyaml_wrap.dependency('yaml') >> +endif >> + >> libcamera_base_deps = [ >> libatomic, >> libdw, >> libthreads, >> libunwind, >> + libyaml, >> ] >> >> # Internal components must use the libcamera_base_private dependency to enable >> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/base/yaml_parser.cpp >> similarity index 100% >> rename from src/libcamera/yaml_parser.cpp >> rename to src/libcamera/base/yaml_parser.cpp >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build >> index aa9ab0291..736cc496f 100644 >> --- a/src/libcamera/meson.build >> +++ b/src/libcamera/meson.build >> @@ -51,7 +51,6 @@ libcamera_internal_sources = files([ >> 'v4l2_pixelformat.cpp', >> 'v4l2_subdevice.cpp', >> 'v4l2_videodevice.cpp', >> - 'yaml_parser.cpp', >> ]) >> >> includes = [ >> @@ -79,7 +78,6 @@ if not cc.has_function('dlopen') >> libdl = cc.find_library('dl') >> endif >> libudev = dependency('libudev', required : get_option('udev')) >> -libyaml = dependency('yaml-0.1', required : false) >> >> # Use one of gnutls or libcrypto (provided by OpenSSL), trying gnutls first. >> libcrypto = dependency('gnutls', required : false) >> @@ -115,17 +113,6 @@ if libudev.found() >> ]) >> endif >> >> -# Fallback to a subproject if libyaml isn't found, as it's not packaged in AOSP. >> -if not libyaml.found() >> - cmake = import('cmake') >> - >> - libyaml_vars = cmake.subproject_options() >> - libyaml_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE': 'ON'}) >> - libyaml_vars.append_compile_args('c', '-Wno-unused-value') >> - libyaml_wrap = cmake.subproject('libyaml', options : libyaml_vars) >> - libyaml = libyaml_wrap.dependency('yaml') >> -endif >> - >> control_sources = [] >> >> controls_mode_files = { >> @@ -185,7 +172,6 @@ libcamera_deps += [ >> libdl, >> liblttng, >> libudev, >> - libyaml, >> ] >> >> # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.
On Fri, Dec 06, 2024 at 12:53:43PM +0100, Milan Zamazal wrote: > Laurent Pinchart writes: > > On Tue, Oct 01, 2024 at 12:27:54PM +0200, Milan Zamazal wrote: > >> Global configuration, which uses the YAML parser, will be in base. Thus > >> YAML parser must be moved to base too. > > > > The global configuration file seems quite specific to libcamera to me. > > Why do you think it should be in base ? > > Because logging is in base and uses the configuration. If logging > configuration was omitted from the global configuration, the global > configuration could be put elsewhere and also the annoying chicken-egg > problem when reading and parsing the configuration while possibly > logging about it would be avoided. But from the user's point of view I > think logging should be configurable in the file. Thanks for the explanation. I'll have a look at that. > > Is that maybe because we haven't really defined what base is ? > > > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/libcamera/base/meson.build | 14 ++++++++++++++ > >> src/libcamera/{ => base}/yaml_parser.cpp | 0 > >> src/libcamera/meson.build | 14 -------------- > >> 3 files changed, 14 insertions(+), 14 deletions(-) > >> rename src/libcamera/{ => base}/yaml_parser.cpp (100%) > >> > >> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build > >> index a742dfdfe..94843eb95 100644 > >> --- a/src/libcamera/base/meson.build > >> +++ b/src/libcamera/base/meson.build > >> @@ -24,10 +24,12 @@ libcamera_base_internal_sources = files([ > >> 'thread.cpp', > >> 'timer.cpp', > >> 'utils.cpp', > >> + 'yaml_parser.cpp', > >> ]) > >> > >> libdw = dependency('libdw', required : false) > >> libunwind = dependency('libunwind', required : false) > >> +libyaml = dependency('yaml-0.1', required : false) > >> > >> if cc.has_header_symbol('execinfo.h', 'backtrace') > >> config_h.set('HAVE_BACKTRACE', 1) > >> @@ -41,11 +43,23 @@ if libunwind.found() > >> config_h.set('HAVE_UNWIND', 1) > >> endif > >> > >> +# Fallback to a subproject if libyaml isn't found, as it's not packaged in AOSP. > >> +if not libyaml.found() > >> + cmake = import('cmake') > >> + > >> + libyaml_vars = cmake.subproject_options() > >> + libyaml_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE': 'ON'}) > >> + libyaml_vars.append_compile_args('c', '-Wno-unused-value') > >> + libyaml_wrap = cmake.subproject('libyaml', options : libyaml_vars) > >> + libyaml = libyaml_wrap.dependency('yaml') > >> +endif > >> + > >> libcamera_base_deps = [ > >> libatomic, > >> libdw, > >> libthreads, > >> libunwind, > >> + libyaml, > >> ] > >> > >> # Internal components must use the libcamera_base_private dependency to enable > >> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/base/yaml_parser.cpp > >> similarity index 100% > >> rename from src/libcamera/yaml_parser.cpp > >> rename to src/libcamera/base/yaml_parser.cpp > >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > >> index aa9ab0291..736cc496f 100644 > >> --- a/src/libcamera/meson.build > >> +++ b/src/libcamera/meson.build > >> @@ -51,7 +51,6 @@ libcamera_internal_sources = files([ > >> 'v4l2_pixelformat.cpp', > >> 'v4l2_subdevice.cpp', > >> 'v4l2_videodevice.cpp', > >> - 'yaml_parser.cpp', > >> ]) > >> > >> includes = [ > >> @@ -79,7 +78,6 @@ if not cc.has_function('dlopen') > >> libdl = cc.find_library('dl') > >> endif > >> libudev = dependency('libudev', required : get_option('udev')) > >> -libyaml = dependency('yaml-0.1', required : false) > >> > >> # Use one of gnutls or libcrypto (provided by OpenSSL), trying gnutls first. > >> libcrypto = dependency('gnutls', required : false) > >> @@ -115,17 +113,6 @@ if libudev.found() > >> ]) > >> endif > >> > >> -# Fallback to a subproject if libyaml isn't found, as it's not packaged in AOSP. > >> -if not libyaml.found() > >> - cmake = import('cmake') > >> - > >> - libyaml_vars = cmake.subproject_options() > >> - libyaml_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE': 'ON'}) > >> - libyaml_vars.append_compile_args('c', '-Wno-unused-value') > >> - libyaml_wrap = cmake.subproject('libyaml', options : libyaml_vars) > >> - libyaml = libyaml_wrap.dependency('yaml') > >> -endif > >> - > >> control_sources = [] > >> > >> controls_mode_files = { > >> @@ -185,7 +172,6 @@ libcamera_deps += [ > >> libdl, > >> liblttng, > >> libudev, > >> - libyaml, > >> ] > >> > >> # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.
diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build index a742dfdfe..94843eb95 100644 --- a/src/libcamera/base/meson.build +++ b/src/libcamera/base/meson.build @@ -24,10 +24,12 @@ libcamera_base_internal_sources = files([ 'thread.cpp', 'timer.cpp', 'utils.cpp', + 'yaml_parser.cpp', ]) libdw = dependency('libdw', required : false) libunwind = dependency('libunwind', required : false) +libyaml = dependency('yaml-0.1', required : false) if cc.has_header_symbol('execinfo.h', 'backtrace') config_h.set('HAVE_BACKTRACE', 1) @@ -41,11 +43,23 @@ if libunwind.found() config_h.set('HAVE_UNWIND', 1) endif +# Fallback to a subproject if libyaml isn't found, as it's not packaged in AOSP. +if not libyaml.found() + cmake = import('cmake') + + libyaml_vars = cmake.subproject_options() + libyaml_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE': 'ON'}) + libyaml_vars.append_compile_args('c', '-Wno-unused-value') + libyaml_wrap = cmake.subproject('libyaml', options : libyaml_vars) + libyaml = libyaml_wrap.dependency('yaml') +endif + libcamera_base_deps = [ libatomic, libdw, libthreads, libunwind, + libyaml, ] # Internal components must use the libcamera_base_private dependency to enable diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/base/yaml_parser.cpp similarity index 100% rename from src/libcamera/yaml_parser.cpp rename to src/libcamera/base/yaml_parser.cpp diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build index aa9ab0291..736cc496f 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -51,7 +51,6 @@ libcamera_internal_sources = files([ 'v4l2_pixelformat.cpp', 'v4l2_subdevice.cpp', 'v4l2_videodevice.cpp', - 'yaml_parser.cpp', ]) includes = [ @@ -79,7 +78,6 @@ if not cc.has_function('dlopen') libdl = cc.find_library('dl') endif libudev = dependency('libudev', required : get_option('udev')) -libyaml = dependency('yaml-0.1', required : false) # Use one of gnutls or libcrypto (provided by OpenSSL), trying gnutls first. libcrypto = dependency('gnutls', required : false) @@ -115,17 +113,6 @@ if libudev.found() ]) endif -# Fallback to a subproject if libyaml isn't found, as it's not packaged in AOSP. -if not libyaml.found() - cmake = import('cmake') - - libyaml_vars = cmake.subproject_options() - libyaml_vars.add_cmake_defines({'CMAKE_POSITION_INDEPENDENT_CODE': 'ON'}) - libyaml_vars.append_compile_args('c', '-Wno-unused-value') - libyaml_wrap = cmake.subproject('libyaml', options : libyaml_vars) - libyaml = libyaml_wrap.dependency('yaml') -endif - control_sources = [] controls_mode_files = { @@ -185,7 +172,6 @@ libcamera_deps += [ libdl, liblttng, libudev, - libyaml, ] # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.
Global configuration, which uses the YAML parser, will be in base. Thus YAML parser must be moved to base too. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/base/meson.build | 14 ++++++++++++++ src/libcamera/{ => base}/yaml_parser.cpp | 0 src/libcamera/meson.build | 14 -------------- 3 files changed, 14 insertions(+), 14 deletions(-) rename src/libcamera/{ => base}/yaml_parser.cpp (100%)