| Message ID | 20210205220125.2931504-1-ndesaulniers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile: reuse CC_VERSION_TEXT | expand |
On Sat, Feb 6, 2021 at 7:01 AM 'Nick Desaulniers' via Clang Built Linux <clang-built-linux@googlegroups.com> wrote: > > I noticed we're invoking $(CC) via $(shell) more than once to check the > version. Let's reuse the first string captured in $CC_VERSION_TEXT. > > Fixes: 315bab4e972d ("kbuild: fix endless syncconfig in case arch Makefile sets CROSS_COMPILE") I did not touch this hunk because I have a plan for different refactoring, but I have never got around to do it. Anyway, you beat me, and I will pick this up. But, the Fixes tag is questionable because this is code refactoring. > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > --- > Makefile | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/Makefile b/Makefile > index a85535eb6a7d..70034d7c1051 100644 > --- a/Makefile > +++ b/Makefile > @@ -557,7 +557,13 @@ ifdef building_out_of_srctree > { echo "# this is build directory, ignore it"; echo "*"; } > .gitignore > endif > > -ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),) > +# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included. > +# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile. > +# CC_VERSION_TEXT is referenced from Kconfig (so it needs export), > +# and from include/config/auto.conf.cmd to detect the compiler upgrade. > +CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1) > + > +ifneq ($(findstring clang,$(CC_VERSION_TEXT)),) > ifneq ($(CROSS_COMPILE),) > CLANG_FLAGS += --target=$(notdir $(CROSS_COMPILE:%-=%)) > GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit)) > @@ -576,12 +582,6 @@ KBUILD_AFLAGS += $(CLANG_FLAGS) > export CLANG_FLAGS > endif > > -# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included. > -# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile. > -# CC_VERSION_TEXT is referenced from Kconfig (so it needs export), > -# and from include/config/auto.conf.cmd to detect the compiler upgrade. > -CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1) > - > ifdef config-build > # =========================================================================== > # *config targets only - make sure prerequisites are updated, and descend > -- > 2.30.0.478.g8a0d178c01-goog > > -- > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20210205220125.2931504-1-ndesaulniers%40google.com.
On Sat, Feb 6, 2021 at 2:49 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Sat, Feb 6, 2021 at 7:01 AM 'Nick Desaulniers' via Clang Built > Linux <clang-built-linux@googlegroups.com> wrote: > > > > I noticed we're invoking $(CC) via $(shell) more than once to check the > > version. Let's reuse the first string captured in $CC_VERSION_TEXT. > > > > Fixes: 315bab4e972d ("kbuild: fix endless syncconfig in case arch Makefile sets CROSS_COMPILE") > > > I did not touch this hunk because I have a plan > for different refactoring, but I have never got > around to do it. > > Anyway, you beat me, and I will pick this up. > But, the Fixes tag is questionable because > this is code refactoring. > When I see this... and hear refactoring... As a suggestion/improvement... Can we have LD_VERSION_TEXT analogue to CC_VERSION_TEXT? Both are shown when doing a `cat /proc/version` (and IIRC in file include/generated/compile.h). Thanks. - Sedat - > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > --- > > Makefile | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index a85535eb6a7d..70034d7c1051 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -557,7 +557,13 @@ ifdef building_out_of_srctree > > { echo "# this is build directory, ignore it"; echo "*"; } > .gitignore > > endif > > > > -ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),) > > +# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included. > > +# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile. > > +# CC_VERSION_TEXT is referenced from Kconfig (so it needs export), > > +# and from include/config/auto.conf.cmd to detect the compiler upgrade. > > +CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1) > > + > > +ifneq ($(findstring clang,$(CC_VERSION_TEXT)),) > > ifneq ($(CROSS_COMPILE),) > > CLANG_FLAGS += --target=$(notdir $(CROSS_COMPILE:%-=%)) > > GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit)) > > @@ -576,12 +582,6 @@ KBUILD_AFLAGS += $(CLANG_FLAGS) > > export CLANG_FLAGS > > endif > > > > -# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included. > > -# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile. > > -# CC_VERSION_TEXT is referenced from Kconfig (so it needs export), > > -# and from include/config/auto.conf.cmd to detect the compiler upgrade. > > -CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1) > > - > > ifdef config-build > > # =========================================================================== > > # *config targets only - make sure prerequisites are updated, and descend > > -- > > 2.30.0.478.g8a0d178c01-goog > > > > -- > > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. > > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. > > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20210205220125.2931504-1-ndesaulniers%40google.com. > > > > -- > Best Regards > Masahiro Yamada > > -- > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/CAK7LNARKHvjTcnic%3DZKntH3NY5meehQbJuBr34y9_tn8b-Ym0w%40mail.gmail.com.
On Fri, Feb 5, 2021 at 5:49 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Sat, Feb 6, 2021 at 7:01 AM 'Nick Desaulniers' via Clang Built > Linux <clang-built-linux@googlegroups.com> wrote: > > > > I noticed we're invoking $(CC) via $(shell) more than once to check the > > version. Let's reuse the first string captured in $CC_VERSION_TEXT. > > > > Fixes: 315bab4e972d ("kbuild: fix endless syncconfig in case arch Makefile sets CROSS_COMPILE") > > > I did not touch this hunk because I have a plan > for different refactoring, but I have never got > around to do it. > > Anyway, you beat me, and I will pick this up. > But, the Fixes tag is questionable because > this is code refactoring. Hi Masahiro, A friendly reminder to please pick this up; I don't see it yet in linux-next. I'm ok with you dropping the fixes tag when applied. (Otherwise, I was about to use `grep -m1` instead of `| head -n1 | grep` here.) > > > > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > --- > > Makefile | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index a85535eb6a7d..70034d7c1051 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -557,7 +557,13 @@ ifdef building_out_of_srctree > > { echo "# this is build directory, ignore it"; echo "*"; } > .gitignore > > endif > > > > -ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),) > > +# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included. > > +# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile. > > +# CC_VERSION_TEXT is referenced from Kconfig (so it needs export), > > +# and from include/config/auto.conf.cmd to detect the compiler upgrade. > > +CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1) > > + > > +ifneq ($(findstring clang,$(CC_VERSION_TEXT)),) > > ifneq ($(CROSS_COMPILE),) > > CLANG_FLAGS += --target=$(notdir $(CROSS_COMPILE:%-=%)) > > GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit)) > > @@ -576,12 +582,6 @@ KBUILD_AFLAGS += $(CLANG_FLAGS) > > export CLANG_FLAGS > > endif > > > > -# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included. > > -# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile. > > -# CC_VERSION_TEXT is referenced from Kconfig (so it needs export), > > -# and from include/config/auto.conf.cmd to detect the compiler upgrade. > > -CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1) > > - > > ifdef config-build > > # =========================================================================== > > # *config targets only - make sure prerequisites are updated, and descend > > --
On Wed, Feb 24, 2021 at 5:10 AM 'Nick Desaulniers' via Clang Built Linux <clang-built-linux@googlegroups.com> wrote: > > On Fri, Feb 5, 2021 at 5:49 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > On Sat, Feb 6, 2021 at 7:01 AM 'Nick Desaulniers' via Clang Built > > Linux <clang-built-linux@googlegroups.com> wrote: > > > > > > I noticed we're invoking $(CC) via $(shell) more than once to check the > > > version. Let's reuse the first string captured in $CC_VERSION_TEXT. > > > > > > Fixes: 315bab4e972d ("kbuild: fix endless syncconfig in case arch Makefile sets CROSS_COMPILE") > > > > > > I did not touch this hunk because I have a plan > > for different refactoring, but I have never got > > around to do it. > > > > Anyway, you beat me, and I will pick this up. > > But, the Fixes tag is questionable because > > this is code refactoring. > > Hi Masahiro, > A friendly reminder to please pick this up; I don't see it yet in > linux-next. I'm ok with you dropping the fixes tag when applied. Sorry, I had completely forgotten to do this. Thanks for the reminder. I inserted this before Nathan's "remove # characters" patch. > (Otherwise, I was about to use `grep -m1` instead of `| head -n1 | grep` here.) > > > > > > > > > > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > > --- > > > Makefile | 14 +++++++------- > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/Makefile b/Makefile > > > index a85535eb6a7d..70034d7c1051 100644 > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -557,7 +557,13 @@ ifdef building_out_of_srctree > > > { echo "# this is build directory, ignore it"; echo "*"; } > .gitignore > > > endif > > > > > > -ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),) > > > +# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included. > > > +# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile. > > > +# CC_VERSION_TEXT is referenced from Kconfig (so it needs export), > > > +# and from include/config/auto.conf.cmd to detect the compiler upgrade. > > > +CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1) > > > + > > > +ifneq ($(findstring clang,$(CC_VERSION_TEXT)),) > > > ifneq ($(CROSS_COMPILE),) > > > CLANG_FLAGS += --target=$(notdir $(CROSS_COMPILE:%-=%)) > > > GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit)) > > > @@ -576,12 +582,6 @@ KBUILD_AFLAGS += $(CLANG_FLAGS) > > > export CLANG_FLAGS > > > endif > > > > > > -# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included. > > > -# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile. > > > -# CC_VERSION_TEXT is referenced from Kconfig (so it needs export), > > > -# and from include/config/auto.conf.cmd to detect the compiler upgrade. > > > -CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1) > > > - > > > ifdef config-build > > > # =========================================================================== > > > # *config targets only - make sure prerequisites are updated, and descend > > > -- > > > -- > Thanks, > ~Nick Desaulniers > > -- > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/CAKwvOd%3Dqh0%3DtU9fr60DxcJepUfg6%2BBkqScwifW4s8A2A%3DofKpw%40mail.gmail.com.
On Sat, Feb 6, 2021 at 12:46 PM Sedat Dilek <sedat.dilek@gmail.com> wrote: > > On Sat, Feb 6, 2021 at 2:49 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > On Sat, Feb 6, 2021 at 7:01 AM 'Nick Desaulniers' via Clang Built > > Linux <clang-built-linux@googlegroups.com> wrote: > > > > > > I noticed we're invoking $(CC) via $(shell) more than once to check the > > > version. Let's reuse the first string captured in $CC_VERSION_TEXT. > > > > > > Fixes: 315bab4e972d ("kbuild: fix endless syncconfig in case arch Makefile sets CROSS_COMPILE") > > > > > > I did not touch this hunk because I have a plan > > for different refactoring, but I have never got > > around to do it. > > > > Anyway, you beat me, and I will pick this up. > > But, the Fixes tag is questionable because > > this is code refactoring. > > > > When I see this... and hear refactoring... As a suggestion/improvement... > > Can we have LD_VERSION_TEXT analogue to CC_VERSION_TEXT? > Both are shown when doing a `cat /proc/version` (and IIRC in file > include/generated/compile.h). Sorry, I had forgotten to answer this question. Probably, we should do so. Feeding it to Kconfig allows us to re-evaluate config settings when tools are upgraded. One question is where we should draw a line. If we record the version text of CC, LD, we should do that for more tools? For LD, I will be probably OK because some features rely on LD_VERSION or $(ld-option ).
On Wed, Feb 24, 2021 at 12:14:04PM +0900, Masahiro Yamada wrote: > On Sat, Feb 6, 2021 at 12:46 PM Sedat Dilek <sedat.dilek@gmail.com> wrote: > > > > On Sat, Feb 6, 2021 at 2:49 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > On Sat, Feb 6, 2021 at 7:01 AM 'Nick Desaulniers' via Clang Built > > > Linux <clang-built-linux@googlegroups.com> wrote: > > > > > > > > I noticed we're invoking $(CC) via $(shell) more than once to check the > > > > version. Let's reuse the first string captured in $CC_VERSION_TEXT. > > > > > > > > Fixes: 315bab4e972d ("kbuild: fix endless syncconfig in case arch Makefile sets CROSS_COMPILE") > > > > > > > > > I did not touch this hunk because I have a plan > > > for different refactoring, but I have never got > > > around to do it. > > > > > > Anyway, you beat me, and I will pick this up. > > > But, the Fixes tag is questionable because > > > this is code refactoring. > > > > > > > When I see this... and hear refactoring... As a suggestion/improvement... > > > > Can we have LD_VERSION_TEXT analogue to CC_VERSION_TEXT? > > Both are shown when doing a `cat /proc/version` (and IIRC in file > > include/generated/compile.h). > > Sorry, I had forgotten to answer this question. > > Probably, we should do so. > > Feeding it to Kconfig allows us to re-evaluate > config settings when tools are upgraded. > > One question is where we should draw a line. > If we record the version text of CC, LD, > we should do that for more tools? > > For LD, I will be probably OK because > some features rely on LD_VERSION or $(ld-option ). > > > -- > Best Regards > Masahiro Yamada It probably makes sense to do this for tools that we depend on in Kconfig, like CC and LD. Those will usually paint a good enough picture for what tools were used on a whole since CC will tell us about the GCC or LLVM version used (as well as distribution usually) and LD will tell us about the binutils or LLVM version. LTO does check $(AR) and $(NM) but it also depends on LLVM=1 LLVM_IAS=1, which would get covered by the $(CC) and $(LD) print outs. Cheers, Nathan
diff --git a/Makefile b/Makefile index a85535eb6a7d..70034d7c1051 100644 --- a/Makefile +++ b/Makefile @@ -557,7 +557,13 @@ ifdef building_out_of_srctree { echo "# this is build directory, ignore it"; echo "*"; } > .gitignore endif -ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),) +# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included. +# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile. +# CC_VERSION_TEXT is referenced from Kconfig (so it needs export), +# and from include/config/auto.conf.cmd to detect the compiler upgrade. +CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1) + +ifneq ($(findstring clang,$(CC_VERSION_TEXT)),) ifneq ($(CROSS_COMPILE),) CLANG_FLAGS += --target=$(notdir $(CROSS_COMPILE:%-=%)) GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit)) @@ -576,12 +582,6 @@ KBUILD_AFLAGS += $(CLANG_FLAGS) export CLANG_FLAGS endif -# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included. -# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile. -# CC_VERSION_TEXT is referenced from Kconfig (so it needs export), -# and from include/config/auto.conf.cmd to detect the compiler upgrade. -CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1) - ifdef config-build # =========================================================================== # *config targets only - make sure prerequisites are updated, and descend
I noticed we're invoking $(CC) via $(shell) more than once to check the version. Let's reuse the first string captured in $CC_VERSION_TEXT. Fixes: 315bab4e972d ("kbuild: fix endless syncconfig in case arch Makefile sets CROSS_COMPILE") Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- Makefile | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)