Dmitry Eremin-Solenikov(lumag) 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:
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_r171495241
updated_at 2018-03-01 08:59:45
