Re: [Xen-devel] [PATCH v2 2/2] arm: rename tiny64.conf to tiny64_defconfig

[Julien Grall <julien.grall@arm.com>]

Hi Volodymyr,

Sorry for the late reply.

On 5/20/19 3:57 PM, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
>> Hi,
>>
>> On 20/05/2019 14:41, Volodymyr Babchuk wrote:
>>> Julien Grall writes:
>>>
>>>> Hi,
>>>>
>>>> First of all, please add a cover letter when you send a series. This
>>>> help for threading and also a place to commend on general feedback.
>>> Oh, okay. That was quite simple change and I didn't wanted to spam with
>>> extra emails. I will include cover letter next time.
>>>
>>>> Furthermore, please use scripts/{add, get}_maintainers.pl to find the
>>>> correct maintainers. While I agree that CCing REST is a good idea, you
>>>> haven't CCed all of them.
>>> Problem is that I used this script:
>>>
>>> $ ./scripts/get_maintainer.pl -f defconfig_v2/v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch
>>
>> -f is to be used on actual file in the source tree. So the result
>> below makes sense. For actual patch, you have to drop the -f.
> Ah, I see. Without -f I'm getting the same message as with
> add-maintainers.pl:
> 
> % ./scripts/get_maintainer.pl defconfig_v2/v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch
> ./scripts/get_maintainer.pl: file 'defconfig_v2/v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch' doesn't appear to be a patch.  Add -f to options?
> 
> [...]
> 
>>>
>>> % scripts/add_maintainers.pl -v 2 -d defconfig_v2
>>> Processing: v2-0001-makefile-add-support-for-_defconfig-targets.patch
>>> Processing: v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch
>>> ./scripts/get_maintainer.pl: file 'defconfig_v2/v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch' doesn't appear to be a patch.  Add -f to options?
>>
>> I have just tried it and can't find the same error. Could you provide
>> more details? Such as where to do call from the exact content of each
>> patches...
> 
> My basic flow:
> 
> % git format-patch -v2 -2 -o defconfig_v2
> % scripts/add_maintainers.pl -v 2 -d defconfig_v2
> Processing: v2-0001-makefile-add-support-for-_defconfig-targets.patch
> Processing: v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch
> ./scripts/get_maintainer.pl: file 'defconfig_v2/v2-0002-arm-rename-tiny64.conf-to-tiny64_defconfig.patch' doesn't appear to be a patch.  Add -f to options?
> Then perform:
> git send-email -to xen-devel@lists.xenproject.org defconfig_v2/v2-*.patch
> 
> 
> HEAD (prior to my patches) is at
> 278c64519c661c851d37e2a929f006fb8a1dcd01
> 
> git version 2.21.0
> 
> Contents of the patch is the exactly the same as in my original
> email. You can find both patches at [1].

It looks like the problem is because the second patch only contains 
renaming. Linux recently fixed it with the following commit:

0455c74788fd "get_maintainer: improve patch recognition"

I guess we need to port the patch in Xen. Volodymyr, would you mind to 
send a patch for it?

> 
>>>
>>>>
>>>> On 16/05/2019 14:37, Volodymyr Babchuk wrote:
>>>>> As build system now supports *_defconfig rules it is good to be able
>>>>> to configure minimal XEN image with
>>>>
>>>> I am afraid this is not correct. tiny64 will not be able to generate a
>>>> minimal config to boot on any platform supported by Xen.
>>>>
>>>> It is meant to be used as a base for tailoring your platform where all
>>>> the options are turned off by default.
>>>>
>>>> So I think offering a direct access is likely going to be misused in
>>>> most of the cases without proper documentation.
>>>
>>> In the original commit message Stefano suggested to use olddefconfig:
>>>
>>> "   Add a tiny kconfig configuration. Enabled only the credit scheduler.
>>>       It only carries non-default options (use make menuconfig or make
>>>       olddefconfig to produce a complete .config file). "
>>>
>>> I don't see any significant difference between
>>
>> Did you actually try the two approach and see how they differ?
> 
> Yes. I did the following:
> 
> % cp arch/arm/configs/tiny64_defconfig .config
> % make olddefconfig
> make -f /home/lorc/work/xen/xen/tools/kconfig/Makefile.kconfig ARCH=arm64 SRCARCH=arm HOSTCC="gcc" HOSTCXX="g++" olddefconfig
> make[1]: Entering directory '/home/lorc/work/xen/xen'
> gcc -Wp,-MD,tools/kconfig/.conf.o.d    -D_GNU_SOURCE -DCURSES_LOC="<ncurses.h>" -DNCURSES_WIDECHAR=1 -DLOCALE   -c -o tools/kconfig/conf.o tools/kconfig/conf.c
> gcc -Wp,-MD,tools/kconfig/.zconf.tab.o.d    -D_GNU_SOURCE -DCURSES_LOC="<ncurses.h>" -DNCURSES_WIDECHAR=1 -DLOCALE  -Itools/kconfig -c -o tools/kconfig/zconf.tab.o tools/kconfig/zconf.tab.c
> gcc  -o tools/kconfig/conf tools/kconfig/conf.o tools/kconfig/zconf.tab.o
> tools/kconfig/conf -s --olddefconfig Kconfig
> make[1]: Leaving directory '/home/lorc/work/xen/xen'
> 
> And
> 
> % make tiny64_defconfig
> make -f /home/lorc/work/xen/xen/tools/kconfig/Makefile.kconfig ARCH=arm64 SRCARCH=arm HOSTCC="gcc" HOSTCXX="g++" tiny64_defconfig
> make[1]: Entering directory '/home/lorc/work/xen/xen'
> gcc -Wp,-MD,tools/kconfig/.conf.o.d    -D_GNU_SOURCE -DCURSES_LOC="<ncurses.h>" -DNCURSES_WIDECHAR=1 -DLOCALE   -c -o tools/kconfig/conf.o tools/kconfig/conf.c
> gcc -Wp,-MD,tools/kconfig/.zconf.tab.o.d    -D_GNU_SOURCE -DCURSES_LOC="<ncurses.h>" -DNCURSES_WIDECHAR=1 -DLOCALE  -Itools/kconfig -c -o tools/kconfig/zconf.tab.o tools/kconfig/zconf.tab.c
> gcc  -o tools/kconfig/conf tools/kconfig/conf.o tools/kconfig/zconf.tab.o
> make[1]: Leaving directory '/home/lorc/work/xen/xen'
> 
> Then I compared both .config files and found no difference at all:
> 
> % diff -u .config1 .config2
> (displayed nothing)
> 
>>>
>>> # cp tiny64.conf .config && make olddefconfig
>>
>> This one will ask you details on the configuration you want while...
> 
> But it does not, while "make oldconfig" does. Are you sure you are not
> confusing oldconfig and olddefconfig targets?

I am confusing both :(. Sorry for the noise.

> 
>>>
>>> and
>>>
>>> # make tiny64_defconfig
>>
>> ... this one will hide the questions.
>>
>>>
>>> Anyways, it is up to you to accept or decline this particular patch. I
>>> mostly interested in the first patch in the series, because our build
>>> system depends on it. This very patch I sent out only because I wanted
>>> to tidy up things a bit. But if you are saying that it is intended to
>>> store minimal config in this way, I'm okay with it.
>>
>> The point of review is to discuss on the approach and find a common agreement.
>>
>> If you read my previous e-mail, I didn't completely reject the
>> approach in my previous e-mail. I pointed out that the user may be
>> misled of the name and hence documentation would be useful.
> 
> I'm okay with this. Any ideas how to document it?

We don't seem to have a place today where we document the defconfig. I 
am thinking to put that in docs/misc/arm.

I would document the purpose of each config. The documentation could be 
in a separate patch.

Cheers,

> 
> [1] https://github.com/lorc/xen/commits/defconfig_v2
> 

-- 
Julien Grall

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