Re: [PATCH] tests/vpci: install test

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.03.2023 14:38, Roger Pau Monné wrote:
> On Fri, Mar 10, 2023 at 12:06:29PM +0100, Jan Beulich wrote:
>> On 09.03.2023 17:58, Roger Pau Monne wrote:
>>> Introduce an install target, like it's used by other tests.  This
>>> allows running the test on the installed systems, which is easier than
>>> running it during the build phase when dealing with automated testing.
>>> Strictly speaking the vpci test doesn't require to be run on a Xen
>>> host currently, but that allows easier integration with logic that
>>> runs the rest of the tests.
>>
>> I accept that as a possible way of looking at things, but personally I
>> remain unconvinced of this model. To me what is installed should be of
>> value to users. If there was a properly separated directory where all
>> (and only) tests were put, I might agree with installing. (Nevertheless
>> this isn't an objection, merely a remark.)
>>
>>> While there also adjust the makefile to use $(RM), and rename the
>>> resulting binary to use a dash instead of an underscore (again to
>>> match the rest of the tests).
>>>
>>> Since the resulting test binary is now part of the distribution CC
>>> must be used instead of HOSTCC.
>>
>> This breaks the run: goal, doesn't it? If the new mode is wanted, I
>> think the two kinds of binaries (and rules) need separating (maybe a
>> way can be found to avoid duplicating the rules, which would seem
>> desirable).
> 
> The run rule is not hooked up in any of the upper level makefile logic,

What about the run-tests-% goal in the top level Makefile?

> so I think it's usage (like in other tests that also use CC and have
> such rule) is left to callers that know that HOSTCC == CC.
> 
>>> --- a/tools/tests/vpci/Makefile
>>> +++ b/tools/tests/vpci/Makefile
>>> @@ -1,7 +1,7 @@
>>>  XEN_ROOT=$(CURDIR)/../../..
>>>  include $(XEN_ROOT)/tools/Rules.mk
>>>  
>>> -TARGET := test_vpci
>>> +TARGET := test-vpci
>>>  
>>>  .PHONY: all
>>>  all: $(TARGET)
>>> @@ -11,17 +11,23 @@ run: $(TARGET)
>>>     ./$(TARGET)
>>>  
>>>  $(TARGET): vpci.c vpci.h list.h main.c emul.h
>>> -   $(HOSTCC) -g -o $@ vpci.c main.c
>>> +   $(CC) -o $@ vpci.c main.c
>>
>> You're losing -g and you're also not covering for it by adding $(CFLAGS)
>> (there should have been use of $(HOSTCFLAGS) already before, I suppose).
> 
> Wasn't sure whether I should add CFLAGS and LDFLAGS here, I guess
> LDFLAGS is really not needed because the test is not linked against
> any library, but could be added just in case.

Perhaps. I find $(LDFLAGS) odd to use anyway - by its name it ought to be
passed to $(LD), not $(CC).

Jan