Matias Elo(matiaselo) replied on github web page:
m4/odp_libconfig.m4
line 28
@@ -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"],
+)
+
+##########################################################################
+# Check for libconfig availability
+##########################################################################
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##########################################################################
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##########################################################################
+ [pushd ${srcdir}/config
+ xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
Comment:
Thanks! I'll try this.
> Dmitry Eremin-Solenikov(lumag) wrote:
> Typical idiom is
> ```make
> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
> $(top_builddir)/config.status
> cd $(top_builddir) && $(SHELL) ./config.status $(subdir)/$@
> ```
>
> I'd suggest you to try building with separate builddir, it should help you to
> catch build/src dir differences.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Makefile part:
>> ```make
>> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
>> $(top_srcdir)/config.status
>> cd $(top_builddir) && $(SHELL) ./config.status odp_libconfig_config.h
>> ```
>>
>> Also it might be nice to use
>> platform/${with_platform}/odp_libconfig_config.h as tag name, so that tags
>> for different platforms won't collide.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> I'd say
>>> ```m4
>>> AC_CONFIG_COMMANDS([odp_libconfig_config.h],
>>> [ mkdir -p platform/${with_platform}/include
>>> xxd -i ${srcdir}/platform/${with_platform}/odp-linux.conf | sed
>>> 's/\([0-9a-f]\)$/\0, 0x00/' > \
>>> platform/${with_platform}/include/odp_libconfig_config.h])
>>> ```
>>>> Matias Elo(matiaselo) wrote:
>>>> I need some assistance with this. What I have now is:
>>>>
>>>> ```
>>>> AC_CONFIG_COMMANDS([odp_libconfig_config.h],
>>>> [
>>>> pushd ${ac_abs_top_builddir}/config
>>>> xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
>>>> ../platform/${with_platform}/include/odp_libconfig_config.h
>>>> popd],
>>>> [with_platform=$with_platform]
>>>> )
>>>> ```
>>>>
>>>> And in .../linux-generic/Makefile.am:
>>>> ```
>>>> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
>>>> $(top_srcdir)/config.status
>>>> ./$(top_srcdir)/config.status odp_libconfig_config.h
>>>> ```
>>>> This seems to work as it detects changes in odp-linux.conf, but I'm not
>>>> sure if this is the proper way to do this.
>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>> Builddir is current directory.
>>>>>> Matias Elo(matiaselo) wrote:
>>>>>> builddir is not defined here. Any ideas?
>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>> Consider somebody changing conf file and reruning make. Make should
>>>>>>> detect that header should be regenerated and invoke ./config.status
>>>>>>> with proper tag to regenrate header in question.
>>>>>>>> Matias Elo(matiaselo) wrote:
>>>>>>>> > Also there should be a dependency line in respective Makefile.am to
>>>>>>>> > regenerate the header.
>>>>>>>>
>>>>>>>> Can you elaborate this a bit?
>>>>>>>>> Matias Elo(matiaselo) wrote:
>>>>>>>>> Yeah, I actually tried merging the files but it ended up being
>>>>>>>>> painful since libconfig doesn't provide good functions for doing
>>>>>>>>> that. So I went with the "safe" solution. I'll try if I can at least
>>>>>>>>> get rid of the extra pointers.
>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>> Merging is not required in the first pass, but it might be a good
>>>>>>>>>> thing in future.
>>>>>>>>>>> Matias Elo(matiaselo) wrote:
>>>>>>>>>>> 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_r171495614
updated_at 2018-03-01 09:01:05
