Dmitry Eremin-Solenikov(lumag) replied on github web page: m4/odp_libconfig.m4 line 28 @@ -0,0 +1,36 @@ +# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND]) +# ----------------------------------------------------- +AC_DEFUN([ODP_LIBCONFIG], +[dnl +########################################################################## +# Set optional libconfig path +########################################################################## +AC_ARG_WITH([libconfig-path], +[AS_HELP_STRING([--with-libconfig-path=DIR], +[path to libconfig libs and headers (use system path if not provided)])], +[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"], +) + +########################################################################## +# Check for libconfig availability +########################################################################## +odp_libconfig_ok=yes +PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [], + [odp_libconfig_ok=no]) + +if test "x$odp_libconfig_ok" = "xyes" ; then +########################################################################## +# Create a header file odp_libconfig_config.h which containins null +# terminated hex dump of odp-linux.conf +########################################################################## + [pushd ${srcdir}/config + xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \ + ../platform/${with_platform}/include/odp_libconfig_config.h
Comment: Makefile part: ```make include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf $(top_srcdir)/config.status cd $(top_builddir) && $(SHELL) ./config.status odp_libconfig_config.h ``` Also it might be nice to use platform/${with_platform}/odp_libconfig_config.h as tag name, so that tags for different platforms won't collide. > Dmitry Eremin-Solenikov(lumag) wrote: > I'd say > ```m4 > AC_CONFIG_COMMANDS([odp_libconfig_config.h], > [ mkdir -p platform/${with_platform}/include > xxd -i ${srcdir}/platform/${with_platform}/odp-linux.conf | sed > 's/\([0-9a-f]\)$/\0, 0x00/' > \ > platform/${with_platform}/include/odp_libconfig_config.h]) > ``` >> Matias Elo(matiaselo) wrote: >> I need some assistance with this. What I have now is: >> >> ``` >> AC_CONFIG_COMMANDS([odp_libconfig_config.h], >> [ >> pushd ${ac_abs_top_builddir}/config >> xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \ >> ../platform/${with_platform}/include/odp_libconfig_config.h >> popd], >> [with_platform=$with_platform] >> ) >> ``` >> >> And in .../linux-generic/Makefile.am: >> ``` >> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf >> $(top_srcdir)/config.status >> ./$(top_srcdir)/config.status odp_libconfig_config.h >> ``` >> This seems to work as it detects changes in odp-linux.conf, but I'm not sure >> if this is the proper way to do this. >>> Dmitry Eremin-Solenikov(lumag) wrote: >>> Builddir is current directory. >>>> Matias Elo(matiaselo) wrote: >>>> builddir is not defined here. Any ideas? >>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>> Consider somebody changing conf file and reruning make. Make should >>>>> detect that header should be regenerated and invoke ./config.status with >>>>> proper tag to regenrate header in question. >>>>>> Matias Elo(matiaselo) wrote: >>>>>> > Also there should be a dependency line in respective Makefile.am to >>>>>> > regenerate the header. >>>>>> >>>>>> Can you elaborate this a bit? >>>>>>> Matias Elo(matiaselo) wrote: >>>>>>> Yeah, I actually tried merging the files but it ended up being painful >>>>>>> since libconfig doesn't provide good functions for doing that. So I >>>>>>> went with the "safe" solution. I'll try if I can at least get rid of >>>>>>> the extra pointers. >>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>> Merging is not required in the first pass, but it might be a good >>>>>>>> thing in future. >>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>> OK >>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>> OK >>>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>>> OK >>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>> Last line should go to `nodist_noinst_HEADERS`, so it does not get >>>>>>>>>>>> included into distribution. >>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>> Just drop this setting/check. `PKG_CHECK_MODULES` will print >>>>>>>>>>>>> usefull message instead. >>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>> Should ODP install this header file somewhere? >>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>> It might be usefull to have "default" config file provided at >>>>>>>>>>>>>>> ${sysconfdir}. >>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>> Please use instead: >>>>>>>>>>>>>>>> `Require.private: libconfig` >>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>> Generated file should go to builddir, not to srcdir. >>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>> Please use `AC_CONFIG_COMMANDS` to regenerate this header. >>>>>>>>>>>>>>>>>> Also there should be a dependency line in respective >>>>>>>>>>>>>>>>>> Makefile.am to regenerate the header. >>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>> Can we please have just two config structures here (default >>>>>>>>>>>>>>>>>>> and running)? I wonder, if we can actually merge default >>>>>>>>>>>>>>>>>>> with provided configurations into a single config structure? >>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>>> No need to `AC_SUBST` them, as you are using >>>>>>>>>>>>>>>>>>>> PKG_CHECK_MODULES. >>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>>>> No, this is not necessary. One can override this setting >>>>>>>>>>>>>>>>>>>>> using environment. >>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>>>>> As the last resort you can load it and then call >>>>>>>>>>>>>>>>>>>>>> `config_file_write()` from the helper app. I'd prefer >>>>>>>>>>>>>>>>>>>>>> instead if callers could provide default values fro >>>>>>>>>>>>>>>>>>>>>> options that they require. >>>>>>>>>>>>>>>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>>>>>>>>>>>>>>> `config_t libconfig`can always be initialized, but in >>>>>>>>>>>>>>>>>>>>>>> case `ODP_CONF_FILE` is not given, no configuration >>>>>>>>>>>>>>>>>>>>>>> file is read. I'm not sure if all libconfig functions >>>>>>>>>>>>>>>>>>>>>>> handle this properly without crashing and I'm playing >>>>>>>>>>>>>>>>>>>>>>> it safe here. Maybe it's cleaner if I replace this with: >>>>>>>>>>>>>>>>>>>>>>> ``` >>>>>>>>>>>>>>>>>>>>>>> config_t *libconfig; >>>>>>>>>>>>>>>>>>>>>>> config_t libconfig_structure; >>>>>>>>>>>>>>>>>>>>>>> ``` >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> Actually, this gave me an idea. It would probably make >>>>>>>>>>>>>>>>>>>>>>> sense to save the `odp-linux.conf` contents to a C file >>>>>>>>>>>>>>>>>>>>>>> during configure. This way we always have a valid >>>>>>>>>>>>>>>>>>>>>>> default configuration and the default options have to >>>>>>>>>>>>>>>>>>>>>>> be saved only to one location. Any suggestions what >>>>>>>>>>>>>>>>>>>>>>> would be a good method to save the file during >>>>>>>>>>>>>>>>>>>>>>> configure? >>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>>>>>>>>>>>>>>>>>>>> `ODP_LOG()` will just print if not overridded. The >>>>>>>>>>>>>>>>>>>>>>>> point is ODP shouldn't be using `printf()` directly. >>>>>>>>>>>>>>>>>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>>>>>>>>>>>>>>>>> I would prefer changing OFP to match our naming >>>>>>>>>>>>>>>>>>>>>>>>> convention. We use 'config' in several APIs, whereas >>>>>>>>>>>>>>>>>>>>>>>>> 'conf' is used only internally. >>>>>>>>>>>>>>>>>>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>> This is just an internal helper to avoid having to >>>>>>>>>>>>>>>>>>>>>>>>>> reference to odp_global_data when calling >>>>>>>>>>>>>>>>>>>>>>>>>> `config_lookup()`. It could actually be removed >>>>>>>>>>>>>>>>>>>>>>>>>> altogether. >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>> Since we are using libconfig only internally, what's >>>>>>>>>>>>>>>>>>>>>>>>>> the benefit of wrapping its functions (other than >>>>>>>>>>>>>>>>>>>>>>>>>> avoiding hard dependency)? We are not wrapping e.g. >>>>>>>>>>>>>>>>>>>>>>>>>> openssl functions. Adding wrappers for the >>>>>>>>>>>>>>>>>>>>>>>>>> aforementioned `_odp_config_lookup_int()` and >>>>>>>>>>>>>>>>>>>>>>>>>> `_odp_config_lookup_string()` would be simple, but >>>>>>>>>>>>>>>>>>>>>>>>>> for example` config_setting_lookup()`, which I'm >>>>>>>>>>>>>>>>>>>>>>>>>> using to find dpdk driver specific options, uses >>>>>>>>>>>>>>>>>>>>>>>>>> libconfig data types which would also have to be >>>>>>>>>>>>>>>>>>>>>>>>>> wrapped. >>>>>>>>>>>>>>>>>>>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>> As we already require for example autoconf, >>>>>>>>>>>>>>>>>>>>>>>>>>> libtool, and pkg-config, adding libconfig >>>>>>>>>>>>>>>>>>>>>>>>>>> dependency shouldn't be a big issue. However, if >>>>>>>>>>>>>>>>>>>>>>>>>>> this causes problems we can remove the hard >>>>>>>>>>>>>>>>>>>>>>>>>>> dependency by wrapping all used libconfig functions >>>>>>>>>>>>>>>>>>>>>>>>>>> with dummy implementations. I would prefer not >>>>>>>>>>>>>>>>>>>>>>>>>>> doing this if not explicitly required. >>>>>>>>>>>>>>>>>>>>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>> Not that familiar with autoconf, so I pretty much >>>>>>>>>>>>>>>>>>>>>>>>>>>> copy-pasted the `odp_openssl.m4` to get this >>>>>>>>>>>>>>>>>>>>>>>>>>>> working. I'll take a look at `PKG_CHECK_MODULES `. >>>>>>>>>>>>>>>>>>>>>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>> OK, will fix. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I'm usually using multiple different NICs, so I >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> find this information useful for logging test >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> runs. Perhaps use ODP_PRINT() here? >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Will fix. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Just use `PKG_CHECK_MODULES` here rather than >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> inventing variables and checks. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Ideally we do not need this field. We should >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> always return valid setting. Maybe by >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> allowing ODP modules to return default value. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I'd prefer not to export config internals >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> here, but rather have >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `_odp_config_lookup_int()`, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `_odp_config_lookup_string()`, etc. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Last line should not be necessary >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> bogdanPricope wrote >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> OFP is using a similar naming for the >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> environment variable but with 'CONF' >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> instead of 'CONFIG' ('OFP_CONF_FILE'). >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Will it makes sense to change the naming >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> to avoid confusions: either >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 'ODP_CONF_FILE' in ODP or >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 'OFP_CONFIG_FILE' in OFP? >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Do we really want to print this >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> unconditionally? In any event shouldn't >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> this be `ODP_LOG()` here rather than >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `printf()`? >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> According to the [libconfig >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> changelog](https://github.com/hyperrealm/libconfig/blob/master/ChangeLog) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> there are versions pre-1.0 (e.g., 0.9) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> which would fail this test. This needs >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> to be reversed so that you use the newer >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> form for v1.5 and higher levels: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ``` >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> #if (LIBCONFIG_VER_MAJOR > 1 || >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (LIBCONFIG_VER_MAJOR == 1 && >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> LIBCONFIG_VER_MINOR >= 5)) ... >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ``` >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Does this have to be a hard dependency? >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Can we have this feature be omitted >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (hardcoded defaults are used) if >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> libconfig is not available? https://github.com/Linaro/odp/pull/499#discussion_r171494469 updated_at 2018-03-01 08:55:56