[v7,5/8] Acceptance tests: keep a stable reference to the QEMU build dir
diff mbox series

Message ID 20191104151323.9883-6-crosa@redhat.com
State New
Headers show
Series
  • Acceptance test: Add "boot_linux" acceptance test
Related show

Commit Message

Cleber Rosa Nov. 4, 2019, 3:13 p.m. UTC
This is related to the the differences in in-tree and out-of-tree
builds in QEMU.  For simplification, <BLD> means my build directory.

Currently, by running a `make check-acceptance` one gets (in
tests/acceptance/avocado_qemu/__init__.py):

   SRC_ROOT_DIR: <BLD>/tests/acceptance/avocado_qemu/../../..

This in itself is problematic, because after the parent directories
are applied, one may be left not with a pointer to the build directory
as intended, but with the location of the source tree (assuming they
differ). Built binaries, such as qemu-img, are of course not there and
can't be found.

Given that a Python '__file__' will contain the absolute path to the
file backing the module, say:

   __file__: <BLD>/tests/acceptance/avocado_qemu/__init__.py

                  |  4  |     3    |      2     |     1     |

A solution is to not "evaluate" the third parent dir (marked as 4
here) because that ends up following the "tests" directory symlink to
the source tree.  In fact, there's no need to keep or evaluate any of
the parent directories, we can just drop the rightmost 4 components,
and we'll keep a stable reference to the build directory (with no
symlink being followed).  This works for either a dedicated build
directory or also a combined source and build tree.

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

Comments

Wainer dos Santos Moschetta Nov. 7, 2019, 7:22 p.m. UTC | #1
On 11/4/19 1:13 PM, Cleber Rosa wrote:
> This is related to the the differences in in-tree and out-of-tree
> builds in QEMU.  For simplification, <BLD> means my build directory.
>
> Currently, by running a `make check-acceptance` one gets (in
> tests/acceptance/avocado_qemu/__init__.py):
>
>     SRC_ROOT_DIR: <BLD>/tests/acceptance/avocado_qemu/../../..
>
> This in itself is problematic, because after the parent directories
> are applied, one may be left not with a pointer to the build directory
> as intended, but with the location of the source tree (assuming they
> differ). Built binaries, such as qemu-img, are of course not there and
> can't be found.
>
> Given that a Python '__file__' will contain the absolute path to the
> file backing the module, say:
>
>     __file__: <BLD>/tests/acceptance/avocado_qemu/__init__.py
>
>                    |  4  |     3    |      2     |     1     |
>
> A solution is to not "evaluate" the third parent dir (marked as 4
> here) because that ends up following the "tests" directory symlink to
> the source tree.  In fact, there's no need to keep or evaluate any of
> the parent directories, we can just drop the rightmost 4 components,
> and we'll keep a stable reference to the build directory (with no
> symlink being followed).  This works for either a dedicated build
> directory or also a combined source and build tree.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/acceptance/avocado_qemu/__init__.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index 6618ea67c1..17ce583c87 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -16,7 +16,7 @@ import tempfile
>   
>   import avocado
>   
> -SRC_ROOT_DIR = os.path.join(os.path.dirname(__file__), '..', '..', '..')
> +SRC_ROOT_DIR = os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(__file__))))

In this case, wouldn't make sense to rename the constant from 
SRC_ROOT_DIR to BUILD_ROOT_DIR?

This patch looks good to me besides that.

- Wainer

>   sys.path.append(os.path.join(SRC_ROOT_DIR, 'python'))
>   
>   from qemu.machine import QEMUMachine
Cleber Rosa Nov. 11, 2019, 10:38 p.m. UTC | #2
On Thu, Nov 07, 2019 at 05:22:24PM -0200, Wainer dos Santos Moschetta wrote:
> 
> On 11/4/19 1:13 PM, Cleber Rosa wrote:
> > This is related to the the differences in in-tree and out-of-tree
> > builds in QEMU.  For simplification, <BLD> means my build directory.
> > 
> > Currently, by running a `make check-acceptance` one gets (in
> > tests/acceptance/avocado_qemu/__init__.py):
> > 
> >     SRC_ROOT_DIR: <BLD>/tests/acceptance/avocado_qemu/../../..
> > 
> > This in itself is problematic, because after the parent directories
> > are applied, one may be left not with a pointer to the build directory
> > as intended, but with the location of the source tree (assuming they
> > differ). Built binaries, such as qemu-img, are of course not there and
> > can't be found.
> > 
> > Given that a Python '__file__' will contain the absolute path to the
> > file backing the module, say:
> > 
> >     __file__: <BLD>/tests/acceptance/avocado_qemu/__init__.py
> > 
> >                    |  4  |     3    |      2     |     1     |
> > 
> > A solution is to not "evaluate" the third parent dir (marked as 4
> > here) because that ends up following the "tests" directory symlink to
> > the source tree.  In fact, there's no need to keep or evaluate any of
> > the parent directories, we can just drop the rightmost 4 components,
> > and we'll keep a stable reference to the build directory (with no
> > symlink being followed).  This works for either a dedicated build
> > directory or also a combined source and build tree.
> > 
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >   tests/acceptance/avocado_qemu/__init__.py | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> > index 6618ea67c1..17ce583c87 100644
> > --- a/tests/acceptance/avocado_qemu/__init__.py
> > +++ b/tests/acceptance/avocado_qemu/__init__.py
> > @@ -16,7 +16,7 @@ import tempfile
> >   import avocado
> > -SRC_ROOT_DIR = os.path.join(os.path.dirname(__file__), '..', '..', '..')
> > +SRC_ROOT_DIR = os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(__file__))))
> 
> In this case, wouldn't make sense to rename the constant from SRC_ROOT_DIR
> to BUILD_ROOT_DIR?
>

True.  I remember thinking about doing that as a separate change and
ended up forgetting.  Maybe it's better to just do it here.

> This patch looks good to me besides that.
> 
> - Wainer
>

Thanks!
- Cleber.
Cleber Rosa Nov. 15, 2019, 9:36 p.m. UTC | #3
On Thu, Nov 07, 2019 at 05:22:24PM -0200, Wainer dos Santos Moschetta wrote:
> 
> On 11/4/19 1:13 PM, Cleber Rosa wrote:
> > This is related to the the differences in in-tree and out-of-tree
> > builds in QEMU.  For simplification, <BLD> means my build directory.
> > 
> > Currently, by running a `make check-acceptance` one gets (in
> > tests/acceptance/avocado_qemu/__init__.py):
> > 
> >     SRC_ROOT_DIR: <BLD>/tests/acceptance/avocado_qemu/../../..
> > 
> > This in itself is problematic, because after the parent directories
> > are applied, one may be left not with a pointer to the build directory
> > as intended, but with the location of the source tree (assuming they
> > differ). Built binaries, such as qemu-img, are of course not there and
> > can't be found.
> > 
> > Given that a Python '__file__' will contain the absolute path to the
> > file backing the module, say:
> > 
> >     __file__: <BLD>/tests/acceptance/avocado_qemu/__init__.py
> > 
> >                    |  4  |     3    |      2     |     1     |
> > 
> > A solution is to not "evaluate" the third parent dir (marked as 4
> > here) because that ends up following the "tests" directory symlink to
> > the source tree.  In fact, there's no need to keep or evaluate any of
> > the parent directories, we can just drop the rightmost 4 components,
> > and we'll keep a stable reference to the build directory (with no
> > symlink being followed).  This works for either a dedicated build
> > directory or also a combined source and build tree.
> > 
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >   tests/acceptance/avocado_qemu/__init__.py | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> > index 6618ea67c1..17ce583c87 100644
> > --- a/tests/acceptance/avocado_qemu/__init__.py
> > +++ b/tests/acceptance/avocado_qemu/__init__.py
> > @@ -16,7 +16,7 @@ import tempfile
> >   import avocado
> > -SRC_ROOT_DIR = os.path.join(os.path.dirname(__file__), '..', '..', '..')
> > +SRC_ROOT_DIR = os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(__file__))))
> 
> In this case, wouldn't make sense to rename the constant from SRC_ROOT_DIR
> to BUILD_ROOT_DIR?
> 
> This patch looks good to me besides that.
> 
> - Wainer
>

So, based on your review, I went back and forward with this.  I'm
going to propose something different on v8, basically distinguishing
between source and build directories.  I'm keeping this patch as is on
v8 though, just for the sake of making the indivdual changes easier to
track.

Thanks,
- Cleber.

> >   sys.path.append(os.path.join(SRC_ROOT_DIR, 'python'))
> >   from qemu.machine import QEMUMachine

Patch
diff mbox series

diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 6618ea67c1..17ce583c87 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -16,7 +16,7 @@  import tempfile
 
 import avocado
 
-SRC_ROOT_DIR = os.path.join(os.path.dirname(__file__), '..', '..', '..')
+SRC_ROOT_DIR = os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(__file__))))
 sys.path.append(os.path.join(SRC_ROOT_DIR, 'python'))
 
 from qemu.machine import QEMUMachine