diff mbox

[v2,1/5] Remove hardcoded strict -Werror checking

Message ID CAKmqyKMorsHFCRWVik5BPeALoyZU6OPtrESf8WTAU1_-mXmiiw@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alistair Francis Dec. 22, 2016, 10:11 p.m. UTC
On Thu, Dec 22, 2016 at 2:00 PM, Doug Goldstein <cardoe@cardoe.com> wrote:
> On 12/22/16 3:47 PM, Andrew Cooper wrote:
>> On 22/12/16 21:41, Alistair Francis wrote:
>>> On Thu, Dec 22, 2016 at 1:15 PM, Alistair Francis
>>> <alistair.francis@xilinx.com> wrote:
>>>> On Thu, Dec 22, 2016 at 1:12 PM, Alistair Francis
>>>> <alistair.francis@xilinx.com> wrote:
>>>>> On Thu, Dec 22, 2016 at 11:22 AM, Ian Jackson
>>>>> <ian.jackson@eu.citrix.com> wrote:
>>>>>> Alistair Francis writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove
>>>>>> hardcoded strict -Werror checking"):
>>>>>>> On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com>
>>>>>>> wrote:
>>>>>>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote:
>>>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>>>>> Without some rationale given I don't think such changes are
>>>>>>>> acceptable at all. And then, as already pointed out others, the
>>>>>>>> use of -Werror is there not just for fun. If anything I think an
>>>>>>>> override to that default could be acceptable.
>>>>>>> Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
>>>>>>> as I still see warnings/errors when building: tools/kconfig/conf.c.
>>>>>> That sounds like a bug to me.  Do you know why it's not effective
>>>>>> there ?
>>>>> It actually might be an issue in the way buildroot is handling the
>>>>> arguments.
>>>>>
>>>>> I'll look into it and see what I find after the holidays.
>>> I dug into this a little more. Adding the APPEND_CFLAGS="-Wno-error"
>>> fixes almost everything. The only problem I see is in the log below,
>>> where tools/kconfig/conf.c fails to build as the -Wno-error doesn't
>>> propagate down.
>>>
>>> If I manage to find a fix today I'll send it, otherwise this can wait
>>> until next year.
>>
>> Something like this?
>>
>> diff --git a/xen/Makefile b/xen/Makefile
>> index dc6862e04d..2d7a567c9c 100644
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -253,14 +253,14 @@ kconfig := silentoldconfig oldconfig config
>> menuconfig defconfig \
>>         randconfig
>>  .PHONY: $(kconfig)
>>  $(kconfig):
>> -       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" $@
>> +       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>> HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" $@
>>
>>  include/config/%.conf: include/config/auto.conf.cmd $(KCONFIG_CONFIG)
>> -       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>> silentoldconfig
>> +       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>> HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" silentoldconfig
>>
>>  # Allow people to just run `make` as before and not force them to
>> configure
>>  $(KCONFIG_CONFIG):
>> -       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>> defconfig
>> +       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>> HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" defconfig
>>
>>  # Break the dependency chain for the first run
>>  include/config/auto.conf.cmd: ;
>>
>
> That should do the trick.
>
> Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

I got this to work as well:

 # Shared Makefile for the various kconfig executables:

But yours looks like it should work as well.

Do you want to send a patch?

Thanks,

Alistair

>
> --
> Doug Goldstein
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
>

Comments

Alistair Francis Dec. 22, 2016, 10:16 p.m. UTC | #1
On Thu, Dec 22, 2016 at 2:11 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Thu, Dec 22, 2016 at 2:00 PM, Doug Goldstein <cardoe@cardoe.com> wrote:
>> On 12/22/16 3:47 PM, Andrew Cooper wrote:
>>> On 22/12/16 21:41, Alistair Francis wrote:
>>>> On Thu, Dec 22, 2016 at 1:15 PM, Alistair Francis
>>>> <alistair.francis@xilinx.com> wrote:
>>>>> On Thu, Dec 22, 2016 at 1:12 PM, Alistair Francis
>>>>> <alistair.francis@xilinx.com> wrote:
>>>>>> On Thu, Dec 22, 2016 at 11:22 AM, Ian Jackson
>>>>>> <ian.jackson@eu.citrix.com> wrote:
>>>>>>> Alistair Francis writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove
>>>>>>> hardcoded strict -Werror checking"):
>>>>>>>> On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com>
>>>>>>>> wrote:
>>>>>>>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote:
>>>>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>>>>>> Without some rationale given I don't think such changes are
>>>>>>>>> acceptable at all. And then, as already pointed out others, the
>>>>>>>>> use of -Werror is there not just for fun. If anything I think an
>>>>>>>>> override to that default could be acceptable.
>>>>>>>> Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
>>>>>>>> as I still see warnings/errors when building: tools/kconfig/conf.c.
>>>>>>> That sounds like a bug to me.  Do you know why it's not effective
>>>>>>> there ?
>>>>>> It actually might be an issue in the way buildroot is handling the
>>>>>> arguments.
>>>>>>
>>>>>> I'll look into it and see what I find after the holidays.
>>>> I dug into this a little more. Adding the APPEND_CFLAGS="-Wno-error"
>>>> fixes almost everything. The only problem I see is in the log below,
>>>> where tools/kconfig/conf.c fails to build as the -Wno-error doesn't
>>>> propagate down.
>>>>
>>>> If I manage to find a fix today I'll send it, otherwise this can wait
>>>> until next year.
>>>
>>> Something like this?
>>>
>>> diff --git a/xen/Makefile b/xen/Makefile
>>> index dc6862e04d..2d7a567c9c 100644
>>> --- a/xen/Makefile
>>> +++ b/xen/Makefile
>>> @@ -253,14 +253,14 @@ kconfig := silentoldconfig oldconfig config
>>> menuconfig defconfig \
>>>         randconfig
>>>  .PHONY: $(kconfig)
>>>  $(kconfig):
>>> -       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" $@
>>> +       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>>> HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" $@
>>>
>>>  include/config/%.conf: include/config/auto.conf.cmd $(KCONFIG_CONFIG)
>>> -       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>>> silentoldconfig
>>> +       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>>> HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" silentoldconfig
>>>
>>>  # Allow people to just run `make` as before and not force them to
>>> configure
>>>  $(KCONFIG_CONFIG):
>>> -       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>>> defconfig
>>> +       $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig
>>> ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)"
>>> HOST_EXTRACFLAGS="$(APPEND_CFLAGS)" defconfig
>>>
>>>  # Break the dependency chain for the first run
>>>  include/config/auto.conf.cmd: ;
>>>
>>
>> That should do the trick.
>>
>> Reviewed-by: Doug Goldstein <cardoe@cardoe.com>
>
> I got this to work as well:
>
> diff --git a/xen/tools/kconfig/Makefile b/xen/tools/kconfig/Makefile
> index aceaaed..32e2359 100644
> --- a/xen/tools/kconfig/Makefile
> +++ b/xen/tools/kconfig/Makefile
> @@ -155,7 +155,7 @@ check-lxdialog  :=
> $(srctree)/$(src)/lxdialog/check-lxdialog.sh
>  # Use recursively expanded variables so we do not call gcc unless
>  # we really need to do so. (Do not call gcc as part of make mrproper)
>  HOST_EXTRACFLAGS += $(shell $(CONFIG_SHELL) $(check-lxdialog) -ccflags) \
> -                    -DLOCALE
> +                    -DLOCALE $(APPEND_CFLAGS)
>
>  # ===========================================================================
>  # Shared Makefile for the various kconfig executables:
>
> But yours looks like it should work as well.
>
> Do you want to send a patch?

You can add this to it:

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
Tested-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

>
> Thanks,
>
> Alistair
>
>>
>> --
>> Doug Goldstein
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> https://lists.xen.org/xen-devel
>>
diff mbox

Patch

diff --git a/xen/tools/kconfig/Makefile b/xen/tools/kconfig/Makefile
index aceaaed..32e2359 100644
--- a/xen/tools/kconfig/Makefile
+++ b/xen/tools/kconfig/Makefile
@@ -155,7 +155,7 @@  check-lxdialog  :=
$(srctree)/$(src)/lxdialog/check-lxdialog.sh
 # Use recursively expanded variables so we do not call gcc unless
 # we really need to do so. (Do not call gcc as part of make mrproper)
 HOST_EXTRACFLAGS += $(shell $(CONFIG_SHELL) $(check-lxdialog) -ccflags) \
-                    -DLOCALE
+                    -DLOCALE $(APPEND_CFLAGS)

 # ===========================================================================