Re: [Qemu-devel] [PATCH v4 1/3] Bootstrap Python venv for tests

[Caio Carrara]

On 12-10-2018 13:53, Cleber Rosa wrote:
> A number of QEMU tests are written in Python, and may benefit
> from an untainted Python venv.
> 
> By using make rules, tests that depend on specific Python libs
> can set that rule as a requirement, along with rules that require
> the presence or installation of specific libraries.
> 
> The tests/venv-requirements.txt is supposed to contain the
> Python requirements that should be added to the venv created
> by check-venv.
> 
> Signed-off-by: Cleber Rosa <address@hidden>
> ---
>  tests/Makefile.include      | 20 ++++++++++++++++++++
>  tests/venv-requirements.txt |  3 +++

Any special reason to name `venv-requirements.txt` instead of only
`requirements.txt`? I think the second way is better (if there's no
cons) since it's the default from most of Python projects. Besides that
seems more semantic to have a `tests/requirements.txt` file that
registers the requirements for tests.

Still in this topic. As far as I could see, the files in `tests/`
directory are almost all C source code. The right place for the Python
requirements seems the `tests/acceptance/` directory specially because
the subject of all this patches. Keeping only one requirements file for
all kinds of tests (that uses Python) can be problematic in my opinion.
If it makes any sense, probably is also a good idea rename the target
from `check-venv` to something like `check-acceptance-venv`.

>  2 files changed, 23 insertions(+)
>  create mode 100644 tests/venv-requirements.txt
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 5eadfd52f9..b66180efa1 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -12,6 +12,7 @@ check-help:
>       @echo " $(MAKE) check-block          Run block tests"
>       @echo " $(MAKE) check-tcg            Run TCG tests"
>       @echo " $(MAKE) check-report.html    Generates an HTML test report"
> +     @echo " $(MAKE) check-venv           Creates a Python venv for tests"
>       @echo " $(MAKE) check-clean          Clean the tests"
>       @echo
>       @echo "Please note that HTML reports do not regenerate if the unit 
> tests"
> @@ -1017,6 +1018,24 @@ check-decodetree:
>            ./check.sh "$(PYTHON)" "$(SRC_PATH)/scripts/decodetree.py", \
>            TEST, decodetree.py)
>  
> +# Python venv for running tests
> +
> +.PHONY: check-venv
> +
> +TESTS_VENV_DIR=$(BUILD_DIR)/tests/venv
> +TESTS_VENV_REQ=$(SRC_PATH)/tests/venv-requirements.txt
> +
> +$(TESTS_VENV_DIR): $(TESTS_VENV_REQ)
> +     $(call quiet-command, \
> +            $(PYTHON) -m venv --system-site-packages $@, \
> +            VENV, $@)
> +     $(call quiet-command, \
> +            $(TESTS_VENV_DIR)/bin/python -m pip -q install -r 
> $(TESTS_VENV_REQ), \
> +            PIP, $(TESTS_VENV_REQ))
> +     $(call quiet-command, touch $@)
> +
> +check-venv: $(TESTS_VENV_DIR)
> +
>  # Consolidated targets
>  
>  .PHONY: check-qapi-schema check-qtest check-unit check check-clean
> @@ -1030,6 +1049,7 @@ check-clean:
>       rm -rf $(check-unit-y) tests/*.o $(QEMU_IOTESTS_HELPERS-y)
>       rm -rf $(sort $(foreach target,$(SYSEMU_TARGET_LIST), 
> $(check-qtest-$(target)-y)) $(check-qtest-generic-y))
>       rm -f tests/test-qapi-gen-timestamp
> +     rm -rf $(TESTS_VENV_DIR)
>  
>  clean: check-clean
>  
> diff --git a/tests/venv-requirements.txt b/tests/venv-requirements.txt
> new file mode 100644
> index 0000000000..d39f9d1576
> --- /dev/null
> +++ b/tests/venv-requirements.txt
> @@ -0,0 +1,3 @@
> +# Add Python module requirements, one per line, to be installed
> +# in the tests/venv Python virtual environment. For more info,
> +# refer to: https://pip.pypa.io/en/stable/user_guide/#id1
> 


-- 
Caio Carrara
Software Engineer, Virt Team
Red Hat
address@hidden