Dmitry Eremin-Solenikov(lumag) replied on github web page: platform/linux-generic/libodp-linux.pc.in line 5 @@ -7,5 +7,5 @@ Name: libodp-linux Description: The ODP packet processing engine Version: @PKGCONFIG_VERSION@ Libs: -L${libdir} -lodp-linux -Libs.private: @OPENSSL_STATIC_LIBS@ @DPDK_LIBS@ @PCAP_LIBS@ @PTHREAD_LIBS@ @TIMER_LIBS@ -lpthread @ATOMIC_LIBS@ +Libs.private: @OPENSSL_STATIC_LIBS@ @LIBCONFIG_LIBS@ @DPDK_LIBS@ @PCAP_LIBS@ @PTHREAD_LIBS@ @TIMER_LIBS@ -lpthread @ATOMIC_LIBS@
Comment: 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_r171260686 updated_at 2018-02-28 14:40:20