On 06/22/17 19:54, Brian Brooks wrote: > On 06/22 19:44:45, Maxim Uvarov wrote: >> 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. > > They are in alphabetical order according to arch: A > M > P > X > Do you see something else? >
cpu flags before odp_ +__LIB__libodp_linux_la_SOURCES += arch/x86/odp_cpu_arch.c \ + arch/x86/odp_sysinfo_parse.c \ + arch/x86/cpu_flags.c >> I think we can try to add arm-qemu to travis to also capture such bugs. > > I thought there were ARM machines in LNG CI? Is there a way for users > to trigger a CI run over there before submitting a patch? > it's possible to integrate it with github. And it has to do the same thing which travis do now. I created ticket to automation team to do it. But looks like it's very low priority for then. I can not do it because it requires admin rights to Jenkins I think. Maxim. >> 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 >>>>>>> >>>>>>> >>>> >>
