Dmitry Eremin-Solenikov(lumag) replied on github web page: Makefile.am line 5 @@ -20,7 +20,7 @@ SUBDIRS = \ @DX_RULES@ -EXTRA_DIST = bootstrap CHANGELOG config/README +EXTRA_DIST = bootstrap CHANGELOG config/README config/odp-linux.conf
Comment: 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_r171261849 updated_at 2018-02-28 14:40:20