Dmitry Eremin-Solenikov(lumag) replied on github web page:
Makefile.am
line 5
@@ -20,7 +20,7 @@ SUBDIRS = \
@DX_RULES@
-EXTRA_DIST = bootstrap CHANGELOG config/README
+EXTRA_DIST = bootstrap CHANGELOG config/README config/odp-linux.conf
Comment:
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_r171261849
updated_at 2018-02-28 14:40:20
