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? > 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? > 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 > >>>>> > >>>>> > >> >
