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:
Adding `DISTCLEANFILES = include/odp_libconfig_config.h` fixed the discheck
failure when odp_libconfig_config.h is added to noinst_HEADERS.
> Matias Elo(matiaselo) wrote:
> 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_r171673380
updated_at 2018-03-01 19:48:02
