Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
On 22 June 2017 at 11:58, Maxim Uvarov <maxim.uva...@linaro.org> wrote: > 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 >>>>>>>> >>>>>>>> >>>>> >>> >
