Matias Elo(matiaselo) replied on github web page: platform/linux-generic/Makefile.am line 14 @@ -94,6 +96,8 @@ noinst_HEADERS = \ include/odp_forward_typedefs_internal.h \ include/odp_internal.h \ include/odp_ipsec_internal.h \ + include/odp_libconfig_config.h \ + include/odp_libconfig_internal.h \
Comment: 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_r171272412 updated_at 2018-02-28 15:01:18