Re: [Xen-devel] [XEN PATCH v3 2/6] xen: Have Kconfig check $(CC)'s version
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
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
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
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
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
