Re: [Xen-devel] [XEN PATCH v3 2/6] xen: Have Kconfig check $(CC)'s version

2020-01-20 Thread Jan Beulich
On 17.01.2020 17:23, Anthony PERARD wrote:
> On Thu, Jan 16, 2020 at 01:40:39PM +0100, Jan Beulich wrote:
>> On 16.01.2020 13:29, Anthony PERARD wrote:
>> Indeed, hence also my "as suggested before". I remain unconvinced
>> that is it useful to have e.g.
>>
>> CONFIG_GCC_VERSION=80300
>> CONFIG_CLANG_VERSION=0
>>
>> in xen/.config. This is at best confusing, because it may not
>> represent what the system actually has installed (which I realize
>> is also not the intention, but the variable naming suggests that
>> this is what was found on the system; I have no better naming
>> suggestion, to preempt a possible question to this effect).
> 
> After a talk on #xendevel yesterday, I have George agreeing that we
> should keep the same behavior as the one Linux have. And Ian saying that
> we should copy entire files where we can. If we modify the behavior of
> %_VERSION, it would make it more difficult to copy entire files from
> %Linux later.
> 
> So, now, can we finally commit the patch series, with both %_VERSION
> set, and let this bikeshedding rest, and move on?

Well, someone feel free to go ahead then. Every time I'll get to see
this I'll be tempted to clean up the redundancy, but so be it. (I
don't view this as bike shedding at all, btw. While every individual
instance may not matter much, it is the sum of them that I'm worried
about.)

Jan

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 2/6] xen: Have Kconfig check $(CC)'s version

2020-01-17 Thread Anthony PERARD
On Thu, Jan 16, 2020 at 01:40:39PM +0100, Jan Beulich wrote:
> On 16.01.2020 13:29, Anthony PERARD wrote:
> Indeed, hence also my "as suggested before". I remain unconvinced
> that is it useful to have e.g.
> 
> CONFIG_GCC_VERSION=80300
> CONFIG_CLANG_VERSION=0
> 
> in xen/.config. This is at best confusing, because it may not
> represent what the system actually has installed (which I realize
> is also not the intention, but the variable naming suggests that
> this is what was found on the system; I have no better naming
> suggestion, to preempt a possible question to this effect).

After a talk on #xendevel yesterday, I have George agreeing that we
should keep the same behavior as the one Linux have. And Ian saying that
we should copy entire files where we can. If we modify the behavior of
%_VERSION, it would make it more difficult to copy entire files from
%Linux later.

So, now, can we finally commit the patch series, with both %_VERSION
set, and let this bikeshedding rest, and move on?

Thank you,

-- 
Anthony PERARD

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 2/6] xen: Have Kconfig check $(CC)'s version

2020-01-16 Thread Jan Beulich
On 16.01.2020 13:29, Anthony PERARD wrote:
> On Thu, Jan 16, 2020 at 12:30:49PM +0100, Jan Beulich wrote:
>> On 15.01.2020 18:00, Anthony PERARD wrote:
>>> --- a/xen/Kconfig
>>> +++ b/xen/Kconfig
>>> @@ -4,9 +4,25 @@
>>>  #
>>>  mainmenu "Xen/$(SRCARCH) $(XEN_FULLVERSION) Configuration"
>>>  
>>> +source "scripts/Kconfig.include"
>>> +
>>>  config BROKEN
>>> bool
>>>  
>>> +config CC_IS_GCC
>>> +   def_bool $(success,$(CC) --version | head -n 1 | grep -q gcc)
>>> +
>>> +config GCC_VERSION
>>> +   int
>>> +   default $(shell,$(BASEDIR)/scripts/gcc-version.sh $(CC))
>>> +
>>> +config CC_IS_CLANG
>>> +   def_bool $(success,$(CC) --version | head -n 1 | grep -q clang)
>>> +
>>> +config CLANG_VERSION
>>> +   int
>>> +   default $(shell,$(BASEDIR)/scripts/clang-version.sh $(CC))
>>
>> I continue to be unhappy about the redundancy, but I will accept
>> it if others indeed think this is helpful. However, I don't see
>> then why the setting of CC_IS_* need another shell invocation
>> each - this could just be *_VERSION > 0 then, seeing that the
>> scripts already to a respective grep of the --version output.
> 
> From functionality point of view, replacing the macro by
> "def_bool %_VERSION > 0" in "config CC_IS_%" would be fine, even so it
> would be weird to read. I think that would need a comment saying:
>   # %-version.sh is expected to return "0" when $(CC) isn't %
> 
> That could be done on commit.

Sure.

>> Even better would imo be, as suggested before, a "depends on
>> CC_IS_*" on each *_VERSION.
> 
> Haven't we discussed this before?

Indeed, hence also my "as suggested before". I remain unconvinced
that is it useful to have e.g.

CONFIG_GCC_VERSION=80300
CONFIG_CLANG_VERSION=0

in xen/.config. This is at best confusing, because it may not
represent what the system actually has installed (which I realize
is also not the intention, but the variable naming suggests that
this is what was found on the system; I have no better naming
suggestion, to preempt a possible question to this effect).

>> As a nit - common style elsewhere would suggest that there ought
>> to be a blank after the commas in $(macro, ...) invocations.
>> This would then extend to Kconfig.include as well, unless that's
>> a largely verbatim inherited file.
> 
> That's not a good idea, it may not matter in this case but adding a
> space after commas in some other cases will not do what one wants. make
> and Kconfig keeps the spaces when expanding the macro.

Where blanks matter they should of course be omitted. When they
don't matter, personally I think common style guidelines should be
honored. But this may be just me ...

Jan

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 2/6] xen: Have Kconfig check $(CC)'s version

2020-01-16 Thread Anthony PERARD
On Thu, Jan 16, 2020 at 12:30:49PM +0100, Jan Beulich wrote:
> On 15.01.2020 18:00, Anthony PERARD wrote:
> > --- a/xen/Kconfig
> > +++ b/xen/Kconfig
> > @@ -4,9 +4,25 @@
> >  #
> >  mainmenu "Xen/$(SRCARCH) $(XEN_FULLVERSION) Configuration"
> >  
> > +source "scripts/Kconfig.include"
> > +
> >  config BROKEN
> > bool
> >  
> > +config CC_IS_GCC
> > +   def_bool $(success,$(CC) --version | head -n 1 | grep -q gcc)
> > +
> > +config GCC_VERSION
> > +   int
> > +   default $(shell,$(BASEDIR)/scripts/gcc-version.sh $(CC))
> > +
> > +config CC_IS_CLANG
> > +   def_bool $(success,$(CC) --version | head -n 1 | grep -q clang)
> > +
> > +config CLANG_VERSION
> > +   int
> > +   default $(shell,$(BASEDIR)/scripts/clang-version.sh $(CC))
> 
> I continue to be unhappy about the redundancy, but I will accept
> it if others indeed think this is helpful. However, I don't see
> then why the setting of CC_IS_* need another shell invocation
> each - this could just be *_VERSION > 0 then, seeing that the
> scripts already to a respective grep of the --version output.

From functionality point of view, replacing the macro by
"def_bool %_VERSION > 0" in "config CC_IS_%" would be fine, even so it
would be weird to read. I think that would need a comment saying:
  # %-version.sh is expected to return "0" when $(CC) isn't %

That could be done on commit.


> Even better would imo be, as suggested before, a "depends on
> CC_IS_*" on each *_VERSION.

Haven't we discussed this before?

> As a nit - common style elsewhere would suggest that there ought
> to be a blank after the commas in $(macro, ...) invocations.
> This would then extend to Kconfig.include as well, unless that's
> a largely verbatim inherited file.

That's not a good idea, it may not matter in this case but adding a
space after commas in some other cases will not do what one wants. make
and Kconfig keeps the spaces when expanding the macro.

-- 
Anthony PERARD

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3 2/6] xen: Have Kconfig check $(CC)'s version

2020-01-16 Thread Jan Beulich
On 15.01.2020 18:00, Anthony PERARD wrote:
> --- a/xen/Kconfig
> +++ b/xen/Kconfig
> @@ -4,9 +4,25 @@
>  #
>  mainmenu "Xen/$(SRCARCH) $(XEN_FULLVERSION) Configuration"
>  
> +source "scripts/Kconfig.include"
> +
>  config BROKEN
>   bool
>  
> +config CC_IS_GCC
> + def_bool $(success,$(CC) --version | head -n 1 | grep -q gcc)
> +
> +config GCC_VERSION
> + int
> + default $(shell,$(BASEDIR)/scripts/gcc-version.sh $(CC))
> +
> +config CC_IS_CLANG
> + def_bool $(success,$(CC) --version | head -n 1 | grep -q clang)
> +
> +config CLANG_VERSION
> + int
> + default $(shell,$(BASEDIR)/scripts/clang-version.sh $(CC))

I continue to be unhappy about the redundancy, but I will accept
it if others indeed think this is helpful. However, I don't see
then why the setting of CC_IS_* need another shell invocation
each - this could just be *_VERSION > 0 then, seeing that the
scripts already to a respective grep of the --version output.

Even better would imo be, as suggested before, a "depends on
CC_IS_*" on each *_VERSION.

As a nit - common style elsewhere would suggest that there ought
to be a blank after the commas in $(macro, ...) invocations.
This would then extend to Kconfig.include as well, unless that's
a largely verbatim inherited file.

Jan

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [XEN PATCH v3 2/6] xen: Have Kconfig check $(CC)'s version

2020-01-15 Thread Anthony PERARD
This import several files from Linux v5.3
 - scripts/Kconfig.include
 - scripts/clang-version.sh
 - scripts/gcc-version.sh
 and several config values from from Linux's init/Kconfig file.
But gcc-version.sh have been modified to return "0" when $CC isn't
GCC, like clang-version.sh do.

Files are copied into scripts/ directory because that's were the files
are found in Linux tree, and also because we are going to import more
of Kbuild from Linux which is located in scripts/.

CONFIG_GCC_VERSION and CONFIG_CC_IS_CLANG are going to be use in
follow-up patches.

Signed-off-by: Anthony PERARD 
Acked-by: Andrew Cooper 
---

Notes:
v3:
- Have gcc-version behave like clang-version and return 0 when the
  compiler tested isn't the expected one.

v2:
- move the export CC* earlier in xen/Makefile, and add CXX to the list.

 xen/Kconfig  | 16 +++
 xen/Makefile |  2 ++
 xen/scripts/Kconfig.include  | 39 
 xen/scripts/clang-version.sh | 19 ++
 xen/scripts/gcc-version.sh   | 25 +++
 5 files changed, 101 insertions(+)
 create mode 100644 xen/scripts/Kconfig.include
 create mode 100755 xen/scripts/clang-version.sh
 create mode 100755 xen/scripts/gcc-version.sh

diff --git a/xen/Kconfig b/xen/Kconfig
index 01067326b4e7..57427927abf0 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -4,9 +4,25 @@
 #
 mainmenu "Xen/$(SRCARCH) $(XEN_FULLVERSION) Configuration"
 
+source "scripts/Kconfig.include"
+
 config BROKEN
bool
 
+config CC_IS_GCC
+   def_bool $(success,$(CC) --version | head -n 1 | grep -q gcc)
+
+config GCC_VERSION
+   int
+   default $(shell,$(BASEDIR)/scripts/gcc-version.sh $(CC))
+
+config CC_IS_CLANG
+   def_bool $(success,$(CC) --version | head -n 1 | grep -q clang)
+
+config CLANG_VERSION
+   int
+   default $(shell,$(BASEDIR)/scripts/clang-version.sh $(CC))
+
 source "arch/$(SRCARCH)/Kconfig"
 
 config DEFCONFIG_LIST
diff --git a/xen/Makefile b/xen/Makefile
index efbe9605e52b..c326fee5880e 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -18,6 +18,8 @@ export XEN_CONFIG_EXPERT ?= n
 PYTHON_INTERPRETER := $(word 1,$(shell which python3 python python2 
2>/dev/null) python)
 export PYTHON  ?= $(PYTHON_INTERPRETER)
 
+export CC CXX LD
+
 export BASEDIR := $(CURDIR)
 export XEN_ROOT := $(BASEDIR)/..
 
diff --git a/xen/scripts/Kconfig.include b/xen/scripts/Kconfig.include
new file mode 100644
index ..8221095ca34b
--- /dev/null
+++ b/xen/scripts/Kconfig.include
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: GPL-2.0-only
+# Kconfig helper macros
+
+# Convenient variables
+comma   := ,
+quote   := "
+squote  := '
+empty   :=
+space   := $(empty) $(empty)
+dollar  := $
+right_paren := )
+left_paren  := (
+
+# $(if-success,,,)
+# Return  if  exits with 0,  otherwise.
+if-success = $(shell,{ $(1); } >/dev/null 2>&1 && echo "$(2)" || echo "$(3)")
+
+# $(success,)
+# Return y if  exits with 0, n otherwise
+success = $(if-success,$(1),y,n)
+
+# $(failure,)
+# Return n if  exits with 0, y otherwise
+failure = $(if-success,$(1),n,y)
+
+# $(cc-option,)
+# Return y if the compiler supports , n otherwise
+cc-option = $(success,$(CC) -Werror $(CLANG_FLAGS) $(1) -E -x c /dev/null -o 
/dev/null)
+
+# $(ld-option,)
+# Return y if the linker supports , n otherwise
+ld-option = $(success,$(LD) -v $(1))
+
+# check if $(CC) and $(LD) exist
+$(error-if,$(failure,command -v $(CC)),compiler '$(CC)' not found)
+$(error-if,$(failure,command -v $(LD)),linker '$(LD)' not found)
+
+# gcc version including patch level
+gcc-version := $(shell,$(BASEDIR)/scripts/gcc-version.sh $(CC))
diff --git a/xen/scripts/clang-version.sh b/xen/scripts/clang-version.sh
new file mode 100755
index ..6fabf0695761
--- /dev/null
+++ b/xen/scripts/clang-version.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# clang-version clang-command
+#
+# Print the compiler version of `clang-command' in a 5 or 6-digit form
+# such as `50001' for clang-5.0.1 etc.
+
+compiler="$*"
+
+if ! ( $compiler --version | grep -q clang) ; then
+   echo 0
+   exit 1
+fi
+
+MAJOR=$(echo __clang_major__ | $compiler -E -x c - | tail -n 1)
+MINOR=$(echo __clang_minor__ | $compiler -E -x c - | tail -n 1)
+PATCHLEVEL=$(echo __clang_patchlevel__ | $compiler -E -x c - | tail -n 1)
+printf "%d%02d%02d\\n" $MAJOR $MINOR $PATCHLEVEL
diff --git a/xen/scripts/gcc-version.sh b/xen/scripts/gcc-version.sh
new file mode 100755
index ..b3261949dea5
--- /dev/null
+++ b/xen/scripts/gcc-version.sh
@@ -0,0 +1,25 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# gcc-version gcc-command
+#
+# Print the gcc version of `gcc-command' in a 5 or 6-digit form
+# such as `29503' for gcc-2.95.3, `30301' for gcc-3.3.1, etc.
+
+compiler="$*"
+
+if [ ${#compiler} -eq 0 ]; then
+   echo "Error: No compiler specified." >&2
+   printf "Usage:\n\t$0 \n"