Matias Elo(matiaselo) replied on github web page: m4/odp_libconfig.m4 line 11 @@ -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"],
Comment: 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_r171272122 updated_at 2018-02-28 15:00:26