Matias Elo(matiaselo) 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:
A good point. Would this be OK?
.../platform/Makefile.inc
```
configdir = $(sysconfdir)/odp
config_DATA = $(top_srcdir)/config/odp-$(with_platform).conf
```
> Matias Elo(matiaselo) wrote:
> 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_r171600672
updated_at 2018-03-01 15:48:03
