Dmitry Eremin-Solenikov(lumag) replied on github web page: m4/odp_libconfig.m4 line 31 @@ -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 + popd] + AC_SUBST([LIBCONFIG_CFLAGS]) + AC_SUBST([LIBCONFIG_LIBS])
Comment: 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_r171258500 updated_at 2018-02-28 14:40:20