Matias Elo(matiaselo) replied on github web page:
platform/linux-generic/Makefile.am
line 14
@@ -94,6 +96,8 @@ noinst_HEADERS = \
include/odp_forward_typedefs_internal.h \
include/odp_internal.h \
include/odp_ipsec_internal.h \
+ include/odp_libconfig_config.h \
+ include/odp_libconfig_internal.h \
Comment:
I assume you meant odp_libconfig_config.h Anyway, doing this breaks distcheck
build:
```
CC odp_libconfig.lo
../../../../platform/linux-generic/odp_libconfig.c:17:34: fatal error:
odp_libconfig_config.h: No such file or directory
compilation terminated.
Makefile:1120: recipe for target 'odp_libconfig.lo' failed
make[2]: *** [odp_libconfig.lo] Error 1
make[2]: Leaving directory
'/home/nnn/dev/odp.git/opendataplane-1.17.0.0/_build/sub/platform/linux-generic'
```
With both headers in noinst_HEADERS distcheck completes but complains at the
end:
```
ERROR: files left in build directory after distclean:
./platform/linux-generic/include/odp_libconfig_config.h
Makefile:793: recipe for target 'distcleancheck' failed
make[1]: *** [distcleancheck] Error 1
make[1]: Leaving directory
'/home/nnn/dev/odp.git/opendataplane-1.17.0.0/_build/sub'
Makefile:719: recipe for target 'distcheck' failed
make: *** [distcheck] Error 1
```
Any pointers?
> Matias Elo(matiaselo) wrote:
> 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_r171621062
updated_at 2018-03-01 16:48:09
