| Message ID | 1606238094-28940-3-git-send-email-philipp.tomsich@vrull.eu |
|---|---|
| State | Rejected |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [1/3] patman: Add --no-signoff to suppress adding signoffs | expand |
Hi Philipp, On Tue, 24 Nov 2020 at 10:15, Philipp Tomsich <philipp.tomsich@vrull.eu> wrote: > > Project defaults (e.g. for linux and gcc) do not propagate into the > subparsers. As both the processing of tags (e.g. in the defaults > for the linux project) and supressing the signoff (in the defaults > for the gcc project) are settings from subparsers, these would still > require an explicit commandline option mirroring the (ignored) default. > > This change ensures that defaults are updated in all parsers. > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu> > --- > > tools/patman/settings.py | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tools/patman/settings.py b/tools/patman/settings.py > index bb3f868..dc57b2f 100644 > --- a/tools/patman/settings.py > +++ b/tools/patman/settings.py > @@ -266,7 +266,8 @@ def _UpdateDefaults(main_parser, config): > print("WARNING: Unknown setting %s" % name) > > # Set all the defaults (this propagates through all subparsers) > - main_parser.set_defaults(**defaults) > + for parser in parsers: > + parser.set_defaults(**defaults) According to the Python documentation and my testing, it should propagate. Do you know what is going wrong here? If there is a problem, we should update the comment. > > def _ReadAliasFile(fname): > """Read in the U-Boot git alias file if it exists. > -- > 1.8.3.1 > Regards, Simon
Simon, On Wed, 25 Nov 2020 at 00:41, Simon Glass <sjg@chromium.org> wrote: > > > Project defaults (e.g. for linux and gcc) do not propagate into the > > subparsers. As both the processing of tags (e.g. in the defaults > > for the linux project) and supressing the signoff (in the defaults > > for the gcc project) are settings from subparsers, these would still > > require an explicit commandline option mirroring the (ignored) default. > > > > This change ensures that defaults are updated in all parsers. > > > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu> > > --- > > > > tools/patman/settings.py | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/tools/patman/settings.py b/tools/patman/settings.py > > index bb3f868..dc57b2f 100644 > > --- a/tools/patman/settings.py > > +++ b/tools/patman/settings.py > > @@ -266,7 +266,8 @@ def _UpdateDefaults(main_parser, config): > > print("WARNING: Unknown setting %s" % name) > > > > # Set all the defaults (this propagates through all subparsers) > > - main_parser.set_defaults(**defaults) > > + for parser in parsers: > > + parser.set_defaults(**defaults) > > According to the Python documentation and my testing, it should > propagate. Do you know what is going wrong here? If there is a > problem, we should update the comment. I haven't dug down to find the root-cause, but I see the following behavior both on Debian 10.6 and CentOS 7 when adding a print(args) in main.py just before the __name__ == "__main__" check... With this commit: $ tools/patman/patman -p linux -c1 send -n Namespace(add_maintainers=True, branch=None, cc_cmd=None, check_patch=True, cmd='send', count=1, debug=False, dest_branch=None, dry_run=True, end=0, force=False, full_help=False, ignore_bad_tags=False, ignore_binary=False, ignore_errors=False, in_reply_to=None, limit=None, patchfiles=['linux'], patchwork_url='https://patchwork.ozlabs.org', process_tags=False, project='linux', show_comments=False, smtp_server=None, start=0, testname='linux', thread=False, verbose=False) => process_tags=False Without this commit: Namespace(add_maintainers=True, branch=None, cc_cmd=None, check_patch=True, cmd='send', count=1, debug=False, dest_branch=None, dry_run=True, end=0, force=False, full_help=False, ignore_bad_tags=False, ignore_binary=False, ignore_errors=False, in_reply_to=None, limit=None, patchfiles=[], patchwork_url='https://patchwork.ozlabs.org', process_tags=True, project='linux', show_comments=False, smtp_server=None, start=0, testname='linux', thread=False, verbose=False) => process_tags=True So in my testing, the problem reproduces reliably across distributions. Regards, Philipp
Simon, On Wed, 25 Nov 2020 at 00:41, Simon Glass <sjg@chromium.org> wrote: > According to the Python documentation and my testing, it should > propagate. Do you know what is going wrong here? If there is a > problem, we should update the comment. I don't see any code for propagating this in the argparse module: https://github.com/python/cpython/blob/85c84920f511d0d73a133336daeaf715a022cd64/Lib/argparse.py#L1763 https://github.com/python/cpython/blob/85c84920f511d0d73a133336daeaf715a022cd64/Lib/argparse.py#L1363 Philipp.
Hi Philipp, On Wed, 25 Nov 2020 at 06:02, Philipp Tomsich <philipp.tomsich@vrull.eu> wrote: > > Simon, > > On Wed, 25 Nov 2020 at 00:41, Simon Glass <sjg@chromium.org> wrote: > > According to the Python documentation and my testing, it should > > propagate. Do you know what is going wrong here? If there is a > > problem, we should update the comment. > > I don't see any code for propagating this in the argparse module: > https://github.com/python/cpython/blob/85c84920f511d0d73a133336daeaf715a022cd64/Lib/argparse.py#L1763 > https://github.com/python/cpython/blob/85c84920f511d0d73a133336daeaf715a022cd64/Lib/argparse.py#L1363 Thanks for dingging into this. I'll give it another try and try to figure out why it worked for me, but I expect I'll find the same problem. Here is a pointer to the docs I saw: https://docs.python.org/3/library/argparse.html#argparse.ArgumentParser.set_defaults "Parser-level defaults can be particularly useful when working with multiple parsers. See the add_subparsers() method for an example of this type." Regards, Simon
Simon, On Wed, 25 Nov 2020 at 16:30, Simon Glass <sjg@chromium.org> wrote: > Here is a pointer to the docs I saw: > > https://docs.python.org/3/library/argparse.html#argparse.ArgumentParser.set_defaults > > "Parser-level defaults can be particularly useful when working with > multiple parsers. See the add_subparsers() method for an example of > this type." I had looked at the same documentation and needed to carefully read the example to infer the original author's intended messaging... The only mention of this in the subparser-docs was: > One particularly effective way of handling sub-commands is to combine the > use of the add_subparsers() method with calls to set_defaults() so that > each subparser knows which Python function it should execute. This example just demonstrated that two different subparsers could create a different default for the same argument name. I couldn't find any example of a set_defaults() on a higher-level parser propagating and the argparse-source doesn't seem to try to propagate defaults either. I am starting to think that the correct fix for this would be more along the lines of: > diff --git a/tools/patman/settings.py b/tools/patman/settings.py > index bb3f868..525c69e 100644 > --- a/tools/patman/settings.py > +++ b/tools/patman/settings.py > @@ -266,7 +266,11 @@ def _UpdateDefaults(main_parser, config): > print("WARNING: Unknown setting %s" % name) > > # Set all the defaults (this propagates through all subparsers) > - main_parser.set_defaults(**defaults) > + for parser in parsers: > + for name, val in defaults.items(): > + if parser.get_default(name) and val: > + parser.set_defaults(name=val) > > def _ReadAliasFile(fname): > """Read in the U-Boot git alias file if it exists. > than of what I sent out earlier. An interesting aside: it looks as if the double-parsing of the args leads to 'defaults' being installed for all arguments that are passed in the first cycle (e.g. count, project and patchwork_url suddenly have the values loaded from the config files and parsed from the args populated with 'default' values). Philipp.
Hi Philipp, On Wed, 25 Nov 2020 at 12:41, Philipp Tomsich <philipp.tomsich@vrull.eu> wrote: > > Simon, > > On Wed, 25 Nov 2020 at 16:30, Simon Glass <sjg@chromium.org> wrote: > > Here is a pointer to the docs I saw: > > > > https://docs.python.org/3/library/argparse.html#argparse.ArgumentParser.set_defaults > > > > "Parser-level defaults can be particularly useful when working with > > multiple parsers. See the add_subparsers() method for an example of > > this type." > > I had looked at the same documentation and needed to carefully read the example > to infer the original author's intended messaging... > > The only mention of this in the subparser-docs was: >> >> One particularly effective way of handling sub-commands is to combine the use of the add_subparsers() method with calls to set_defaults() so that each subparser knows which Python function it should execute. > > This example just demonstrated that two different subparsers could create a different > default for the same argument name. I couldn't find any example of a set_defaults() > on a higher-level parser propagating and the argparse-source doesn't seem to try to > propagate defaults either. > > I am starting to think that the correct fix for this would be more along the lines of: >> >> diff --git a/tools/patman/settings.py b/tools/patman/settings.py >> index bb3f868..525c69e 100644 >> --- a/tools/patman/settings.py >> +++ b/tools/patman/settings.py >> @@ -266,7 +266,11 @@ def _UpdateDefaults(main_parser, config): >> print("WARNING: Unknown setting %s" % name) >> >> # Set all the defaults (this propagates through all subparsers) >> - main_parser.set_defaults(**defaults) >> + for parser in parsers: >> + for name, val in defaults.items(): >> + if parser.get_default(name) and val: >> + parser.set_defaults(name=val) >> >> def _ReadAliasFile(fname): >> """Read in the U-Boot git alias file if it exists. > > than of what I sent out earlier. Can you check u-boot/next for this? I have been testing it a bit and from what I can tell the code there does propagate things down to parsers. > > An interesting aside: it looks as if the double-parsing of the args leads to 'defaults' > being installed for all arguments that are passed in the first cycle (e.g. count, > project and patchwork_url suddenly have the values loaded from the config files > and parsed from the args populated with 'default' values). Yes, it isn't very elegant. If you have a better way, it would be nice to improve it. Regards, Simon
diff --git a/tools/patman/settings.py b/tools/patman/settings.py index bb3f868..dc57b2f 100644 --- a/tools/patman/settings.py +++ b/tools/patman/settings.py @@ -266,7 +266,8 @@ def _UpdateDefaults(main_parser, config): print("WARNING: Unknown setting %s" % name) # Set all the defaults (this propagates through all subparsers) - main_parser.set_defaults(**defaults) + for parser in parsers: + parser.set_defaults(**defaults) def _ReadAliasFile(fname): """Read in the U-Boot git alias file if it exists.
Project defaults (e.g. for linux and gcc) do not propagate into the subparsers. As both the processing of tags (e.g. in the defaults for the linux project) and supressing the signoff (in the defaults for the gcc project) are settings from subparsers, these would still require an explicit commandline option mirroring the (ignored) default. This change ensures that defaults are updated in all parsers. Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu> --- tools/patman/settings.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)