| Message ID | 1455304826-10557-3-git-send-email-patrickdepinguin@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
Hi Thomas, all, Le 12/02/2016 20:20, Thomas De Schampheleire a écrit : > From: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> > > In TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS, ARCH_SUBDIR is calculated but not > used, and can thus be removed. Since SYSROOT_DIR is only used for the > calculation of ARCH_SUBDIR, it can be removed too. > > Signed-off-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> Reviewed-by: Romain Naour <romain.naour@gmail.com> Best regards, Romain > --- > toolchain/toolchain-external/toolchain-external.mk | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/toolchain/toolchain-external/toolchain-external.mk b/toolchain/toolchain-external/toolchain-external.mk > index ffdee49..9d88158 100644 > --- a/toolchain/toolchain-external/toolchain-external.mk > +++ b/toolchain/toolchain-external/toolchain-external.mk > @@ -587,12 +587,7 @@ endef > # to the target filesystem. > > define TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS > - $(Q)SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC))" ; \ > - if test -z "$${SYSROOT_DIR}" ; then \ > - @echo "External toolchain doesn't support --sysroot. Cannot use." ; \ > - exit 1 ; \ > - fi ; \ > - ARCH_SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \ > + $(Q)ARCH_SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \ > ARCH_LIB_DIR="$(call toolchain_find_libdir,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \ > SUPPORT_LIB_DIR="" ; \ > if test `find $${ARCH_SYSROOT_DIR} -name 'libstdc++.a' | wc -l` -eq 0 ; then \ > @@ -601,7 +596,6 @@ define TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS > SUPPORT_LIB_DIR=`readlink -f $${LIBSTDCPP_A_LOCATION} | sed -r -e 's:libstdc\+\+\.a::'` ; \ > fi ; \ > fi ; \ > - ARCH_SUBDIR=`echo $${ARCH_SYSROOT_DIR} | sed -r -e "s:^$${SYSROOT_DIR}(.*)/$$:\1:"` ; \ > if test -z "$(BR2_STATIC_LIBS)" ; then \ > $(call MESSAGE,"Copying external toolchain libraries to target...") ; \ > for libs in $(LIB_EXTERNAL_LIBS); do \ >
I'm sorry, I still have comments... On 02/12/16 20:20, Thomas De Schampheleire wrote: > From: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> > > In TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS, ARCH_SUBDIR is calculated but not > used, and can thus be removed. Since SYSROOT_DIR is only used for the > calculation of ARCH_SUBDIR, it can be removed too. > > Signed-off-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> > --- > toolchain/toolchain-external/toolchain-external.mk | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/toolchain/toolchain-external/toolchain-external.mk b/toolchain/toolchain-external/toolchain-external.mk > index ffdee49..9d88158 100644 > --- a/toolchain/toolchain-external/toolchain-external.mk > +++ b/toolchain/toolchain-external/toolchain-external.mk > @@ -587,12 +587,7 @@ endef > # to the target filesystem. > > define TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS > - $(Q)SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC))" ; \ > - if test -z "$${SYSROOT_DIR}" ; then \ > - @echo "External toolchain doesn't support --sysroot. Cannot use." ; \ This was the only place where we gave that error message, so it should be kept. That said, it probably wouldn't have shown anyway, because we would already error out with an incomprehensible error message in the configure step. Bottom line: move it to the configure step first. Otherwise, it looks good however. So, since it anyway didn't work properly before, I'll already give this patch my Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> Regards, Arnout > - exit 1 ; \ > - fi ; \ > - ARCH_SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \ > + $(Q)ARCH_SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \ > ARCH_LIB_DIR="$(call toolchain_find_libdir,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \ > SUPPORT_LIB_DIR="" ; \ > if test `find $${ARCH_SYSROOT_DIR} -name 'libstdc++.a' | wc -l` -eq 0 ; then \ > @@ -601,7 +596,6 @@ define TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS > SUPPORT_LIB_DIR=`readlink -f $${LIBSTDCPP_A_LOCATION} | sed -r -e 's:libstdc\+\+\.a::'` ; \ > fi ; \ > fi ; \ > - ARCH_SUBDIR=`echo $${ARCH_SYSROOT_DIR} | sed -r -e "s:^$${SYSROOT_DIR}(.*)/$$:\1:"` ; \ > if test -z "$(BR2_STATIC_LIBS)" ; then \ > $(call MESSAGE,"Copying external toolchain libraries to target...") ; \ > for libs in $(LIB_EXTERNAL_LIBS); do \ >
On 03/27/16 22:34, Arnout Vandecappelle wrote: > I'm sorry, I still have comments... > > On 02/12/16 20:20, Thomas De Schampheleire wrote: >> From: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> >> >> In TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS, ARCH_SUBDIR is calculated but not >> used, and can thus be removed. Since SYSROOT_DIR is only used for the >> calculation of ARCH_SUBDIR, it can be removed too. >> >> Signed-off-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> >> --- >> toolchain/toolchain-external/toolchain-external.mk | 8 +------- >> 1 file changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/toolchain/toolchain-external/toolchain-external.mk >> b/toolchain/toolchain-external/toolchain-external.mk >> index ffdee49..9d88158 100644 >> --- a/toolchain/toolchain-external/toolchain-external.mk >> +++ b/toolchain/toolchain-external/toolchain-external.mk >> @@ -587,12 +587,7 @@ endef >> # to the target filesystem. >> >> define TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS >> - $(Q)SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC))" >> ; \ >> - if test -z "$${SYSROOT_DIR}" ; then \ >> - @echo "External toolchain doesn't support --sysroot. Cannot use." ; \ > > This was the only place where we gave that error message, so it should be kept. > > That said, it probably wouldn't have shown anyway, because we would already > error out with an incomprehensible error message in the configure step. Bottom > line: move it to the configure step first. As discussed with Romain, please disregard this comment, the check is indeed still there in the configure step. > > Otherwise, it looks good however. So, since it anyway didn't work properly > before, I'll already give this patch my > > Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> So this clearly still stands. Regards, Arnout > > Regards, > Arnout > >> - exit 1 ; \ >> - fi ; \ >> - ARCH_SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC) >> $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \ >> + $(Q)ARCH_SYSROOT_DIR="$(call >> toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" >> ; \ >> ARCH_LIB_DIR="$(call toolchain_find_libdir,$(TOOLCHAIN_EXTERNAL_CC) >> $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \ >> SUPPORT_LIB_DIR="" ; \ >> if test `find $${ARCH_SYSROOT_DIR} -name 'libstdc++.a' | wc -l` -eq 0 ; >> then \ >> @@ -601,7 +596,6 @@ define TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS >> SUPPORT_LIB_DIR=`readlink -f $${LIBSTDCPP_A_LOCATION} | sed -r >> -e 's:libstdc\+\+\.a::'` ; \ >> fi ; \ >> fi ; \ >> - ARCH_SUBDIR=`echo $${ARCH_SYSROOT_DIR} | sed -r -e >> "s:^$${SYSROOT_DIR}(.*)/$$:\1:"` ; \ >> if test -z "$(BR2_STATIC_LIBS)" ; then \ >> $(call MESSAGE,"Copying external toolchain libraries to target...") ; \ >> for libs in $(LIB_EXTERNAL_LIBS); do \ >> > >
Hello, On Fri, 12 Feb 2016 20:20:23 +0100, Thomas De Schampheleire wrote: > From: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> > > In TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS, ARCH_SUBDIR is calculated but not > used, and can thus be removed. Since SYSROOT_DIR is only used for the > calculation of ARCH_SUBDIR, it can be removed too. > > Signed-off-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com> > --- > toolchain/toolchain-external/toolchain-external.mk | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) Applied to master, thanks. Thomas
diff --git a/toolchain/toolchain-external/toolchain-external.mk b/toolchain/toolchain-external/toolchain-external.mk index ffdee49..9d88158 100644 --- a/toolchain/toolchain-external/toolchain-external.mk +++ b/toolchain/toolchain-external/toolchain-external.mk @@ -587,12 +587,7 @@ endef # to the target filesystem. define TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS - $(Q)SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC))" ; \ - if test -z "$${SYSROOT_DIR}" ; then \ - @echo "External toolchain doesn't support --sysroot. Cannot use." ; \ - exit 1 ; \ - fi ; \ - ARCH_SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \ + $(Q)ARCH_SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \ ARCH_LIB_DIR="$(call toolchain_find_libdir,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \ SUPPORT_LIB_DIR="" ; \ if test `find $${ARCH_SYSROOT_DIR} -name 'libstdc++.a' | wc -l` -eq 0 ; then \ @@ -601,7 +596,6 @@ define TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS SUPPORT_LIB_DIR=`readlink -f $${LIBSTDCPP_A_LOCATION} | sed -r -e 's:libstdc\+\+\.a::'` ; \ fi ; \ fi ; \ - ARCH_SUBDIR=`echo $${ARCH_SYSROOT_DIR} | sed -r -e "s:^$${SYSROOT_DIR}(.*)/$$:\1:"` ; \ if test -z "$(BR2_STATIC_LIBS)" ; then \ $(call MESSAGE,"Copying external toolchain libraries to target...") ; \ for libs in $(LIB_EXTERNAL_LIBS); do \