Re: [PATCH kvmtool] Makefile: 'lvm version' works incorrect. Because CFLAGS can not get sub-make variable $(KVMTOOLS_VERSION)

From: "haibiao.xiao" <haibiao.xiao@zstack.io>
To: Andre Przywara <andre.przywara@arm.com>
Cc: kvm@vger.kernel.org, will@kernel.org,
	julien.thierry.kdev@gmail.com,
	"haibiao.xiao" <xiaohaibiao331@outlook.com>,
	Alexandru Elisei <Alexandru.Elisei@arm.com>
Subject: Re: [PATCH kvmtool] Makefile: 'lvm version' works incorrect. Because CFLAGS can not get sub-make variable $(KVMTOOLS_VERSION)
Date: Mon, 13 Dec 2021 11:19:02 +0800	[thread overview]
Message-ID: <9b41e9ec-6e8e-855b-a002-0914d8b6a3db@zstack.io> (raw)
In-Reply-To: <20211210110002.137a2fc3@donnerap.cambridge.arm.com>

Hi,

Sorry for getting back to you late.

Fri, 10 Dec 2021 11:00:02 +0000, Andre Przywara wrote:
> On Fri, 10 Dec 2021 10:55:40 +0800
> "haibiao.xiao" <haibiao.xiao@zstack.io> wrote:
> 
> Hi,
> 
>> On Thu, 9 Dec 2021 15:57:46 +0000, Andre Przywara wrote:
>>> On Sat,  4 Dec 2021 14:14:36 +0800
>>> "haibiao.xiao" <haibiao.xiao@zstack.io> wrote:
>>>
>>> Hi,
>>>   
>>>> From: "haibiao.xiao" <xiaohaibiao331@outlook.com>
>>>>
>>>> Command 'lvm version' works incorrect.
>>>> It is expected to print:
>>>>
>>>>     # ./lvm version
>>>>     # kvm tool [KVMTOOLS_VERSION]
>>>>
>>>> but the KVMTOOLS_VERSION is missed:
>>>>
>>>>     # ./lvm version
>>>>     # kvm tool
>>>>
>>>> The KVMTOOLS_VERSION is defined in the KVMTOOLS-VERSION-FILE file which
>>>> is included at the end of Makefile. Since the CFLAGS is a 'Simply
>>>> expanded variables' which means CFLAGS is only scanned once. So the
>>>> definetion of KVMTOOLS_VERSION at the end of Makefile would not scanned
>>>> by CFLAGS. So the '-DKVMTOOLS_VERSION=' remains empty.
>>>>
>>>> I fixed the bug by moving the '-include $(OUTPUT)KVMTOOLS-VERSION-FILE'
>>>> before the CFLAGS.  
>>>
>>> While this is indeed a bug that this patch fixes, I wonder if we should
>>> actually get rid of this whole versioning attempt altogether at this
>>> point. Originally this was following the containing kernel version, but
>>> it is stuck ever since at v3.18, without any change.
>>>
>>> So either we introduce proper versioning (not sure it's worth it?), or we
>>> just remove all code that pretends to print a version number? Or just
>>> hardcode v3.18 into the printf, at least for now? At the very least I
>>> think we don't need a KVMTOOLS-VERSION-FILE anymore.
>>>   
>>
>> Thanks for your reply. 
>>
>> The reason I look at this project is tend to learn something about kvm. 
>> With the version number I can tell which kernel(kvm) is compatible.
> 
> I am afraid this is not what the version number tells you. The Linux
> kernel provides a stable interface to userland, that includes KVM.
> So you should be able to run any kvmtool version on any host kernel
> (ignoring bugs). Granted, you will only get the features implemented in
> both parts, but this only applies to new features (like PMU
> virtualisation), not the main functionality.
> 

I thought about it again. Yes, you are right.

>> Although it is stuck at v3.18, there still some commits in recent 
>> months, which means the kvmtool still changing according to the kvm 
>> features.
> 
> Sure, we keep it alive, but adapting to new kernel features is only one
> part of that. Recently we mostly improved functionality (like
> adding firmware support, emulating later PCIe versions, ...) and were
> fixing bugs, independent of the kernel version.
> 
>> So I think what's kvmtool need is a version control, but not 
>> remove/hardcode.
> 
> So whether that's really worth it, is the question. If you need some number
> to compare, distributions tend to use the date of the last commit for that.
> 

And I reviewed the code about version generation this weekends. I found it
was already hardcode to 3.18.0 in `kvmtool/util/KVMTOOLS-VERSION-GEN`. What's
more, it leaves a way to change the version number by getting from git 
describe.

> Cheers,
> Andre
> 
> 
>>
>> Thanks,
>> haibiao.xiao
>>> Cheers,
>>> Andre
>>>   
>>>>
>>>> Signed-off-by: haibiao.xiao <xiaohaibiao331@outlook.com>
>>>> ---
>>>>  Makefile | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Makefile b/Makefile
>>>> index bb7ad3e..9afb5e3 100644
>>>> --- a/Makefile
>>>> +++ b/Makefile
>>>> @@ -17,6 +17,7 @@ export E Q
>>>>  
>>>>  include config/utilities.mak
>>>>  include config/feature-tests.mak
>>>> +-include $(OUTPUT)KVMTOOLS-VERSION-FILE
>>>>  
>>>>  CC	:= $(CROSS_COMPILE)gcc
>>>>  CFLAGS	:=
>>>> @@ -559,5 +560,4 @@ ifneq ($(MAKECMDGOALS),clean)
>>>>  
>>>>  KVMTOOLS-VERSION-FILE:
>>>>  	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
>>>> --include $(OUTPUT)KVMTOOLS-VERSION-FILE
>>>> -endif
>>>> +endif
>>>> \ No newline at end of file  