diff mbox series

[v1,1/2] kunit: tool: fix running kunit_tool from outside kernel tree

Message ID 20200808011651.2113930-1-brendanhiggins@google.com (mailing list archive)
State Accepted
Headers show
Series [v1,1/2] kunit: tool: fix running kunit_tool from outside kernel tree | expand

Commit Message

Brendan Higgins Aug. 8, 2020, 1:16 a.m. UTC 
Currently kunit_tool does not work correctly when executed from a path
outside of the kernel tree, so make sure that the current working
directory is correct and the kunit_dir is properly initialized before
running.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
 tools/testing/kunit/kunit.py | 8 ++++++++
 1 file changed, 8 insertions(+)


base-commit: 30185b69a2d533c4ba6ca926b8390ce7de495e29

Comments

David Gow Aug. 8, 2020, 5:45 a.m. UTC | #1 
On Sat, Aug 8, 2020 at 9:17 AM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> Currently kunit_tool does not work correctly when executed from a path
> outside of the kernel tree, so make sure that the current working
> directory is correct and the kunit_dir is properly initialized before
> running.
>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
>  tools/testing/kunit/kunit.py | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> index 425ef40067e7..96344a11ff1f 100755
> --- a/tools/testing/kunit/kunit.py
> +++ b/tools/testing/kunit/kunit.py
> @@ -237,9 +237,14 @@ def main(argv, linux=None):
>
>         cli_args = parser.parse_args(argv)
>
> +       if get_kernel_root_path():
> +               print('cd ' + get_kernel_root_path())
Do we want to print this, or is it a leftover debug statement?


> +               os.chdir(get_kernel_root_path())
> +
>         if cli_args.subcommand == 'run':
>                 if not os.path.exists(cli_args.build_dir):
>                         os.mkdir(cli_args.build_dir)
> +                       create_default_kunitconfig()
Why are we adding this everywhere when it's already in config_tests,
which should already be called in all of the places where a
kunitconfig is required?
Is the goal to always copy the default kunitconfig when creating a new
build_dir? While I can sort-of see why we might want to do that, if
the build dir doesn't exist, most of the subcommands will fail anyway
(maybe we should only create the build-dir for 'config' and 'run'?)


>
>                 if not linux:
>                         linux = kunit_kernel.LinuxSourceTree()
> @@ -257,6 +262,7 @@ def main(argv, linux=None):
>                 if cli_args.build_dir:
>                         if not os.path.exists(cli_args.build_dir):
>                                 os.mkdir(cli_args.build_dir)
> +                               create_default_kunitconfig()
>
>                 if not linux:
>                         linux = kunit_kernel.LinuxSourceTree()
> @@ -273,6 +279,7 @@ def main(argv, linux=None):
>                 if cli_args.build_dir:
>                         if not os.path.exists(cli_args.build_dir):
>                                 os.mkdir(cli_args.build_dir)
> +                               create_default_kunitconfig()
>
>                 if not linux:
>                         linux = kunit_kernel.LinuxSourceTree()
> @@ -291,6 +298,7 @@ def main(argv, linux=None):
>                 if cli_args.build_dir:
>                         if not os.path.exists(cli_args.build_dir):
>                                 os.mkdir(cli_args.build_dir)
> +                               create_default_kunitconfig()
>
>                 if not linux:
>                         linux = kunit_kernel.LinuxSourceTree()
>
> base-commit: 30185b69a2d533c4ba6ca926b8390ce7de495e29
> --
> 2.28.0.236.gb10cc79966-goog
>
Brendan Higgins Aug. 8, 2020, 8:50 a.m. UTC | #2 
On Fri, Aug 7, 2020 at 10:45 PM David Gow <davidgow@google.com> wrote:
>
> On Sat, Aug 8, 2020 at 9:17 AM Brendan Higgins
> <brendanhiggins@google.com> wrote:
> >
> > Currently kunit_tool does not work correctly when executed from a path
> > outside of the kernel tree, so make sure that the current working
> > directory is correct and the kunit_dir is properly initialized before
> > running.
> >
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > ---
> >  tools/testing/kunit/kunit.py | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> > index 425ef40067e7..96344a11ff1f 100755
> > --- a/tools/testing/kunit/kunit.py
> > +++ b/tools/testing/kunit/kunit.py
> > @@ -237,9 +237,14 @@ def main(argv, linux=None):
> >
> >         cli_args = parser.parse_args(argv)
> >
> > +       if get_kernel_root_path():
> > +               print('cd ' + get_kernel_root_path())
> Do we want to print this, or is it a leftover debug statement?

Whoops, I was supposed to delete that. That's embarrassing... ^_^;

> > +               os.chdir(get_kernel_root_path())
> > +
> >         if cli_args.subcommand == 'run':
> >                 if not os.path.exists(cli_args.build_dir):
> >                         os.mkdir(cli_args.build_dir)
> > +                       create_default_kunitconfig()
> Why are we adding this everywhere when it's already in config_tests,
> which should already be called in all of the places where a
> kunitconfig is required?

Ah yes, .kunitconfig needs to be created before config_tests() can be
called because the LinuxSourceTree constructor needs .kunitconfig to
exist.

> Is the goal to always copy the default kunitconfig when creating a new
> build_dir? While I can sort-of see why we might want to do that, if
> the build dir doesn't exist, most of the subcommands will fail anyway
> (maybe we should only create the build-dir for 'config' and 'run'?)

I just did it because we were getting a failure in a constructor so we
couldn't do much. Ideally we would check that the current state allows
for the command that the user intended to run, but I think that's
beyond the scope of this change.

So I guess the real question is: Is it okay for it to crash in the
constructor with a cryptic error message for now, or do we want to let
it fail with a slightly less cryptic message later?

> >                 if not linux:
> >                         linux = kunit_kernel.LinuxSourceTree()
> > @@ -257,6 +262,7 @@ def main(argv, linux=None):
> >                 if cli_args.build_dir:
> >                         if not os.path.exists(cli_args.build_dir):
> >                                 os.mkdir(cli_args.build_dir)
> > +                               create_default_kunitconfig()
> >
> >                 if not linux:
> >                         linux = kunit_kernel.LinuxSourceTree()
> > @@ -273,6 +279,7 @@ def main(argv, linux=None):
> >                 if cli_args.build_dir:
> >                         if not os.path.exists(cli_args.build_dir):
> >                                 os.mkdir(cli_args.build_dir)
> > +                               create_default_kunitconfig()
> >
> >                 if not linux:
> >                         linux = kunit_kernel.LinuxSourceTree()
> > @@ -291,6 +298,7 @@ def main(argv, linux=None):
> >                 if cli_args.build_dir:
> >                         if not os.path.exists(cli_args.build_dir):
> >                                 os.mkdir(cli_args.build_dir)
> > +                               create_default_kunitconfig()
> >
> >                 if not linux:
> >                         linux = kunit_kernel.LinuxSourceTree()
> >
> > base-commit: 30185b69a2d533c4ba6ca926b8390ce7de495e29
> > --
> > 2.28.0.236.gb10cc79966-goog
> >
David Gow Aug. 11, 2020, 4:29 a.m. UTC | #3 
On Sat, Aug 8, 2020 at 4:51 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Fri, Aug 7, 2020 at 10:45 PM David Gow <davidgow@google.com> wrote:
> >
> > On Sat, Aug 8, 2020 at 9:17 AM Brendan Higgins
> > <brendanhiggins@google.com> wrote:
> > >
> > > Currently kunit_tool does not work correctly when executed from a path
> > > outside of the kernel tree, so make sure that the current working
> > > directory is correct and the kunit_dir is properly initialized before
> > > running.
> > >
> > > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > > ---
> > >  tools/testing/kunit/kunit.py | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> > > index 425ef40067e7..96344a11ff1f 100755
> > > --- a/tools/testing/kunit/kunit.py
> > > +++ b/tools/testing/kunit/kunit.py
> > > @@ -237,9 +237,14 @@ def main(argv, linux=None):
> > >
> > >         cli_args = parser.parse_args(argv)
> > >
> > > +       if get_kernel_root_path():
> > > +               print('cd ' + get_kernel_root_path())
> > Do we want to print this, or is it a leftover debug statement?
>
> Whoops, I was supposed to delete that. That's embarrassing... ^_^;
>
> > > +               os.chdir(get_kernel_root_path())
> > > +
> > >         if cli_args.subcommand == 'run':
> > >                 if not os.path.exists(cli_args.build_dir):
> > >                         os.mkdir(cli_args.build_dir)
> > > +                       create_default_kunitconfig()
> > Why are we adding this everywhere when it's already in config_tests,
> > which should already be called in all of the places where a
> > kunitconfig is required?
>
> Ah yes, .kunitconfig needs to be created before config_tests() can be
> called because the LinuxSourceTree constructor needs .kunitconfig to
> exist.

I see. I guess the ultimate solution will be for LinuxSourceTree not
require a .kunitconfig unless it's actually being used.

> > Is the goal to always copy the default kunitconfig when creating a new
> > build_dir? While I can sort-of see why we might want to do that, if
> > the build dir doesn't exist, most of the subcommands will fail anyway
> > (maybe we should only create the build-dir for 'config' and 'run'?)
>
> I just did it because we were getting a failure in a constructor so we
> couldn't do much. Ideally we would check that the current state allows
> for the command that the user intended to run, but I think that's
> beyond the scope of this change.
>
> So I guess the real question is: Is it okay for it to crash in the
> constructor with a cryptic error message for now, or do we want to let
> it fail with a slightly less cryptic message later?
>

I personally am leaning towards allowing it to crash in the build,
exec, etc. subcommands for now, and tidying up the error messages
later, rather than silently creating a blank build dir, only for it
then to fail later.

In the meantime, yeah, we can add this for the config and run tasks,
and maybe remove the whole "if cli_args.build_dir" / mkdir branch from
the other subcommands.

If we weren't going to fix the LinuxSourceTree constructor, it'd make
sense to get rid of the redundant code to create it in config_tests(),
too, but I'm not sure it's worthwhile.

In any case, now I know what's happening, I'm okay with anything
moderately sensible which gets the 'config' and 'run' subcommands
working on an empty build dir, and the code and error messages can be
fixed when tidying up the LinuxSourceTree() constructor in a separate
patch.

Cheers,
-- David

> > >                 if not linux:
> > >                         linux = kunit_kernel.LinuxSourceTree()
> > > @@ -257,6 +262,7 @@ def main(argv, linux=None):
> > >                 if cli_args.build_dir:
> > >                         if not os.path.exists(cli_args.build_dir):
> > >                                 os.mkdir(cli_args.build_dir)
> > > +                               create_default_kunitconfig()
> > >
> > >                 if not linux:
> > >                         linux = kunit_kernel.LinuxSourceTree()
> > > @@ -273,6 +279,7 @@ def main(argv, linux=None):
> > >                 if cli_args.build_dir:
> > >                         if not os.path.exists(cli_args.build_dir):
> > >                                 os.mkdir(cli_args.build_dir)
> > > +                               create_default_kunitconfig()
> > >
> > >                 if not linux:
> > >                         linux = kunit_kernel.LinuxSourceTree()
> > > @@ -291,6 +298,7 @@ def main(argv, linux=None):
> > >                 if cli_args.build_dir:
> > >                         if not os.path.exists(cli_args.build_dir):
> > >                                 os.mkdir(cli_args.build_dir)
> > > +                               create_default_kunitconfig()
> > >
> > >                 if not linux:
> > >                         linux = kunit_kernel.LinuxSourceTree()
> > >
> > > base-commit: 30185b69a2d533c4ba6ca926b8390ce7de495e29
> > > --
> > > 2.28.0.236.gb10cc79966-goog
> > >
diff mbox series

Patch

diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
index 425ef40067e7..96344a11ff1f 100755
--- a/tools/testing/kunit/kunit.py
+++ b/tools/testing/kunit/kunit.py
@@ -237,9 +237,14 @@  def main(argv, linux=None):
 
 	cli_args = parser.parse_args(argv)
 
+	if get_kernel_root_path():
+		print('cd ' + get_kernel_root_path())
+		os.chdir(get_kernel_root_path())
+
 	if cli_args.subcommand == 'run':
 		if not os.path.exists(cli_args.build_dir):
 			os.mkdir(cli_args.build_dir)
+			create_default_kunitconfig()
 
 		if not linux:
 			linux = kunit_kernel.LinuxSourceTree()
@@ -257,6 +262,7 @@  def main(argv, linux=None):
 		if cli_args.build_dir:
 			if not os.path.exists(cli_args.build_dir):
 				os.mkdir(cli_args.build_dir)
+				create_default_kunitconfig()
 
 		if not linux:
 			linux = kunit_kernel.LinuxSourceTree()
@@ -273,6 +279,7 @@  def main(argv, linux=None):
 		if cli_args.build_dir:
 			if not os.path.exists(cli_args.build_dir):
 				os.mkdir(cli_args.build_dir)
+				create_default_kunitconfig()
 
 		if not linux:
 			linux = kunit_kernel.LinuxSourceTree()
@@ -291,6 +298,7 @@  def main(argv, linux=None):
 		if cli_args.build_dir:
 			if not os.path.exists(cli_args.build_dir):
 				os.mkdir(cli_args.build_dir)
+				create_default_kunitconfig()
 
 		if not linux:
 			linux = kunit_kernel.LinuxSourceTree()