Dmitry Eremin-Solenikov(lumag) 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:
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_r171263768
updated_at 2018-02-28 14:40:20
