Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 11
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -----------------------------------------------------
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##########################################################################
+# Set optional libconfig path
+##########################################################################
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
Comment:
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_r171258219
updated_at 2018-02-28 14:40:20
