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
