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
