| Message ID | 20220701132328.v2.1.Ic2c915fb5435e9862da7d42116db55791cb327c7@changeid |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | patman: Small fixes plus remove --no-tree from checkpatch for linux | expand |
On Fri, 1 Jul 2022 at 14:24, Douglas Anderson <dianders@chromium.org> wrote: > > Ever since commit 4600767d294d ("patman: Refactor how the default > subcommand works"), when I use patman on the Linux tree I get grumbles > about unknown tags. This is because the Linux default making > process_tags be False wasn't working anymore. > > It appears that the comment claiming that the defaults propagates > through all subparsers no longer works for some reason. > > We're already looking at all the subparsers anyway. Let's just update > each one. > > Fixes: 4600767d294d ("patman: Refactor how the default subcommand works") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > Tested-by: Brian Norris <briannorris@chromium.org> > Reviewed-by: Brian Norris <briannorris@chromium.org> > --- > > (no changes since v1) > > tools/patman/settings.py | 41 +++++++++++++++++++++------------------- > 1 file changed, 22 insertions(+), 19 deletions(-) +Sean Anderson Reviewed-by: Simon Glass <sjg@chromium.org>
Hi Doug, On 7/1/22 4:23 PM, Douglas Anderson wrote: > Ever since commit 4600767d294d ("patman: Refactor how the default > subcommand works"), when I use patman on the Linux tree I get grumbles > about unknown tags. This is because the Linux default making > process_tags be False wasn't working anymore. > > It appears that the comment claiming that the defaults propagates > through all subparsers no longer works for some reason. > > We're already looking at all the subparsers anyway. Let's just update > each one. > > Fixes: 4600767d294d ("patman: Refactor how the default subcommand works") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > Tested-by: Brian Norris <briannorris@chromium.org> > Reviewed-by: Brian Norris <briannorris@chromium.org> > --- > > (no changes since v1) > > tools/patman/settings.py | 41 +++++++++++++++++++++------------------- > 1 file changed, 22 insertions(+), 19 deletions(-) > > diff --git a/tools/patman/settings.py b/tools/patman/settings.py > index 7c2b5c196c06..5eefe3d1f55e 100644 > --- a/tools/patman/settings.py > +++ b/tools/patman/settings.py > @@ -244,28 +244,31 @@ def _UpdateDefaults(main_parser, config): > if isinstance(action, argparse._SubParsersAction) > for _, subparser in action.choices.items()] > > + unknown_settings = set(name for name, val in config.items('settings')) > + > # Collect the defaults from each parser > - defaults = {} > for parser in parsers: > pdefs = parser.parse_known_args()[0] > - defaults.update(vars(pdefs)) > - > - # Go through the settings and collect defaults > - for name, val in config.items('settings'): > - if name in defaults: > - default_val = defaults[name] > - if isinstance(default_val, bool): > - val = config.getboolean('settings', name) > - elif isinstance(default_val, int): > - val = config.getint('settings', name) > - elif isinstance(default_val, str): > - val = config.get('settings', name) > - defaults[name] = val > - else: > - print("WARNING: Unknown setting %s" % name) > - > - # Set all the defaults (this propagates through all subparsers) > - main_parser.set_defaults(**defaults) > + defaults = dict(vars(pdefs)) > + > + # Go through the settings and collect defaults > + for name, val in config.items('settings'): > + if name in defaults: > + default_val = defaults[name] > + if isinstance(default_val, bool): > + val = config.getboolean('settings', name) > + elif isinstance(default_val, int): > + val = config.getint('settings', name) > + elif isinstance(default_val, str): > + val = config.get('settings', name) > + defaults[name] = val > + unknown_settings.discard(name) > + > + # Set all the defaults > + parser.set_defaults(**defaults) > + > + for name in sorted(unknown_settings): > + print("WARNING: Unknown setting %s" % name) Can you see if 4780f7d8a6b ("patman: Fix defaults not propagating to subparsers") [1] addresses this problem? The implementation is different, but I believe these should have the same effect. --Sean [1] https://lore.kernel.org/u-boot/20220429145334.2497202-1-sean.anderson@seco.com/
Hi, On Tue, Jul 5, 2022 at 11:16 AM Sean Anderson <sean.anderson@seco.com> wrote: > > Hi Doug, > > On 7/1/22 4:23 PM, Douglas Anderson wrote: > > Ever since commit 4600767d294d ("patman: Refactor how the default > > subcommand works"), when I use patman on the Linux tree I get grumbles > > about unknown tags. This is because the Linux default making > > process_tags be False wasn't working anymore. > > > > It appears that the comment claiming that the defaults propagates > > through all subparsers no longer works for some reason. > > > > We're already looking at all the subparsers anyway. Let's just update > > each one. > > > > Fixes: 4600767d294d ("patman: Refactor how the default subcommand works") > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > Tested-by: Brian Norris <briannorris@chromium.org> > > Reviewed-by: Brian Norris <briannorris@chromium.org> > > --- > > > > (no changes since v1) > > > > tools/patman/settings.py | 41 +++++++++++++++++++++------------------- > > 1 file changed, 22 insertions(+), 19 deletions(-) > > > > diff --git a/tools/patman/settings.py b/tools/patman/settings.py > > index 7c2b5c196c06..5eefe3d1f55e 100644 > > --- a/tools/patman/settings.py > > +++ b/tools/patman/settings.py > > @@ -244,28 +244,31 @@ def _UpdateDefaults(main_parser, config): > > if isinstance(action, argparse._SubParsersAction) > > for _, subparser in action.choices.items()] > > > > + unknown_settings = set(name for name, val in config.items('settings')) > > + > > # Collect the defaults from each parser > > - defaults = {} > > for parser in parsers: > > pdefs = parser.parse_known_args()[0] > > - defaults.update(vars(pdefs)) > > - > > - # Go through the settings and collect defaults > > - for name, val in config.items('settings'): > > - if name in defaults: > > - default_val = defaults[name] > > - if isinstance(default_val, bool): > > - val = config.getboolean('settings', name) > > - elif isinstance(default_val, int): > > - val = config.getint('settings', name) > > - elif isinstance(default_val, str): > > - val = config.get('settings', name) > > - defaults[name] = val > > - else: > > - print("WARNING: Unknown setting %s" % name) > > - > > - # Set all the defaults (this propagates through all subparsers) > > - main_parser.set_defaults(**defaults) > > + defaults = dict(vars(pdefs)) > > + > > + # Go through the settings and collect defaults > > + for name, val in config.items('settings'): > > + if name in defaults: > > + default_val = defaults[name] > > + if isinstance(default_val, bool): > > + val = config.getboolean('settings', name) > > + elif isinstance(default_val, int): > > + val = config.getint('settings', name) > > + elif isinstance(default_val, str): > > + val = config.get('settings', name) > > + defaults[name] = val > > + unknown_settings.discard(name) > > + > > + # Set all the defaults > > + parser.set_defaults(**defaults) > > + > > + for name in sorted(unknown_settings): > > + print("WARNING: Unknown setting %s" % name) > > Can you see if 4780f7d8a6b ("patman: Fix defaults not propagating to > subparsers") [1] addresses this problem? The implementation is different, > but I believe these should have the same effect. To my mind the logic of your patch is a bit harder to follow, but I believe you're correct that it accomplishes the same thing. ...and my quick test also seems to confirm that yours works fine. Too bad it wasn't already in "-next" or it would have saved me a bit of time... I'm curious whether you agree that the logic in my patch is a little simpler. Should I re-post it as a squashed revert of yours and then apply mine and call it a "simplify" instead of a bugfix? ...or just leave yours alone? If we leave yours alone, I guess my patch #2 needs a trivial rebase to fix a merge conflict. -Doug
Hi Doug, On Wed, 6 Jul 2022 at 18:08, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Jul 5, 2022 at 11:16 AM Sean Anderson <sean.anderson@seco.com> wrote: > > > > Hi Doug, > > > > On 7/1/22 4:23 PM, Douglas Anderson wrote: > > > Ever since commit 4600767d294d ("patman: Refactor how the default > > > subcommand works"), when I use patman on the Linux tree I get grumbles > > > about unknown tags. This is because the Linux default making > > > process_tags be False wasn't working anymore. > > > > > > It appears that the comment claiming that the defaults propagates > > > through all subparsers no longer works for some reason. > > > > > > We're already looking at all the subparsers anyway. Let's just update > > > each one. > > > > > > Fixes: 4600767d294d ("patman: Refactor how the default subcommand works") > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > Tested-by: Brian Norris <briannorris@chromium.org> > > > Reviewed-by: Brian Norris <briannorris@chromium.org> > > > --- > > > > > > (no changes since v1) > > > > > > tools/patman/settings.py | 41 +++++++++++++++++++++------------------- > > > 1 file changed, 22 insertions(+), 19 deletions(-) > > > > > > diff --git a/tools/patman/settings.py b/tools/patman/settings.py > > > index 7c2b5c196c06..5eefe3d1f55e 100644 > > > --- a/tools/patman/settings.py > > > +++ b/tools/patman/settings.py > > > @@ -244,28 +244,31 @@ def _UpdateDefaults(main_parser, config): > > > if isinstance(action, argparse._SubParsersAction) > > > for _, subparser in action.choices.items()] > > > > > > + unknown_settings = set(name for name, val in config.items('settings')) > > > + > > > # Collect the defaults from each parser > > > - defaults = {} > > > for parser in parsers: > > > pdefs = parser.parse_known_args()[0] > > > - defaults.update(vars(pdefs)) > > > - > > > - # Go through the settings and collect defaults > > > - for name, val in config.items('settings'): > > > - if name in defaults: > > > - default_val = defaults[name] > > > - if isinstance(default_val, bool): > > > - val = config.getboolean('settings', name) > > > - elif isinstance(default_val, int): > > > - val = config.getint('settings', name) > > > - elif isinstance(default_val, str): > > > - val = config.get('settings', name) > > > - defaults[name] = val > > > - else: > > > - print("WARNING: Unknown setting %s" % name) > > > - > > > - # Set all the defaults (this propagates through all subparsers) > > > - main_parser.set_defaults(**defaults) > > > + defaults = dict(vars(pdefs)) > > > + > > > + # Go through the settings and collect defaults > > > + for name, val in config.items('settings'): > > > + if name in defaults: > > > + default_val = defaults[name] > > > + if isinstance(default_val, bool): > > > + val = config.getboolean('settings', name) > > > + elif isinstance(default_val, int): > > > + val = config.getint('settings', name) > > > + elif isinstance(default_val, str): > > > + val = config.get('settings', name) > > > + defaults[name] = val > > > + unknown_settings.discard(name) > > > + > > > + # Set all the defaults > > > + parser.set_defaults(**defaults) > > > + > > > + for name in sorted(unknown_settings): > > > + print("WARNING: Unknown setting %s" % name) > > > > Can you see if 4780f7d8a6b ("patman: Fix defaults not propagating to > > subparsers") [1] addresses this problem? The implementation is different, > > but I believe these should have the same effect. > > To my mind the logic of your patch is a bit harder to follow, but I > believe you're correct that it accomplishes the same thing. ...and my > quick test also seems to confirm that yours works fine. Too bad it > wasn't already in "-next" or it would have saved me a bit of time... +Tom Rini It's been languishing for two months due to me taking a break. The PR itself was sent a week ago but is waiting on one discussion. > > I'm curious whether you agree that the logic in my patch is a little > simpler. Should I re-post it as a squashed revert of yours and then > apply mine and call it a "simplify" instead of a bugfix? ...or just > leave yours alone? If we leave yours alone, I guess my patch #2 needs > a trivial rebase to fix a merge conflict. > Regards, Simon
On 7/6/22 8:07 PM, Doug Anderson wrote: > Hi, > > On Tue, Jul 5, 2022 at 11:16 AM Sean Anderson <sean.anderson@seco.com> wrote: >> >> Hi Doug, >> >> On 7/1/22 4:23 PM, Douglas Anderson wrote: >> > Ever since commit 4600767d294d ("patman: Refactor how the default >> > subcommand works"), when I use patman on the Linux tree I get grumbles >> > about unknown tags. This is because the Linux default making >> > process_tags be False wasn't working anymore. >> > >> > It appears that the comment claiming that the defaults propagates >> > through all subparsers no longer works for some reason. >> > >> > We're already looking at all the subparsers anyway. Let's just update >> > each one. >> > >> > Fixes: 4600767d294d ("patman: Refactor how the default subcommand works") >> > Signed-off-by: Douglas Anderson <dianders@chromium.org> >> > Tested-by: Brian Norris <briannorris@chromium.org> >> > Reviewed-by: Brian Norris <briannorris@chromium.org> >> > --- >> > >> > (no changes since v1) >> > >> > tools/patman/settings.py | 41 +++++++++++++++++++++------------------- >> > 1 file changed, 22 insertions(+), 19 deletions(-) >> > >> > diff --git a/tools/patman/settings.py b/tools/patman/settings.py >> > index 7c2b5c196c06..5eefe3d1f55e 100644 >> > --- a/tools/patman/settings.py >> > +++ b/tools/patman/settings.py >> > @@ -244,28 +244,31 @@ def _UpdateDefaults(main_parser, config): >> > if isinstance(action, argparse._SubParsersAction) >> > for _, subparser in action.choices.items()] >> > >> > + unknown_settings = set(name for name, val in config.items('settings')) >> > + >> > # Collect the defaults from each parser >> > - defaults = {} >> > for parser in parsers: >> > pdefs = parser.parse_known_args()[0] >> > - defaults.update(vars(pdefs)) >> > - >> > - # Go through the settings and collect defaults >> > - for name, val in config.items('settings'): >> > - if name in defaults: >> > - default_val = defaults[name] >> > - if isinstance(default_val, bool): >> > - val = config.getboolean('settings', name) >> > - elif isinstance(default_val, int): >> > - val = config.getint('settings', name) >> > - elif isinstance(default_val, str): >> > - val = config.get('settings', name) >> > - defaults[name] = val >> > - else: >> > - print("WARNING: Unknown setting %s" % name) >> > - >> > - # Set all the defaults (this propagates through all subparsers) >> > - main_parser.set_defaults(**defaults) >> > + defaults = dict(vars(pdefs)) >> > + >> > + # Go through the settings and collect defaults >> > + for name, val in config.items('settings'): >> > + if name in defaults: >> > + default_val = defaults[name] >> > + if isinstance(default_val, bool): >> > + val = config.getboolean('settings', name) >> > + elif isinstance(default_val, int): >> > + val = config.getint('settings', name) >> > + elif isinstance(default_val, str): >> > + val = config.get('settings', name) >> > + defaults[name] = val >> > + unknown_settings.discard(name) >> > + >> > + # Set all the defaults >> > + parser.set_defaults(**defaults) >> > + >> > + for name in sorted(unknown_settings): >> > + print("WARNING: Unknown setting %s" % name) >> >> Can you see if 4780f7d8a6b ("patman: Fix defaults not propagating to >> subparsers") [1] addresses this problem? The implementation is different, >> but I believe these should have the same effect. > > To my mind the logic of your patch is a bit harder to follow, but I > believe you're correct that it accomplishes the same thing. ...and my > quick test also seems to confirm that yours works fine. Too bad it > wasn't already in "-next" or it would have saved me a bit of time... > > I'm curious whether you agree that the logic in my patch is a little > simpler. Should I re-post it as a squashed revert of yours and then > apply mine and call it a "simplify" instead of a bugfix? ...or just > leave yours alone? If we leave yours alone, I guess my patch #2 needs > a trivial rebase to fix a merge conflict. IMO my version is simpler, but that is mainly because I thought of it. I have no objection to your rearranging, as long as it works afterwards. --Sean
Hi, On Thu, Jul 7, 2022 at 8:11 AM Sean Anderson <sean.anderson@seco.com> wrote: > > On 7/6/22 8:07 PM, Doug Anderson wrote: > > Hi, > > > > On Tue, Jul 5, 2022 at 11:16 AM Sean Anderson <sean.anderson@seco.com> wrote: > >> > >> Hi Doug, > >> > >> On 7/1/22 4:23 PM, Douglas Anderson wrote: > >> > Ever since commit 4600767d294d ("patman: Refactor how the default > >> > subcommand works"), when I use patman on the Linux tree I get grumbles > >> > about unknown tags. This is because the Linux default making > >> > process_tags be False wasn't working anymore. > >> > > >> > It appears that the comment claiming that the defaults propagates > >> > through all subparsers no longer works for some reason. > >> > > >> > We're already looking at all the subparsers anyway. Let's just update > >> > each one. > >> > > >> > Fixes: 4600767d294d ("patman: Refactor how the default subcommand works") > >> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > >> > Tested-by: Brian Norris <briannorris@chromium.org> > >> > Reviewed-by: Brian Norris <briannorris@chromium.org> > >> > --- > >> > > >> > (no changes since v1) > >> > > >> > tools/patman/settings.py | 41 +++++++++++++++++++++------------------- > >> > 1 file changed, 22 insertions(+), 19 deletions(-) > >> > > >> > diff --git a/tools/patman/settings.py b/tools/patman/settings.py > >> > index 7c2b5c196c06..5eefe3d1f55e 100644 > >> > --- a/tools/patman/settings.py > >> > +++ b/tools/patman/settings.py > >> > @@ -244,28 +244,31 @@ def _UpdateDefaults(main_parser, config): > >> > if isinstance(action, argparse._SubParsersAction) > >> > for _, subparser in action.choices.items()] > >> > > >> > + unknown_settings = set(name for name, val in config.items('settings')) > >> > + > >> > # Collect the defaults from each parser > >> > - defaults = {} > >> > for parser in parsers: > >> > pdefs = parser.parse_known_args()[0] > >> > - defaults.update(vars(pdefs)) > >> > - > >> > - # Go through the settings and collect defaults > >> > - for name, val in config.items('settings'): > >> > - if name in defaults: > >> > - default_val = defaults[name] > >> > - if isinstance(default_val, bool): > >> > - val = config.getboolean('settings', name) > >> > - elif isinstance(default_val, int): > >> > - val = config.getint('settings', name) > >> > - elif isinstance(default_val, str): > >> > - val = config.get('settings', name) > >> > - defaults[name] = val > >> > - else: > >> > - print("WARNING: Unknown setting %s" % name) > >> > - > >> > - # Set all the defaults (this propagates through all subparsers) > >> > - main_parser.set_defaults(**defaults) > >> > + defaults = dict(vars(pdefs)) > >> > + > >> > + # Go through the settings and collect defaults > >> > + for name, val in config.items('settings'): > >> > + if name in defaults: > >> > + default_val = defaults[name] > >> > + if isinstance(default_val, bool): > >> > + val = config.getboolean('settings', name) > >> > + elif isinstance(default_val, int): > >> > + val = config.getint('settings', name) > >> > + elif isinstance(default_val, str): > >> > + val = config.get('settings', name) > >> > + defaults[name] = val > >> > + unknown_settings.discard(name) > >> > + > >> > + # Set all the defaults > >> > + parser.set_defaults(**defaults) > >> > + > >> > + for name in sorted(unknown_settings): > >> > + print("WARNING: Unknown setting %s" % name) > >> > >> Can you see if 4780f7d8a6b ("patman: Fix defaults not propagating to > >> subparsers") [1] addresses this problem? The implementation is different, > >> but I believe these should have the same effect. > > > > To my mind the logic of your patch is a bit harder to follow, but I > > believe you're correct that it accomplishes the same thing. ...and my > > quick test also seems to confirm that yours works fine. Too bad it > > wasn't already in "-next" or it would have saved me a bit of time... > > > > I'm curious whether you agree that the logic in my patch is a little > > simpler. Should I re-post it as a squashed revert of yours and then > > apply mine and call it a "simplify" instead of a bugfix? ...or just > > leave yours alone? If we leave yours alone, I guess my patch #2 needs > > a trivial rebase to fix a merge conflict. > > IMO my version is simpler, but that is mainly because I thought of it. > > I have no objection to your rearranging, as long as it works afterwards. No worries then. I'll drop my patch #1 and post a rebase of the rest of the series. -Doug
diff --git a/tools/patman/settings.py b/tools/patman/settings.py index 7c2b5c196c06..5eefe3d1f55e 100644 --- a/tools/patman/settings.py +++ b/tools/patman/settings.py @@ -244,28 +244,31 @@ def _UpdateDefaults(main_parser, config): if isinstance(action, argparse._SubParsersAction) for _, subparser in action.choices.items()] + unknown_settings = set(name for name, val in config.items('settings')) + # Collect the defaults from each parser - defaults = {} for parser in parsers: pdefs = parser.parse_known_args()[0] - defaults.update(vars(pdefs)) - - # Go through the settings and collect defaults - for name, val in config.items('settings'): - if name in defaults: - default_val = defaults[name] - if isinstance(default_val, bool): - val = config.getboolean('settings', name) - elif isinstance(default_val, int): - val = config.getint('settings', name) - elif isinstance(default_val, str): - val = config.get('settings', name) - defaults[name] = val - else: - print("WARNING: Unknown setting %s" % name) - - # Set all the defaults (this propagates through all subparsers) - main_parser.set_defaults(**defaults) + defaults = dict(vars(pdefs)) + + # Go through the settings and collect defaults + for name, val in config.items('settings'): + if name in defaults: + default_val = defaults[name] + if isinstance(default_val, bool): + val = config.getboolean('settings', name) + elif isinstance(default_val, int): + val = config.getint('settings', name) + elif isinstance(default_val, str): + val = config.get('settings', name) + defaults[name] = val + unknown_settings.discard(name) + + # Set all the defaults + parser.set_defaults(**defaults) + + for name in sorted(unknown_settings): + print("WARNING: Unknown setting %s" % name) def _ReadAliasFile(fname): """Read in the U-Boot git alias file if it exists.
Ever since commit 4600767d294d ("patman: Refactor how the default subcommand works"), when I use patman on the Linux tree I get grumbles about unknown tags. This is because the Linux default making process_tags be False wasn't working anymore. It appears that the comment claiming that the defaults propagates through all subparsers no longer works for some reason. We're already looking at all the subparsers anyway. Let's just update each one. Fixes: 4600767d294d ("patman: Refactor how the default subcommand works") Signed-off-by: Douglas Anderson <dianders@chromium.org> Tested-by: Brian Norris <briannorris@chromium.org> Reviewed-by: Brian Norris <briannorris@chromium.org> --- (no changes since v1) tools/patman/settings.py | 41 +++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-)