Matias Elo(matiaselo) 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:
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_r171272122
updated_at 2018-02-28 15:00:26
