On 06/22 19:06:01, Maxim Uvarov wrote: > On 06/22/17 17:17, Brian Brooks wrote: > > On 06/22 11:13:57, Maxim Uvarov wrote: > >> On 22 June 2017 at 06:24, Brian Brooks <brian.bro...@arm.com> wrote: > >> > >>> Explicitly add all arch/<arch>/* files to respective _SOURCES > >>> variables instead of using @ARCH_DIR@ substitution. > >>> > >>> This patch fixes the broken build for ARM, PPC, and MIPS > >>> introduced by [1] and the similar issue reported while > >>> testing [2]. > >>> > >>> From the Autoconf manual [3]: > >>> > >>> You can't put a configure substitution (e.g., '@FOO@' or > >>> '$(FOO)' where FOO is defined via AC_SUBST) into a _SOURCES > >>> variable. The reason for this is a bit hard to explain, but > >>> suffice to say that it simply won't work. > >>> > > > not clean why $(srcdir) work and $(ARCH_DIR) will not work. > > I changed this in your patch and it works well: > > -odpapiinclude_HEADERS += $(srcdir)/arch/x86/odp/api/cpu_arch.h > +odpapiinclude_HEADERS += $(srcdir)/arch/$(ARCH_DIR)/odp/api/cpu_arch.h
Tried it on ARM and it breaks. If you read the Autoconf manual (above) it explicitly states that you cannot use variable substitution in _SOURCES (obviously also _HEADERS). As you point out, this is probably also only for user-defined variables (e.g. configure.ac) instead of preset output variables (e.g. srcdir). > Maxim. > > > >>> Here be dragons.. > >>> > >>> [1] https://lists.linaro.org/pipermail/lng-odp/2017-April/030324.html > >>> [2] https://lists.linaro.org/pipermail/lng-odp/2017-June/031598.html > >>> [3] https://www.gnu.org/software/automake/manual/html_node/ > >>> Conditional-Sources.html > >>> > >>> Signed-off-by: Brian Brooks <brian.bro...@arm.com> > >>> --- > >>> configure.ac | 3 +++ > >>> platform/linux-generic/Makefile.am | 40 ++++++++++++++++++++++++++++++ > >>> -------- > >>> 2 files changed, 35 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/configure.ac b/configure.ac > >>> index 46c7bbd2..45812f66 100644 > >>> --- a/configure.ac > >>> +++ b/configure.ac > >>> @@ -225,6 +225,9 @@ AM_CONDITIONAL([HAVE_DOXYGEN], [test "x${DOXYGEN}" = > >>> "xdoxygen"]) > >>> AM_CONDITIONAL([user_guide], [test "x${user_guides}" = "xyes" ]) > >>> AM_CONDITIONAL([HAVE_MSCGEN], [test "x${MSCGEN}" = "xmscgen"]) > >>> AM_CONDITIONAL([helper_linux], [test x$helper_linux = xyes ]) > >>> +AM_CONDITIONAL([ARCH_IS_ARM], [test "x${ARCH_DIR}" = "xarm"]) > >>> +AM_CONDITIONAL([ARCH_IS_MIPS64], [test "x${ARCH_DIR}" = "xmips64"]) > >>> +AM_CONDITIONAL([ARCH_IS_POWERPC], [test "x${ARCH_DIR}" = "xpowerpc"]) > >>> AM_CONDITIONAL([ARCH_IS_X86], [test "x${ARCH_DIR}" = "xx86"]) > >>> > >>> ############################################################ > >>> ############## > >>> diff --git a/platform/linux-generic/Makefile.am b/platform/linux-generic/ > >>> Makefile.am > >>> index 8dcdebd2..385c5304 100644 > >>> --- a/platform/linux-generic/Makefile.am > >>> +++ b/platform/linux-generic/Makefile.am > >>> @@ -63,8 +63,20 @@ odpapiinclude_HEADERS = \ > >>> $(srcdir)/include/odp/api/time.h \ > >>> $(srcdir)/include/odp/api/timer.h \ > >>> $(srcdir)/include/odp/api/traffic_mngr.h \ > >>> - $(srcdir)/include/odp/api/version.h \ > >>> - $(srcdir)/arch/@ARCH_DIR@/odp/api/cpu_arch.h > >>> + $(srcdir)/include/odp/api/version.h > >>> + > >>> +if ARCH_IS_ARM > >>> +odpapiinclude_HEADERS += $(srcdir)/arch/arm/odp/api/cpu_arch.h > >>> +endif > >>> +if ARCH_IS_MIPS64 > >>> +odpapiinclude_HEADERS += $(srcdir)/arch/mips64/odp/api/cpu_arch.h > >>> +endif > >>> +if ARCH_IS_POWERPC > >>> +odpapiinclude_HEADERS += $(srcdir)/arch/powerpc/odp/api/cpu_arch.h > >>> +endif > >>> +if ARCH_IS_X86 > >>> +odpapiinclude_HEADERS += $(srcdir)/arch/x86/odp/api/cpu_arch.h > >>> +endif > >>> > >>> > >> > >> If - else is better to be here. Something like: > >> > >> ifeq ($ARCH_IS_ARM), 1) > >> .. > >> > >> else ifeq ($ARCH_IS_MIPS64, 1) > >> > >> else > >> unsupported > >> endif > >> > >> > >> It will be more nice if it would be: > >> ifeq ($ARCH, arm) > >> .. > >> else ifeq ($ARCH, mips) > > > > Can't do this because ifeq, ifneq, ifdef, and ifndef are Makefile > > conditionals, > > not Automake conditionals. > > > >> odpapiplatincludedir= $(includedir)/odp/api/plat > >>> odpapiplatinclude_HEADERS = \ > >>> @@ -217,20 +229,32 @@ __LIB__libodp_linux_la_SOURCES = \ > >>> odp_timer_wheel.c \ > >>> odp_traffic_mngr.c \ > >>> odp_version.c \ > >>> - odp_weak.c \ > >>> - arch/@ARCH_DIR@/odp_cpu_arch.c \ > >>> - arch/@ARCH_DIR@/odp_sysinfo_parse.c > >>> - > >>> -__LIB__libodp_linux_la_LIBADD = $(ATOMIC_LIBS) > >>> + odp_weak.c > >>> > >>> +if ARCH_IS_ARM > >>> +__LIB__libodp_linux_la_SOURCES += arch/arm/odp_cpu_arch.c \ > >>> + arch/arm/odp_sysinfo_parse.c > >>> +endif > >>> +if ARCH_IS_MIPS64 > >>> +__LIB__libodp_linux_la_SOURCES += arch/mips64/odp_cpu_arch.c \ > >>> + arch/mips64/odp_sysinfo_parse.c > >>> +endif > >>> +if ARCH_IS_POWERPC > >>> +__LIB__libodp_linux_la_SOURCES += arch/powerpc/odp_cpu_arch.c \ > >>> + arch/powerpc/odp_sysinfo_parse.c > >>> +endif > >>> if ARCH_IS_X86 > >>> -__LIB__libodp_linux_la_SOURCES += arch/@ARCH_DIR@/cpu_flags.c > >>> +__LIB__libodp_linux_la_SOURCES += arch/x86/odp_cpu_arch.c \ > >>> + arch/x86/odp_sysinfo_parse.c \ > >>> + arch/x86/cpu_flags.c > >>> > >> > >> sort this. > >> > >> > >> > >>> endif > >>> > >>> if HAVE_PCAP > >>> __LIB__libodp_linux_la_SOURCES += pktio/pcap.c > >>> endif > >>> > >>> +__LIB__libodp_linux_la_LIBADD = $(ATOMIC_LIBS) > >>> + > >>> # Create symlink for ABI header files. Application does not need to use > >>> the arch > >>> # specific include path for installed files. > >>> install-data-hook: > >>> -- > >>> 2.13.1 > >>> > >>> >
