On 06/22/17 19:19, Brian Brooks wrote: > 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). >
ok thanks. then only one comment for alphabetical reorder. I think we can try to add arm-qemu to travis to also capture such bugs. Maxim. >> 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 >>>>> >>>>> >>
