| Message ID | 20201012102621.32226-2-sjpark@amazon.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
ping On Mon, 12 Oct 2020 12:26:21 +0200 SeongJae Park <sjpark@amazon.com> wrote: > From: SeongJae Park <sjpark@amazon.de> > > 'kunit_kernel.kunittest_config' was constant at first, and therefore it > used UPPER_SNAKE_CASE naming convention that usually means it is > constant in Python world. But, commit e3212513a8f0 ("kunit: Create > default config in '--build_dir'") made it modifiable to fix a use case > of the tool and thus the naming also changed to lower_snake_case. > However, this resulted in a confusion. As a result, some successing > changes made the tool unittest fail, and a fix[1] of it again incurred > the '--build_dir' use case failure. > > As the previous commit fixed the '--build_dir' use case without > modifying the variable again, this commit marks the variable as constant > again with UPPER_SNAKE_CASE, to reduce future confusions. > > [1] Commit d43c7fb05765 ("kunit: tool: fix improper treatment of file location") > > Signed-off-by: SeongJae Park <sjpark@amazon.de> > --- > tools/testing/kunit/kunit.py | 4 ++-- > tools/testing/kunit/kunit_kernel.py | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py > index 611c23e178f8..0a58c1fb87d9 100755 > --- a/tools/testing/kunit/kunit.py > +++ b/tools/testing/kunit/kunit.py > @@ -44,9 +44,9 @@ class KunitStatus(Enum): > TEST_FAILURE = auto() > > def create_default_kunitconfig(): > - if not os.path.exists(kunit_kernel.kunitconfig_path): > + if not os.path.exists(kunit_kernel.KUNITCONFIG_PATH): > shutil.copyfile('arch/um/configs/kunit_defconfig', > - kunit_kernel.kunitconfig_path) > + kunit_kernel.KUNITCONFIG_PATH) > > def get_kernel_root_path(): > parts = sys.argv[0] if not __file__ else __file__ > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py > index 16a997504317..42dca0163479 100644 > --- a/tools/testing/kunit/kunit_kernel.py > +++ b/tools/testing/kunit/kunit_kernel.py > @@ -18,7 +18,7 @@ import kunit_config > import kunit_parser > > KCONFIG_PATH = '.config' > -kunitconfig_path = '.kunitconfig' > +KUNITCONFIG_PATH = '.kunitconfig' > BROKEN_ALLCONFIG_PATH = 'tools/testing/kunit/configs/broken_on_uml.config' > > class ConfigError(Exception): > @@ -106,7 +106,7 @@ class LinuxSourceTree(object): > > def __init__(self, build_dir): > self._kconfig = kunit_config.Kconfig() > - self._kconfig.read_from_file(os.path.join(build_dir, kunitconfig_path)) > + self._kconfig.read_from_file(os.path.join(build_dir, KUNITCONFIG_PATH)) > self._ops = LinuxSourceTreeOperations() > signal.signal(signal.SIGINT, self.signal_handler) > > -- > 2.17.1 >
On Mon, Oct 12, 2020 at 3:27 AM SeongJae Park <sjpark@amazon.com> wrote: > > From: SeongJae Park <sjpark@amazon.de> > > 'kunit_kernel.kunittest_config' was constant at first, and therefore it > used UPPER_SNAKE_CASE naming convention that usually means it is > constant in Python world. But, commit e3212513a8f0 ("kunit: Create > default config in '--build_dir'") made it modifiable to fix a use case > of the tool and thus the naming also changed to lower_snake_case. > However, this resulted in a confusion. As a result, some successing > changes made the tool unittest fail, and a fix[1] of it again incurred > the '--build_dir' use case failure. > > As the previous commit fixed the '--build_dir' use case without > modifying the variable again, this commit marks the variable as constant > again with UPPER_SNAKE_CASE, to reduce future confusions. > > [1] Commit d43c7fb05765 ("kunit: tool: fix improper treatment of file location") > > Signed-off-by: SeongJae Park <sjpark@amazon.de> Reviewed-by: Brendan Higgins <brendanhiggins@google.com> Thanks for this! This is something I meant to fix a while ago and forgot about. One minor issue, this patch does not apply on torvalds/master right now. Could you please rebase this?
On Wed, 21 Oct 2020 14:32:52 -0700 Brendan Higgins <brendanhiggins@google.com> wrote: > On Mon, Oct 12, 2020 at 3:27 AM SeongJae Park <sjpark@amazon.com> wrote: > > > > From: SeongJae Park <sjpark@amazon.de> > > > > 'kunit_kernel.kunittest_config' was constant at first, and therefore it > > used UPPER_SNAKE_CASE naming convention that usually means it is > > constant in Python world. But, commit e3212513a8f0 ("kunit: Create > > default config in '--build_dir'") made it modifiable to fix a use case > > of the tool and thus the naming also changed to lower_snake_case. > > However, this resulted in a confusion. As a result, some successing > > changes made the tool unittest fail, and a fix[1] of it again incurred > > the '--build_dir' use case failure. > > > > As the previous commit fixed the '--build_dir' use case without > > modifying the variable again, this commit marks the variable as constant > > again with UPPER_SNAKE_CASE, to reduce future confusions. > > > > [1] Commit d43c7fb05765 ("kunit: tool: fix improper treatment of file location") > > > > Signed-off-by: SeongJae Park <sjpark@amazon.de> > > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> Thanks :) > > Thanks for this! This is something I meant to fix a while ago and forgot about. > > One minor issue, this patch does not apply on torvalds/master right > now. Could you please rebase this? Surely of course, I will send next version soon. Thanks, SeongJae Park
On Thu, Oct 22, 2020 at 08:35:26AM +0200, SeongJae Park wrote: > On Wed, 21 Oct 2020 14:32:52 -0700 Brendan Higgins <brendanhiggins@google.com> wrote: > > > On Mon, Oct 12, 2020 at 3:27 AM SeongJae Park <sjpark@amazon.com> wrote: > > > > > > From: SeongJae Park <sjpark@amazon.de> > > > > > > 'kunit_kernel.kunittest_config' was constant at first, and therefore it > > > used UPPER_SNAKE_CASE naming convention that usually means it is > > > constant in Python world. But, commit e3212513a8f0 ("kunit: Create > > > default config in '--build_dir'") made it modifiable to fix a use case > > > of the tool and thus the naming also changed to lower_snake_case. > > > However, this resulted in a confusion. As a result, some successing > > > changes made the tool unittest fail, and a fix[1] of it again incurred > > > the '--build_dir' use case failure. > > > > > > As the previous commit fixed the '--build_dir' use case without > > > modifying the variable again, this commit marks the variable as constant > > > again with UPPER_SNAKE_CASE, to reduce future confusions. > > > > > > [1] Commit d43c7fb05765 ("kunit: tool: fix improper treatment of file location") > > > > > > Signed-off-by: SeongJae Park <sjpark@amazon.de> > > > > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > > Thanks :) > > > > > Thanks for this! This is something I meant to fix a while ago and forgot about. > > > > One minor issue, this patch does not apply on torvalds/master right > > now. Could you please rebase this? > > Surely of course, I will send next version soon. May I ask what happened to [1]? I mean it seems these two are goind to collide. Brendan? [1]: https://lore.kernel.org/linux-kselftest/20201015152348.65147-1-andriy.shevchenko@linux.intel.com/
On Sun, Oct 25, 2020 at 5:45 AM <andy@surfacebook.localdomain> wrote: > > On Thu, Oct 22, 2020 at 08:35:26AM +0200, SeongJae Park wrote: > > On Wed, 21 Oct 2020 14:32:52 -0700 Brendan Higgins <brendanhiggins@google.com> wrote: > > > > > On Mon, Oct 12, 2020 at 3:27 AM SeongJae Park <sjpark@amazon.com> wrote: > > > > > > > > From: SeongJae Park <sjpark@amazon.de> > > > > > > > > 'kunit_kernel.kunittest_config' was constant at first, and therefore it > > > > used UPPER_SNAKE_CASE naming convention that usually means it is > > > > constant in Python world. But, commit e3212513a8f0 ("kunit: Create > > > > default config in '--build_dir'") made it modifiable to fix a use case > > > > of the tool and thus the naming also changed to lower_snake_case. > > > > However, this resulted in a confusion. As a result, some successing > > > > changes made the tool unittest fail, and a fix[1] of it again incurred > > > > the '--build_dir' use case failure. > > > > > > > > As the previous commit fixed the '--build_dir' use case without > > > > modifying the variable again, this commit marks the variable as constant > > > > again with UPPER_SNAKE_CASE, to reduce future confusions. > > > > > > > > [1] Commit d43c7fb05765 ("kunit: tool: fix improper treatment of file location") > > > > > > > > Signed-off-by: SeongJae Park <sjpark@amazon.de> > > > > > > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > > > > Thanks :) > > > > > > > > Thanks for this! This is something I meant to fix a while ago and forgot about. > > > > > > One minor issue, this patch does not apply on torvalds/master right > > > now. Could you please rebase this? > > > > Surely of course, I will send next version soon. > > May I ask what happened to [1]? > I mean it seems these two are goind to collide. > > Brendan? > > [1]: https://lore.kernel.org/linux-kselftest/20201015152348.65147-1-andriy.shevchenko@linux.intel.com/ Sorry for the confusion here. After an initial glance at your patches (before I did the review end of last week) I thought only the first patch from SeongJae would potentially conflict with yours (Andy's) (hence why I hadn't reviewed it yet, I was waiting until after I looked at yours). I noticed on Friday that SeongJae's changes were actually fully encompassed by Andy's, so I am taking Andy's not SongJae's. Sorry, I was going to notify SongJae today, but you beat me to it. Sorry everyone.
On Mon, 26 Oct 2020 13:44:33 -0700 Brendan Higgins <brendanhiggins@google.com> wrote: > On Sun, Oct 25, 2020 at 5:45 AM <andy@surfacebook.localdomain> wrote: > > > > On Thu, Oct 22, 2020 at 08:35:26AM +0200, SeongJae Park wrote: > > > On Wed, 21 Oct 2020 14:32:52 -0700 Brendan Higgins <brendanhiggins@google.com> wrote: > > > > > > > On Mon, Oct 12, 2020 at 3:27 AM SeongJae Park <sjpark@amazon.com> wrote: > > > > > > > > > > From: SeongJae Park <sjpark@amazon.de> > > > > > > > > > > 'kunit_kernel.kunittest_config' was constant at first, and therefore it > > > > > used UPPER_SNAKE_CASE naming convention that usually means it is > > > > > constant in Python world. But, commit e3212513a8f0 ("kunit: Create > > > > > default config in '--build_dir'") made it modifiable to fix a use case > > > > > of the tool and thus the naming also changed to lower_snake_case. > > > > > However, this resulted in a confusion. As a result, some successing > > > > > changes made the tool unittest fail, and a fix[1] of it again incurred > > > > > the '--build_dir' use case failure. > > > > > > > > > > As the previous commit fixed the '--build_dir' use case without > > > > > modifying the variable again, this commit marks the variable as constant > > > > > again with UPPER_SNAKE_CASE, to reduce future confusions. > > > > > > > > > > [1] Commit d43c7fb05765 ("kunit: tool: fix improper treatment of file location") > > > > > > > > > > Signed-off-by: SeongJae Park <sjpark@amazon.de> > > > > > > > > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > > > > > > Thanks :) > > > > > > > > > > > Thanks for this! This is something I meant to fix a while ago and forgot about. > > > > > > > > One minor issue, this patch does not apply on torvalds/master right > > > > now. Could you please rebase this? > > > > > > Surely of course, I will send next version soon. > > > > May I ask what happened to [1]? > > I mean it seems these two are goind to collide. > > > > Brendan? > > > > [1]: https://lore.kernel.org/linux-kselftest/20201015152348.65147-1-andriy.shevchenko@linux.intel.com/ > > Sorry for the confusion here. After an initial glance at your patches > (before I did the review end of last week) I thought only the first > patch from SeongJae would potentially conflict with yours (Andy's) > (hence why I hadn't reviewed it yet, I was waiting until after I > looked at yours). > > I noticed on Friday that SeongJae's changes were actually fully > encompassed by Andy's, so I am taking Andy's not SongJae's. Sorry, I > was going to notify SongJae today, but you beat me to it. > > Sorry everyone. It's ok, I understand the situation and respect your decision. After all, what I really wanted was just fixing the problem by whoever. I would like to say thank you to Andy for fixing it instead of me :) Also, thank you Brendan, for maintaining the cool Kunit ;) Thanks, SeongJae Park
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 611c23e178f8..0a58c1fb87d9 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -44,9 +44,9 @@ class KunitStatus(Enum): TEST_FAILURE = auto() def create_default_kunitconfig(): - if not os.path.exists(kunit_kernel.kunitconfig_path): + if not os.path.exists(kunit_kernel.KUNITCONFIG_PATH): shutil.copyfile('arch/um/configs/kunit_defconfig', - kunit_kernel.kunitconfig_path) + kunit_kernel.KUNITCONFIG_PATH) def get_kernel_root_path(): parts = sys.argv[0] if not __file__ else __file__ diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index 16a997504317..42dca0163479 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -18,7 +18,7 @@ import kunit_config import kunit_parser KCONFIG_PATH = '.config' -kunitconfig_path = '.kunitconfig' +KUNITCONFIG_PATH = '.kunitconfig' BROKEN_ALLCONFIG_PATH = 'tools/testing/kunit/configs/broken_on_uml.config' class ConfigError(Exception): @@ -106,7 +106,7 @@ class LinuxSourceTree(object): def __init__(self, build_dir): self._kconfig = kunit_config.Kconfig() - self._kconfig.read_from_file(os.path.join(build_dir, kunitconfig_path)) + self._kconfig.read_from_file(os.path.join(build_dir, KUNITCONFIG_PATH)) self._ops = LinuxSourceTreeOperations() signal.signal(signal.SIGINT, self.signal_handler)