Re: [Qemu-devel] [PATCH 2/2] Acceptance tests: add make rule for running them

[Cleber Rosa]

On 9/21/18 12:59 PM, Eduardo Habkost wrote:
> On Fri, Sep 21, 2018 at 03:34:02PM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <address@hidden> writes:
>>
>>> On Fri, Sep 21, 2018 at 08:36:22AM +0200, Markus Armbruster wrote:
>>>> Eduardo Habkost <address@hidden> writes:
>>>>
>>>>> On Thu, Sep 20, 2018 at 11:19:56AM -0400, Cleber Rosa wrote:
> [...]
>>>>>> +$(TESTS_RESULTS_DIR):
>>>>>> +        $(call quiet-command, mkdir -p $@, \
>>>>>> +            MKDIR, $@)
>>>>>> +
>>>>>>  check-venv: $(TESTS_VENV_DIR)
>>>>>>  
>>>>>> +check-acceptance: check-venv $(TESTS_RESULTS_DIR)
>>>>>> +        $(call quiet-command, \
>>>>>> +            $(TESTS_VENV_DIR)/bin/avocado \
>>>>>> +            --show=none run --job-results-dir=$(TESTS_RESULTS_DIR) 
>>>>>> --failfast=on \
>>>>>> +            $(SRC_PATH)/tests/acceptance, \
>>>>>> +            "AVOCADO", "tests/acceptance")
>>>>>
>>>>> I think we should provide something easy to use for people who
>>>>> already have the right Avocado version installed in their system
>>>>> and want to avoid re-downloading Avocado every time.
>>>>>
>>>>> We already have plans to do this automatically/transparently in
>>>>> the future, but maybe while we don't have something automatic we
>>>>> could have two separate rules.  e.g.:
>>>>>
>>>>>   AVOCADO = avocado
>>>>>
>>>>>   check-acceptance: $(TESTS_RESULTS_DIR)
>>>>>           $(call quiet-command, \
>>>>>               $(AVOCADO) \
>>>>>               --show=none run --job-results-dir=$(TESTS_RESULTS_DIR) 
>>>>> --failfast=on \
>>>>>               $(SRC_PATH)/tests/acceptance, \
>>>>>               "AVOCADO", "tests/acceptance")
>>>>>
>>>>>   check-acceptance-venv: check-venv
>>>>>           $(MAKE) check-acceptance AVOCADO=$(TESTS_VENV_DIR)/bin/avocado
>>>>
>>>> Recursion just to override a variable feels like overkill.
>>>> Have you considered target-specific variable values?
>>>>
>>>> https://www.gnu.org/software/make/manual/html_node/Target_002dspecific.html#Target_002dspecific
>>>
>>> I have, but I wasn't sure what would be the pitfalls when doing
>>> that.
>>>
>>> For example, the following seems to work but makes the build
>>> results depend on the ordering of targets:
>>>
>>>   $ cat Makefile
>>>   VAR=x
>>>   a:
>>>     echo $(VAR)
>>>   b: VAR=b
>>>   b: a
>>>   $ make a
>>>   echo x
>>>   x
>>>   $ make b
>>>   echo b
>>>   b
>>>   $ make a b
>>>   echo x
>>>   x
>>>   make: Nothing to be done for 'b'.
>>>   $ make b a
>>>   echo b
>>>   b
>>>   make: 'a' is up to date.
>>>   $
>>
>> I doubt that's a problem for practical applications.  Why would anyone
>> ask for two conflicting check-acceptance runs, and expect to get *both*?
> 
> I think you're right.  I'm just not confident in my understanding
> of make to be sure there are no other unexpected pitfalls.  If
> you don't see any problem, we can do that.
> 
> Anyway, we probably won't need two separate rules in this case if
> we use the '--system-site-packages' option of venv.
> 

I think we can get away with a single target here indeed.

- Cleber.