Re: [PATCH v2 1/6] patman: Fix updating argument defaults from settings
Hi,
On Thu, Jul 7, 2022 at 8:11 AM Sean Anderson wrote:
>
> On 7/6/22 8:07 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Jul 5, 2022 at 11:16 AM Sean Anderson
> > 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
> >> > Tested-by: Brian Norris
> >> > Reviewed-by: Brian Norris
> >> > ---
> >> >
> >> > (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
Re: [PATCH v2 1/6] patman: Fix updating argument defaults from settings
On 7/6/22 8:07 PM, Doug Anderson wrote:
> Hi,
>
> On Tue, Jul 5, 2022 at 11:16 AM Sean Anderson 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
>> > Tested-by: Brian Norris
>> > Reviewed-by: Brian Norris
>> > ---
>> >
>> > (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
Re: [PATCH v2 1/6] patman: Fix updating argument defaults from settings
Hi Doug,
On Wed, 6 Jul 2022 at 18:08, Doug Anderson wrote:
>
> Hi,
>
> On Tue, Jul 5, 2022 at 11:16 AM Sean Anderson 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
> > > Tested-by: Brian Norris
> > > Reviewed-by: Brian Norris
> > > ---
> > >
> > > (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
Re: [PATCH v2 1/6] patman: Fix updating argument defaults from settings
Hi,
On Tue, Jul 5, 2022 at 11:16 AM Sean Anderson 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
> > Tested-by: Brian Norris
> > Reviewed-by: Brian Norris
> > ---
> >
> > (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
Re: [PATCH v2 1/6] patman: Fix updating argument defaults from settings
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
> Tested-by: Brian Norris
> Reviewed-by: Brian Norris
> ---
>
> (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.ander...@seco.com/
Re: [PATCH v2 1/6] patman: Fix updating argument defaults from settings
On Fri, 1 Jul 2022 at 14:24, 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
> Tested-by: Brian Norris
> Reviewed-by: Brian Norris
> ---
>
> (no changes since v1)
>
> tools/patman/settings.py | 41 +---
> 1 file changed, 22 insertions(+), 19 deletions(-)
+Sean Anderson
Reviewed-by: Simon Glass
[PATCH v2 1/6] patman: Fix updating argument defaults from settings
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
Tested-by: Brian Norris
Reviewed-by: Brian Norris
---
(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)
def _ReadAliasFile(fname):
"""Read in the U-Boot git alias file if it exists.
--
2.37.0.rc0.161.g10f37bed90-goog
