| Message ID | 20201203194127.1813731-3-dlatypov@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] kunit: tool: surface and address more typing issues | expand |
On Fri, Dec 4, 2020 at 3:41 AM Daniel Latypov <dlatypov@google.com> wrote: > > LinuxSourceTree will unceremoniously crash if the user doesn't call > read_kunitconfig() first in a number of functions. This patch seems to partly be reverting the changes here, right: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/tools/testing/kunit?h=kunit&id=fcdb0bc08ced274078f371e1e0fe6421a97fa9f2 (That patch moved the reading of kunitconfig out of __init__) My overall concern is that, really, there are some operations that shouldn't need a kunitconfig (even if they do at the moment), so we'd ideally want at least some of the operations currently under LinuxSourceTree to be able to be run without first reading a kunitconfig. Most notably, it'd be nice if kunit.py exec (and hence LinuxSourceTree::run_kernel()) didn't need a kunitconfig, as the kernel ought to already be built at this point. Now, this is all a little bit hypothetical, as we haven't bothered to make kunit.py exec work without a kunitconfig thus far, but I'm a touch hesitant to make it harder to bypass the kunitconfig reading anyway. > > Adn currently every place we create an instance, the caller also calls > create_kunitconfig() and read_kunitconfig(). > > Move these instead into __init__() so they can't be forgotten and to > reduce copy-paste. This seems to now be missing the create_kunitconfig() stuff (see below). > > The https://github.com/google/pytype type-checker complained that > _config wasn't initialized. With this, kunit_tool now type checks > under both pytype and mypy. > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > --- > tools/testing/kunit/kunit.py | 20 ++++---------------- > tools/testing/kunit/kunit_kernel.py | 19 +++++++------------ > 2 files changed, 11 insertions(+), 28 deletions(-) > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py > index 08951a114654..b58fb3733cfa 100755 > --- a/tools/testing/kunit/kunit.py > +++ b/tools/testing/kunit/kunit.py > @@ -256,10 +256,7 @@ def main(argv, linux=None): > os.mkdir(cli_args.build_dir) > > if not linux: > - linux = kunit_kernel.LinuxSourceTree() > - > - linux.create_kunitconfig(cli_args.build_dir) > - linux.read_kunitconfig(cli_args.build_dir) > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) > > request = KunitRequest(cli_args.raw_output, > cli_args.timeout, > @@ -277,10 +274,7 @@ def main(argv, linux=None): > os.mkdir(cli_args.build_dir) > > if not linux: > - linux = kunit_kernel.LinuxSourceTree() > - > - linux.create_kunitconfig(cli_args.build_dir) > - linux.read_kunitconfig(cli_args.build_dir) > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) > > request = KunitConfigRequest(cli_args.build_dir, > cli_args.make_options) > @@ -292,10 +286,7 @@ def main(argv, linux=None): > sys.exit(1) > elif cli_args.subcommand == 'build': > if not linux: > - linux = kunit_kernel.LinuxSourceTree() > - > - linux.create_kunitconfig(cli_args.build_dir) > - linux.read_kunitconfig(cli_args.build_dir) > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) > > request = KunitBuildRequest(cli_args.jobs, > cli_args.build_dir, > @@ -309,10 +300,7 @@ def main(argv, linux=None): > sys.exit(1) > elif cli_args.subcommand == 'exec': > if not linux: > - linux = kunit_kernel.LinuxSourceTree() > - > - linux.create_kunitconfig(cli_args.build_dir) > - linux.read_kunitconfig(cli_args.build_dir) > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) > > exec_request = KunitExecRequest(cli_args.timeout, > cli_args.build_dir, > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py > index bda7c4fd4d3e..79793031d2c4 100644 > --- a/tools/testing/kunit/kunit_kernel.py > +++ b/tools/testing/kunit/kunit_kernel.py > @@ -129,10 +129,15 @@ def get_outfile_path(build_dir) -> str: > class LinuxSourceTree(object): > """Represents a Linux kernel source tree with KUnit tests.""" > > - def __init__(self) -> None: > - self._ops = LinuxSourceTreeOperations() > + def __init__(self, build_dir: str, defconfig=DEFAULT_KUNITCONFIG_PATH) -> None: > signal.signal(signal.SIGINT, self.signal_handler) > > + self._ops = LinuxSourceTreeOperations() > + > + kunitconfig_path = get_kunitconfig_path(build_dir) > + self._kconfig = kunit_config.Kconfig() > + self._kconfig.read_from_file(kunitconfig_path) > + > def clean(self) -> bool: > try: > self._ops.make_mrproper() > @@ -141,16 +146,6 @@ class LinuxSourceTree(object): > return False > return True > > - def create_kunitconfig(self, build_dir, defconfig=DEFAULT_KUNITCONFIG_PATH) -> None: > - kunitconfig_path = get_kunitconfig_path(build_dir) > - if not os.path.exists(kunitconfig_path): > - shutil.copyfile(defconfig, kunitconfig_path) > - What happened to create_kunitconfig() here? With this patch, I can no longer run .../kunit.py run with an empty build_dir and get results, instead getting: --- Traceback (most recent call last): File "./tools/testing/kunit/kunit.py", line 336, in <module> main(sys.argv[1:]) File "./tools/testing/kunit/kunit.py", line 259, in main linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) File "./tools/testing/kunit/kunit_kernel.py", line 139, in __init__ self._kconfig.read_from_file(kunitconfig_path) File "./tools/testing/kunit/kunit_config.py", line 89, in read_from_file with open(path, 'r') as f: FileNotFoundError: [Errno 2] No such file or directory: 'asdf/.kunitconfig' --- Prior to this change, the defconfig is copied over, and the kernel is built, tests run succesfully. > - def read_kunitconfig(self, build_dir) -> None: > - kunitconfig_path = get_kunitconfig_path(build_dir) > - self._kconfig = kunit_config.Kconfig() > - self._kconfig.read_from_file(kunitconfig_path) > - > def validate_config(self, build_dir) -> bool: > kconfig_path = get_kconfig_path(build_dir) > validated_kconfig = kunit_config.Kconfig() > -- > 2.29.2.576.ga3fc446d84-goog >
On Thu, Dec 3, 2020 at 7:57 PM David Gow <davidgow@google.com> wrote: > > On Fri, Dec 4, 2020 at 3:41 AM Daniel Latypov <dlatypov@google.com> wrote: > > > > LinuxSourceTree will unceremoniously crash if the user doesn't call > > read_kunitconfig() first in a number of functions. > > This patch seems to partly be reverting the changes here, right: > https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/tools/testing/kunit?h=kunit&id=fcdb0bc08ced274078f371e1e0fe6421a97fa9f2 > (That patch moved the reading of kunitconfig out of __init__) Yes. > > My overall concern is that, really, there are some operations that > shouldn't need a kunitconfig (even if they do at the moment), so we'd > ideally want at least some of the operations currently under > LinuxSourceTree to be able to be run without first reading a > kunitconfig. Most notably, it'd be nice if kunit.py exec (and hence > LinuxSourceTree::run_kernel()) didn't need a kunitconfig, as the > kernel ought to already be built at this point. > > Now, this is all a little bit hypothetical, as we haven't bothered to > make kunit.py exec work without a kunitconfig thus far, but I'm a > touch hesitant to make it harder to bypass the kunitconfig reading > anyway. Fair point. So one alternative to this to make type-checkers happy is to declare _config instead of sneakily setting it in some random later method. Then in all the places that rely on _config, we'd need to add in checks that it's in fact set to give a better error message (so it's clear to the user that it's an internal tool bug and has nothing to do with them). The copy-paste of create+read_kunitconfig() is annoying, which is why I went with this. How about __init__ takes an optional argument that can disable this parsing? E.g. def __init__(kconfig = None): if kconfig is not None: self._config = kconfig else: // create and read Or if we don't like the idea of requiring users who don't want a kconfig to pass in a dummy, def __init__(load_kconfig=True): if not load_kconfig: self._config = None ... > > > > > Adn currently every place we create an instance, the caller also calls > > create_kunitconfig() and read_kunitconfig(). > > > > Move these instead into __init__() so they can't be forgotten and to > > reduce copy-paste. > > This seems to now be missing the create_kunitconfig() stuff (see below). Ah good catch. Completely unintentional. I'm sure I had the create_kunitconfig() stuff in __init__() at some point but must have inadvertently removed it somehow later on. > > > > The https://github.com/google/pytype type-checker complained that > > _config wasn't initialized. With this, kunit_tool now type checks > > under both pytype and mypy. > > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > --- > > tools/testing/kunit/kunit.py | 20 ++++---------------- > > tools/testing/kunit/kunit_kernel.py | 19 +++++++------------ > > 2 files changed, 11 insertions(+), 28 deletions(-) > > > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py > > index 08951a114654..b58fb3733cfa 100755 > > --- a/tools/testing/kunit/kunit.py > > +++ b/tools/testing/kunit/kunit.py > > @@ -256,10 +256,7 @@ def main(argv, linux=None): > > os.mkdir(cli_args.build_dir) > > > > if not linux: > > - linux = kunit_kernel.LinuxSourceTree() > > - > > - linux.create_kunitconfig(cli_args.build_dir) > > - linux.read_kunitconfig(cli_args.build_dir) > > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) > > > > request = KunitRequest(cli_args.raw_output, > > cli_args.timeout, > > @@ -277,10 +274,7 @@ def main(argv, linux=None): > > os.mkdir(cli_args.build_dir) > > > > if not linux: > > - linux = kunit_kernel.LinuxSourceTree() > > - > > - linux.create_kunitconfig(cli_args.build_dir) > > - linux.read_kunitconfig(cli_args.build_dir) > > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) > > > > request = KunitConfigRequest(cli_args.build_dir, > > cli_args.make_options) > > @@ -292,10 +286,7 @@ def main(argv, linux=None): > > sys.exit(1) > > elif cli_args.subcommand == 'build': > > if not linux: > > - linux = kunit_kernel.LinuxSourceTree() > > - > > - linux.create_kunitconfig(cli_args.build_dir) > > - linux.read_kunitconfig(cli_args.build_dir) > > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) > > > > request = KunitBuildRequest(cli_args.jobs, > > cli_args.build_dir, > > @@ -309,10 +300,7 @@ def main(argv, linux=None): > > sys.exit(1) > > elif cli_args.subcommand == 'exec': > > if not linux: > > - linux = kunit_kernel.LinuxSourceTree() > > - > > - linux.create_kunitconfig(cli_args.build_dir) > > - linux.read_kunitconfig(cli_args.build_dir) > > + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) > > > > exec_request = KunitExecRequest(cli_args.timeout, > > cli_args.build_dir, > > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py > > index bda7c4fd4d3e..79793031d2c4 100644 > > --- a/tools/testing/kunit/kunit_kernel.py > > +++ b/tools/testing/kunit/kunit_kernel.py > > @@ -129,10 +129,15 @@ def get_outfile_path(build_dir) -> str: > > class LinuxSourceTree(object): > > """Represents a Linux kernel source tree with KUnit tests.""" > > > > - def __init__(self) -> None: > > - self._ops = LinuxSourceTreeOperations() > > + def __init__(self, build_dir: str, defconfig=DEFAULT_KUNITCONFIG_PATH) -> None: > > signal.signal(signal.SIGINT, self.signal_handler) > > > > + self._ops = LinuxSourceTreeOperations() > > + > > + kunitconfig_path = get_kunitconfig_path(build_dir) > > + self._kconfig = kunit_config.Kconfig() > > + self._kconfig.read_from_file(kunitconfig_path) > > + > > def clean(self) -> bool: > > try: > > self._ops.make_mrproper() > > @@ -141,16 +146,6 @@ class LinuxSourceTree(object): > > return False > > return True > > > > - def create_kunitconfig(self, build_dir, defconfig=DEFAULT_KUNITCONFIG_PATH) -> None: > > - kunitconfig_path = get_kunitconfig_path(build_dir) > > - if not os.path.exists(kunitconfig_path): > > - shutil.copyfile(defconfig, kunitconfig_path) > > - > > What happened to create_kunitconfig() here? With this patch, I can no > longer run .../kunit.py run with an empty build_dir and get results, > instead getting: > --- > Traceback (most recent call last): > File "./tools/testing/kunit/kunit.py", line 336, in <module> > main(sys.argv[1:]) > File "./tools/testing/kunit/kunit.py", line 259, in main > linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) > File "./tools/testing/kunit/kunit_kernel.py", line 139, in __init__ > self._kconfig.read_from_file(kunitconfig_path) > File "./tools/testing/kunit/kunit_config.py", line 89, in read_from_file > with open(path, 'r') as f: > FileNotFoundError: [Errno 2] No such file or directory: 'asdf/.kunitconfig' > --- > > Prior to this change, the defconfig is copied over, and the kernel is > built, tests run succesfully. > > > > - def read_kunitconfig(self, build_dir) -> None: > > - kunitconfig_path = get_kunitconfig_path(build_dir) > > - self._kconfig = kunit_config.Kconfig() > > - self._kconfig.read_from_file(kunitconfig_path) > > - > > def validate_config(self, build_dir) -> bool: > > kconfig_path = get_kconfig_path(build_dir) > > validated_kconfig = kunit_config.Kconfig() > > -- > > 2.29.2.576.ga3fc446d84-goog > >
On Sat, Dec 5, 2020 at 2:18 AM Daniel Latypov <dlatypov@google.com> wrote: > > On Thu, Dec 3, 2020 at 7:57 PM David Gow <davidgow@google.com> wrote: > > > > On Fri, Dec 4, 2020 at 3:41 AM Daniel Latypov <dlatypov@google.com> wrote: > > > > > > LinuxSourceTree will unceremoniously crash if the user doesn't call > > > read_kunitconfig() first in a number of functions. > > > > This patch seems to partly be reverting the changes here, right: > > https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/tools/testing/kunit?h=kunit&id=fcdb0bc08ced274078f371e1e0fe6421a97fa9f2 > > (That patch moved the reading of kunitconfig out of __init__) > > Yes. > > > > > My overall concern is that, really, there are some operations that > > shouldn't need a kunitconfig (even if they do at the moment), so we'd > > ideally want at least some of the operations currently under > > LinuxSourceTree to be able to be run without first reading a > > kunitconfig. Most notably, it'd be nice if kunit.py exec (and hence > > LinuxSourceTree::run_kernel()) didn't need a kunitconfig, as the > > kernel ought to already be built at this point. > > > > Now, this is all a little bit hypothetical, as we haven't bothered to > > make kunit.py exec work without a kunitconfig thus far, but I'm a > > touch hesitant to make it harder to bypass the kunitconfig reading > > anyway. > > Fair point. > > So one alternative to this to make type-checkers happy is to declare > _config instead of sneakily setting it in some random later method. > Then in all the places that rely on _config, we'd need to add in > checks that it's in fact set to give a better error message (so it's > clear to the user that it's an internal tool bug and has nothing to do > with them). This seems plausible, if a bit verbose. > > The copy-paste of create+read_kunitconfig() is annoying, which is why > I went with this. Personally, the duplication of calls to {create,read}_kunitconfig() doesn't bother me, but I definitely can see the advantage of having the type system pick up when we've missed one. > How about __init__ takes an optional argument that can disable this parsing? This would be okay: I'm starting to feel that really, the ultimate solution is either to split LinuxSourceTree up (and have separate things for configuring, building, and running the kernel), or to pass the kconfig stuff into just the functions that require it. But that is a much more serious refactor, which I haven't fully thought through, and I don't want to let the perfect be the enemy of the good here. > > E.g. > > def __init__(kconfig = None): > if kconfig is not None: > self._config = kconfig > else: > // create and read > What would the kconfig argument here be? Just an empty Kconfig()? I'm not a huge fan of passing a "None" kconfig object when we want to load a config, and a non-None one when we want an empty one: that seems confusingly backwards. Maybe it'd be possible to move the loading of the kunitconfig outside LinuxSourceTree, and pass that (or an empty one) as needed? > Or if we don't like the idea of requiring users who don't want a > kconfig to pass in a dummy, > > def __init__(load_kconfig=True): > if not load_kconfig: > self._config = None > ... > I slightly prefer this, for the reasons above: True/False makes more sense than None/Kconfig(). > > > > > > > > Adn currently every place we create an instance, the caller also calls > > > create_kunitconfig() and read_kunitconfig(). > > > > > > Move these instead into __init__() so they can't be forgotten and to > > > reduce copy-paste. > > > > This seems to now be missing the create_kunitconfig() stuff (see below). > > Ah good catch. Completely unintentional. > I'm sure I had the create_kunitconfig() stuff in __init__() at some > point but must have inadvertently removed it somehow later on. > > > > > > > The https://github.com/google/pytype type-checker complained that > > > _config wasn't initialized. With this, kunit_tool now type checks > > > under both pytype and mypy. > > > > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > > --- Okay, so it looks like there are a few options with _kconfig: 1. Check for None everywhere (after explicitly setting it in the constructor). Pros: Nicer error messages, doesn't require other changes, Cons: verbose, still somewhat prone to error (could forget {create,read}_kunitconfig()) 2. Pass a Kconfig object into the constructor. Pros: a kconfig must exist, so less error prone, Cons: if we allow passing None to load it, that's confusing. 3. Pass a bool into the constructor. Pros: similarly less error prone. Cons: True/False is not very descriptive. 4. Refactor and (e.g.) pass the kconfig only into functions which require it. Pros: removes a "false dependency" on a kconfig for things which don't need it. Cons: more work, more verbose. Of these, I probably like them in this order 4, 2 (without None), 3, 2 (with None), 1. But, to be honest, I'd be happy enough with any of them. So, if we can get a v2 that fixes the missing create_kunitconfig(), and does any of the above things, that sounds good to me. Cheers, -- David
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 08951a114654..b58fb3733cfa 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -256,10 +256,7 @@ def main(argv, linux=None): os.mkdir(cli_args.build_dir) if not linux: - linux = kunit_kernel.LinuxSourceTree() - - linux.create_kunitconfig(cli_args.build_dir) - linux.read_kunitconfig(cli_args.build_dir) + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) request = KunitRequest(cli_args.raw_output, cli_args.timeout, @@ -277,10 +274,7 @@ def main(argv, linux=None): os.mkdir(cli_args.build_dir) if not linux: - linux = kunit_kernel.LinuxSourceTree() - - linux.create_kunitconfig(cli_args.build_dir) - linux.read_kunitconfig(cli_args.build_dir) + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) request = KunitConfigRequest(cli_args.build_dir, cli_args.make_options) @@ -292,10 +286,7 @@ def main(argv, linux=None): sys.exit(1) elif cli_args.subcommand == 'build': if not linux: - linux = kunit_kernel.LinuxSourceTree() - - linux.create_kunitconfig(cli_args.build_dir) - linux.read_kunitconfig(cli_args.build_dir) + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) request = KunitBuildRequest(cli_args.jobs, cli_args.build_dir, @@ -309,10 +300,7 @@ def main(argv, linux=None): sys.exit(1) elif cli_args.subcommand == 'exec': if not linux: - linux = kunit_kernel.LinuxSourceTree() - - linux.create_kunitconfig(cli_args.build_dir) - linux.read_kunitconfig(cli_args.build_dir) + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) exec_request = KunitExecRequest(cli_args.timeout, cli_args.build_dir, diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index bda7c4fd4d3e..79793031d2c4 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -129,10 +129,15 @@ def get_outfile_path(build_dir) -> str: class LinuxSourceTree(object): """Represents a Linux kernel source tree with KUnit tests.""" - def __init__(self) -> None: - self._ops = LinuxSourceTreeOperations() + def __init__(self, build_dir: str, defconfig=DEFAULT_KUNITCONFIG_PATH) -> None: signal.signal(signal.SIGINT, self.signal_handler) + self._ops = LinuxSourceTreeOperations() + + kunitconfig_path = get_kunitconfig_path(build_dir) + self._kconfig = kunit_config.Kconfig() + self._kconfig.read_from_file(kunitconfig_path) + def clean(self) -> bool: try: self._ops.make_mrproper() @@ -141,16 +146,6 @@ class LinuxSourceTree(object): return False return True - def create_kunitconfig(self, build_dir, defconfig=DEFAULT_KUNITCONFIG_PATH) -> None: - kunitconfig_path = get_kunitconfig_path(build_dir) - if not os.path.exists(kunitconfig_path): - shutil.copyfile(defconfig, kunitconfig_path) - - def read_kunitconfig(self, build_dir) -> None: - kunitconfig_path = get_kunitconfig_path(build_dir) - self._kconfig = kunit_config.Kconfig() - self._kconfig.read_from_file(kunitconfig_path) - def validate_config(self, build_dir) -> bool: kconfig_path = get_kconfig_path(build_dir) validated_kconfig = kunit_config.Kconfig()
LinuxSourceTree will unceremoniously crash if the user doesn't call read_kunitconfig() first in a number of functions. Adn currently every place we create an instance, the caller also calls create_kunitconfig() and read_kunitconfig(). Move these instead into __init__() so they can't be forgotten and to reduce copy-paste. The https://github.com/google/pytype type-checker complained that _config wasn't initialized. With this, kunit_tool now type checks under both pytype and mypy. Signed-off-by: Daniel Latypov <dlatypov@google.com> --- tools/testing/kunit/kunit.py | 20 ++++---------------- tools/testing/kunit/kunit_kernel.py | 19 +++++++------------ 2 files changed, 11 insertions(+), 28 deletions(-)