So that when binaries such as qemu-img are searched for, those in the
build tree will be favored.  As a clarification, SRC_ROOT_DIR is
dependent on the location from where tests are executed, so they are
equal to the build directory if one is being used.

The original motivation is that Avocado libraries such as
avocado.utils.vmimage.get() may use the matching binaries, but it may
also apply to any other binary that test code may eventually attempt
to execute.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/acceptance/avocado_qemu/__init__.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 17ce583c87..a4bb796a47 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -110,6 +110,12 @@ class Test(avocado.Test):
         return None
 
     def setUp(self):
+        # Some utility code uses binaries from the system's PATH.  For
+        # instance, avocado.utils.vmimage.get() uses qemu-img, to
+        # create a snapshot image.  This is a transparent way of
+        # making sure those utilities find and use binaries on the
+        # build tree by default.
+        os.environ['PATH'] = '%s:%s' % (SRC_ROOT_DIR, os.environ['PATH'])
         self._vms = {}
 
         self.arch = self.params.get('arch',
-- 
2.21.0

On 11/4/19 1:13 PM, Cleber Rosa wrote:
> So that when binaries such as qemu-img are searched for, those in the
> build tree will be favored.  As a clarification, SRC_ROOT_DIR is
> dependent on the location from where tests are executed, so they are
> equal to the build directory if one is being used.
>
> The original motivation is that Avocado libraries such as
> avocado.utils.vmimage.get() may use the matching binaries, but it may
> also apply to any other binary that test code may eventually attempt
> to execute.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/acceptance/avocado_qemu/__init__.py | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index 17ce583c87..a4bb796a47 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -110,6 +110,12 @@ class Test(avocado.Test):
>           return None
>   
>       def setUp(self):
> +        # Some utility code uses binaries from the system's PATH.  For
> +        # instance, avocado.utils.vmimage.get() uses qemu-img, to
> +        # create a snapshot image.  This is a transparent way of

Because PATH is changed in a transparent way, wouldn't be better to also 
self.log.info() that fact?

> +        # making sure those utilities find and use binaries on the
> +        # build tree by default.
> +        os.environ['PATH'] = '%s:%s' % (SRC_ROOT_DIR, os.environ['PATH'])

I think PATH should be set only once at class initialization. Perhaps in 
setUpClass()?

- Wainer

>           self._vms = {}
>   
>           self.arch = self.params.get('arch',

On Thu, Nov 07, 2019 at 05:46:13PM -0200, Wainer dos Santos Moschetta wrote:
> 
> On 11/4/19 1:13 PM, Cleber Rosa wrote:
> > So that when binaries such as qemu-img are searched for, those in the
> > build tree will be favored.  As a clarification, SRC_ROOT_DIR is
> > dependent on the location from where tests are executed, so they are
> > equal to the build directory if one is being used.
> > 
> > The original motivation is that Avocado libraries such as
> > avocado.utils.vmimage.get() may use the matching binaries, but it may
> > also apply to any other binary that test code may eventually attempt
> > to execute.
> > 
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >   tests/acceptance/avocado_qemu/__init__.py | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> > index 17ce583c87..a4bb796a47 100644
> > --- a/tests/acceptance/avocado_qemu/__init__.py
> > +++ b/tests/acceptance/avocado_qemu/__init__.py
> > @@ -110,6 +110,12 @@ class Test(avocado.Test):
> >           return None
> >       def setUp(self):
> > +        # Some utility code uses binaries from the system's PATH.  For
> > +        # instance, avocado.utils.vmimage.get() uses qemu-img, to
> > +        # create a snapshot image.  This is a transparent way of
> 
> Because PATH is changed in a transparent way, wouldn't be better to also
> self.log.info() that fact?
>

I don't have a problem with logging it, but because it will happen for
*every single* test, it seems like it will become noise.  I think it's
better to properly document this aspect of "avocado_qemu.Test" instead
(which is currently missing here).  Something like:

"Tests based on avocado_qemu.Test will have, as a convenience, the 
QEMU build directory added to their PATH environment variable.  The goal
is to allow tests to seamless use matching built binaries, instead of
binaries installed elsewhere in the system".

How does it sound?

> > +        # making sure those utilities find and use binaries on the
> > +        # build tree by default.
> > +        os.environ['PATH'] = '%s:%s' % (SRC_ROOT_DIR, os.environ['PATH'])
> 
> I think PATH should be set only once at class initialization. Perhaps in
> setUpClass()?
> 
> - Wainer
>

The Avocado test isolation model makes setUpClass() unnecessary,
unsupported and pointless, so we only support setUp().

Every test already runs on its own process, and with the nrunner
model, should be able to run on completely different systems.  That's
why we don't want to support a setUpClass() like approach.

- Cleber.

On 11/11/19 8:49 PM, Cleber Rosa wrote:
> On Thu, Nov 07, 2019 at 05:46:13PM -0200, Wainer dos Santos Moschetta wrote:
>> On 11/4/19 1:13 PM, Cleber Rosa wrote:
>>> So that when binaries such as qemu-img are searched for, those in the
>>> build tree will be favored.  As a clarification, SRC_ROOT_DIR is
>>> dependent on the location from where tests are executed, so they are
>>> equal to the build directory if one is being used.
>>>
>>> The original motivation is that Avocado libraries such as
>>> avocado.utils.vmimage.get() may use the matching binaries, but it may
>>> also apply to any other binary that test code may eventually attempt
>>> to execute.
>>>
>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>> ---
>>>    tests/acceptance/avocado_qemu/__init__.py | 6 ++++++
>>>    1 file changed, 6 insertions(+)
>>>
>>> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
>>> index 17ce583c87..a4bb796a47 100644
>>> --- a/tests/acceptance/avocado_qemu/__init__.py
>>> +++ b/tests/acceptance/avocado_qemu/__init__.py
>>> @@ -110,6 +110,12 @@ class Test(avocado.Test):
>>>            return None
>>>        def setUp(self):
>>> +        # Some utility code uses binaries from the system's PATH.  For
>>> +        # instance, avocado.utils.vmimage.get() uses qemu-img, to
>>> +        # create a snapshot image.  This is a transparent way of
>> Because PATH is changed in a transparent way, wouldn't be better to also
>> self.log.info() that fact?
>>
> I don't have a problem with logging it, but because it will happen for
> *every single* test, it seems like it will become noise.  I think it's
> better to properly document this aspect of "avocado_qemu.Test" instead
> (which is currently missing here).  Something like:
>
> "Tests based on avocado_qemu.Test will have, as a convenience, the
> QEMU build directory added to their PATH environment variable.  The goal
> is to allow tests to seamless use matching built binaries, instead of
> binaries installed elsewhere in the system".
>
> How does it sound?


It does.


>
>>> +        # making sure those utilities find and use binaries on the
>>> +        # build tree by default.
>>> +        os.environ['PATH'] = '%s:%s' % (SRC_ROOT_DIR, os.environ['PATH'])
>> I think PATH should be set only once at class initialization. Perhaps in
>> setUpClass()?
>>
>> - Wainer
>>
> The Avocado test isolation model makes setUpClass() unnecessary,
> unsupported and pointless, so we only support setUp().
>
> Every test already runs on its own process, and with the nrunner
> model, should be able to run on completely different systems.  That's
> why we don't want to support a setUpClass() like approach.

Okay, thanks for the explanation.

Thanks,

Wainer

>
> - Cleber.
>
>

On Tue, Nov 12, 2019 at 12:00:20PM -0200, Wainer dos Santos Moschetta wrote:
> 
> On 11/11/19 8:49 PM, Cleber Rosa wrote:
> > On Thu, Nov 07, 2019 at 05:46:13PM -0200, Wainer dos Santos Moschetta wrote:
> > > On 11/4/19 1:13 PM, Cleber Rosa wrote:
> > > > So that when binaries such as qemu-img are searched for, those in the
> > > > build tree will be favored.  As a clarification, SRC_ROOT_DIR is
> > > > dependent on the location from where tests are executed, so they are
> > > > equal to the build directory if one is being used.
> > > > 
> > > > The original motivation is that Avocado libraries such as
> > > > avocado.utils.vmimage.get() may use the matching binaries, but it may
> > > > also apply to any other binary that test code may eventually attempt
> > > > to execute.
> > > > 
> > > > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > > > ---
> > > >    tests/acceptance/avocado_qemu/__init__.py | 6 ++++++
> > > >    1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> > > > index 17ce583c87..a4bb796a47 100644
> > > > --- a/tests/acceptance/avocado_qemu/__init__.py
> > > > +++ b/tests/acceptance/avocado_qemu/__init__.py
> > > > @@ -110,6 +110,12 @@ class Test(avocado.Test):
> > > >            return None
> > > >        def setUp(self):
> > > > +        # Some utility code uses binaries from the system's PATH.  For
> > > > +        # instance, avocado.utils.vmimage.get() uses qemu-img, to
> > > > +        # create a snapshot image.  This is a transparent way of
> > > Because PATH is changed in a transparent way, wouldn't be better to also
> > > self.log.info() that fact?
> > > 
> > I don't have a problem with logging it, but because it will happen for
> > *every single* test, it seems like it will become noise.  I think it's
> > better to properly document this aspect of "avocado_qemu.Test" instead
> > (which is currently missing here).  Something like:
> > 
> > "Tests based on avocado_qemu.Test will have, as a convenience, the
> > QEMU build directory added to their PATH environment variable.  The goal
> > is to allow tests to seamless use matching built binaries, instead of
> > binaries installed elsewhere in the system".
> > 
> > How does it sound?
> 
> 
> It does.
> 
> 
> > 
> > > > +        # making sure those utilities find and use binaries on the
> > > > +        # build tree by default.
> > > > +        os.environ['PATH'] = '%s:%s' % (SRC_ROOT_DIR, os.environ['PATH'])
> > > I think PATH should be set only once at class initialization. Perhaps in
> > > setUpClass()?
> > > 
> > > - Wainer
> > > 
> > The Avocado test isolation model makes setUpClass() unnecessary,
> > unsupported and pointless, so we only support setUp().
> > 
> > Every test already runs on its own process, and with the nrunner
> > model, should be able to run on completely different systems.  That's
> > why we don't want to support a setUpClass() like approach.
> 
> Okay, thanks for the explanation.
>

And thanks for the review.  Given the level of controversy here, I've
decided to take a different approach on v8.  Basically, I'm adding an
interface to avocado.utils.vmimage[1], so that we can explicitly
control the qemu-img binary used.

Looking forward to your opinion on v8.

Thanks,
- Cleber.

[1] - https://github.com/avocado-framework/avocado/pull/3374

> Thanks,
> 
> Wainer
> 
> > 
> > - Cleber.
> > 
> > 
> 
> 

On 11/4/19 4:13 PM, Cleber Rosa wrote:
> So that when binaries such as qemu-img are searched for, those in the
> build tree will be favored.  As a clarification, SRC_ROOT_DIR is
> dependent on the location from where tests are executed, so they are
> equal to the build directory if one is being used.
> 
> The original motivation is that Avocado libraries such as
> avocado.utils.vmimage.get() may use the matching binaries, but it may
> also apply to any other binary that test code may eventually attempt
> to execute.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/acceptance/avocado_qemu/__init__.py | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index 17ce583c87..a4bb796a47 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -110,6 +110,12 @@ class Test(avocado.Test):
>           return None
>   
>       def setUp(self):
> +        # Some utility code uses binaries from the system's PATH.  For
> +        # instance, avocado.utils.vmimage.get() uses qemu-img, to
> +        # create a snapshot image.  This is a transparent way of
> +        # making sure those utilities find and use binaries on the
> +        # build tree by default.
> +        os.environ['PATH'] = '%s:%s' % (SRC_ROOT_DIR, os.environ['PATH'])

But qemu-img is compiled in BUILD_ROOT_DIR, isn't it?

Maybe we should pass its path by argument, such --qemu-img /path/to/it.

>           self._vms = {}
>   
>           self.arch = self.params.get('arch',
> 

On Fri, Nov 08, 2019 at 02:13:02PM +0100, Philippe Mathieu-Daudé wrote:
> On 11/4/19 4:13 PM, Cleber Rosa wrote:
> > So that when binaries such as qemu-img are searched for, those in the
> > build tree will be favored.  As a clarification, SRC_ROOT_DIR is
> > dependent on the location from where tests are executed, so they are
> > equal to the build directory if one is being used.
> > 
> > The original motivation is that Avocado libraries such as
> > avocado.utils.vmimage.get() may use the matching binaries, but it may
> > also apply to any other binary that test code may eventually attempt
> > to execute.
> > 
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >   tests/acceptance/avocado_qemu/__init__.py | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> > index 17ce583c87..a4bb796a47 100644
> > --- a/tests/acceptance/avocado_qemu/__init__.py
> > +++ b/tests/acceptance/avocado_qemu/__init__.py
> > @@ -110,6 +110,12 @@ class Test(avocado.Test):
> >           return None
> >       def setUp(self):
> > +        # Some utility code uses binaries from the system's PATH.  For
> > +        # instance, avocado.utils.vmimage.get() uses qemu-img, to
> > +        # create a snapshot image.  This is a transparent way of
> > +        # making sure those utilities find and use binaries on the
> > +        # build tree by default.
> > +        os.environ['PATH'] = '%s:%s' % (SRC_ROOT_DIR, os.environ['PATH'])
> 
> But qemu-img is compiled in BUILD_ROOT_DIR, isn't it?
> 
> Maybe we should pass its path by argument, such --qemu-img /path/to/it.
>

Hi Philippe,

On the next version we should see a properly named variable for the
build directory, and (as explained in the previous response) also
a more explicit setting of the qemu-img binary used (although not
a parameter or command line argument at this point).

Looking forward for your opinion on the next version, and thanks
again!

- Cleber.

> >           self._vms = {}
> >           self.arch = self.params.get('arch',
> >