[PATCH] tests/vpci: install test

Roger Pau Monne posted 1 patch 1 year ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20230309165812.33918-1-roger.pau@citrix.com
There is a newer version of this series
tools/tests/vpci/Makefile | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
[PATCH] tests/vpci: install test
Posted by Roger Pau Monne 1 year ago
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.

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.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
XenRT has recently gained the ability to run the tests in tools/tests
that are installed, so the install target is needed for that use-case.
---
 tools/tests/vpci/Makefile | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tools/tests/vpci/Makefile b/tools/tests/vpci/Makefile
index 5075bc2be2..3b85b689d3 100644
--- 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
 
 .PHONY: clean
 clean:
-	rm -rf $(TARGET) *.o *~ vpci.h vpci.c list.h
+	$(RM) -- $(TARGET) *.o *~ vpci.h vpci.c list.h
 
 .PHONY: distclean
 distclean: clean
 
 .PHONY: install
-install:
+install: all
+	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
+	$(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC_BIN)
+
+.PHONY: uninstall
+uninstall:
+	$(RM) -- $(DESTDIR)$(LIBEXEC_BIN)/$(TARGET)
 
 vpci.c: $(XEN_ROOT)/xen/drivers/vpci/vpci.c
 	# Remove includes and add the test harness header
-- 
2.39.0


Re: [PATCH] tests/vpci: install test
Posted by Jan Beulich 1 year ago
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).

> --- 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).

Jan
Re: [PATCH] tests/vpci: install test
Posted by Roger Pau Monné 1 year ago
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,
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.

Thanks, Roger.
Re: [PATCH] tests/vpci: install test
Posted by Jan Beulich 1 year ago
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

Re: [PATCH] tests/vpci: install test
Posted by Roger Pau Monné 1 year ago
On Fri, Mar 10, 2023 at 03:32:41PM +0100, Jan Beulich wrote:
> 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?

Urg, I wasn't aware of that target.  I assume just removing the `run`
target from the vpci makefile would be an acceptable solution then.

It's still the user that needs to explicitly call run-tests-vpci, so
it would better know that HOSTCC == CC before attempting that.

Note cpu-policy and resource should also be fixed, but I don't plan
to do it as part of this patch.

Roger.

Re: [PATCH] tests/vpci: install test
Posted by Jan Beulich 1 year ago
On 13.03.2023 11:31, Roger Pau Monné wrote:
> On Fri, Mar 10, 2023 at 03:32:41PM +0100, Jan Beulich wrote:
>> 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?
> 
> Urg, I wasn't aware of that target.  I assume just removing the `run`
> target from the vpci makefile would be an acceptable solution then.

I'm afraid I wouldn't view this as acceptable. I would very much like
to retain these run: goals, as I view it as important that such tests
be possible to run easily and right from the build area. What might be
acceptable to me is if ...

> It's still the user that needs to explicitly call run-tests-vpci, so
> it would better know that HOSTCC == CC before attempting that.

... the run: rune would be enclosed in "ifeq ($(CC),$(HOSTCC))". Yet
even that is fragile. For tests like this I view it as secondary to
be runnable on the destination architecture, and hence I continue to
think that if installing such tests is really wanted, binaries for
host and target should be properly separated.

Jan

Re: [PATCH] tests/vpci: install test
Posted by Roger Pau Monné 1 year ago
On Mon, Mar 13, 2023 at 11:43:43AM +0100, Jan Beulich wrote:
> On 13.03.2023 11:31, Roger Pau Monné wrote:
> > On Fri, Mar 10, 2023 at 03:32:41PM +0100, Jan Beulich wrote:
> >> 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?
> > 
> > Urg, I wasn't aware of that target.  I assume just removing the `run`
> > target from the vpci makefile would be an acceptable solution then.
> 
> I'm afraid I wouldn't view this as acceptable. I would very much like
> to retain these run: goals, as I view it as important that such tests
> be possible to run easily and right from the build area. What might be
> acceptable to me is if ...
> 
> > It's still the user that needs to explicitly call run-tests-vpci, so
> > it would better know that HOSTCC == CC before attempting that.
> 
> ... the run: rune would be enclosed in "ifeq ($(CC),$(HOSTCC))". Yet
> even that is fragile. For tests like this I view it as secondary to
> be runnable on the destination architecture, and hence I continue to
> think that if installing such tests is really wanted, binaries for
> host and target should be properly separated.

vpci test is special in this regard when compared to the rest of the
tests that do make use of the hypercall interface to a degree, and
hence are not expected to be run from the build host as they require
to be run from a Xen domain.

I think the benefit of having the test run part of XenRT is greater
than the downfall of installing a test as part of the distribution.

I've added a guard to the `run` target in order to check that HOSTCC
== CC, hope that is enough.

Thanks, Roger.

Re: [PATCH] tests/vpci: install test
Posted by Jan Beulich 1 year ago
On 13.03.2023 12:15, Roger Pau Monné wrote:
> On Mon, Mar 13, 2023 at 11:43:43AM +0100, Jan Beulich wrote:
>> On 13.03.2023 11:31, Roger Pau Monné wrote:
>>> On Fri, Mar 10, 2023 at 03:32:41PM +0100, Jan Beulich wrote:
>>>> 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?
>>>
>>> Urg, I wasn't aware of that target.  I assume just removing the `run`
>>> target from the vpci makefile would be an acceptable solution then.
>>
>> I'm afraid I wouldn't view this as acceptable. I would very much like
>> to retain these run: goals, as I view it as important that such tests
>> be possible to run easily and right from the build area. What might be
>> acceptable to me is if ...
>>
>>> It's still the user that needs to explicitly call run-tests-vpci, so
>>> it would better know that HOSTCC == CC before attempting that.
>>
>> ... the run: rune would be enclosed in "ifeq ($(CC),$(HOSTCC))". Yet
>> even that is fragile. For tests like this I view it as secondary to
>> be runnable on the destination architecture, and hence I continue to
>> think that if installing such tests is really wanted, binaries for
>> host and target should be properly separated.
> 
> vpci test is special in this regard when compared to the rest of the
> tests that do make use of the hypercall interface to a degree, and
> hence are not expected to be run from the build host as they require
> to be run from a Xen domain.

Right, which is why I said "for tests like this" (which includes in
particular also the x86 emulator harness).

> I think the benefit of having the test run part of XenRT is greater
> than the downfall of installing a test as part of the distribution.
> 
> I've added a guard to the `run` target in order to check that HOSTCC
> == CC, hope that is enough.

With that at least I won't further object to the change.

Jan