* [PATCH v3 0/5] patman: Small fixes plus remove --no-tree from checkpatch for linux
@ 2022-07-07 15:23 Douglas Anderson
2022-07-07 15:23 ` [PATCH v3 1/5] patman: Fix implicit command inserting Douglas Anderson
` (7 more replies)
0 siblings, 8 replies; 12+ messages in thread
From: Douglas Anderson @ 2022-07-07 15:23 UTC (permalink / raw)
To: sjg; +Cc: amstan, mka, Sean Anderson, briannorris, Douglas Anderson, u-boot
The whole point of this series is really to make it so that when we're
using patman for sending Linux patches that we don't pass "--no-tree"
to checkpatch. While doing that, though, I found a number of bugs
including an explanation about why recent version of patman have been
yelling about "tags" when used with Linux even though Linux is
supposed to have "process_tags" defaulted to False.
Changes in v3:
- Rebased atop ("patman: Fix defaults not propagating to subparsers")
Changes in v2:
- Fix doc string for --ignore-bad-tags
- Make comment about parsing three times less nonsensical.
Douglas Anderson (5):
patman: Fix implicit command inserting
patman: Don't look at sys.argv when parsing settings
patman: Make most bool arguments BooleanOptionalAction
patman: By default don't pass "--no-tree" to checkpatch for linux
patman: Take project defaults into account for --help
tools/patman/checkpatch.py | 11 ++++--
tools/patman/control.py | 7 ++--
tools/patman/main.py | 77 ++++++++++++++++++++++----------------
tools/patman/settings.py | 5 ++-
4 files changed, 58 insertions(+), 42 deletions(-)
--
2.37.0.rc0.161.g10f37bed90-goog
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 1/5] patman: Fix implicit command inserting
2022-07-07 15:23 [PATCH v3 0/5] patman: Small fixes plus remove --no-tree from checkpatch for linux Douglas Anderson
@ 2022-07-07 15:23 ` Douglas Anderson
2022-07-07 15:23 ` [PATCH v3 2/5] patman: Don't look at sys.argv when parsing settings Douglas Anderson
` (6 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Douglas Anderson @ 2022-07-07 15:23 UTC (permalink / raw)
To: sjg; +Cc: amstan, mka, Sean Anderson, briannorris, Douglas Anderson, u-boot
The logic to insert an implicit command has always been a bit broken
but it was masked by another bug fixed in the patch ("patman: Don't
look at sys.argv when parsing settings"). Specifically, imagine that
you're just calling patman like this:
patman -c1
After the parse_known_args() command then the "-c1" will have been
parsed and we'll have no command. The "rest" variable will be an empty
list. Going into the logic you can see that nargs = 0. The implicit
insertion of send ideally would create an argument list of:
['-c1', 'send']
...but it doesn't because argv[:-0] is the same as argv[:0] and that's
an empty list.
Let's fix this little glitch.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
(no changes since v1)
tools/patman/main.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/patman/main.py b/tools/patman/main.py
index 66d4806c8d8f..e868878ad5a7 100755
--- a/tools/patman/main.py
+++ b/tools/patman/main.py
@@ -119,7 +119,9 @@ else:
# No command, so insert it after the known arguments and before the ones
# that presumably relate to the 'send' subcommand
nargs = len(rest)
- argv = argv[:-nargs] + ['send'] + rest
+ if nargs:
+ argv = argv[:-nargs]
+ argv = argv + ['send'] + rest
args = parser.parse_args(argv)
if __name__ != "__main__":
--
2.37.0.rc0.161.g10f37bed90-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 2/5] patman: Don't look at sys.argv when parsing settings
2022-07-07 15:23 [PATCH v3 0/5] patman: Small fixes plus remove --no-tree from checkpatch for linux Douglas Anderson
2022-07-07 15:23 ` [PATCH v3 1/5] patman: Fix implicit command inserting Douglas Anderson
@ 2022-07-07 15:23 ` Douglas Anderson
2022-07-07 15:23 ` [PATCH v3 3/5] patman: Make most bool arguments BooleanOptionalAction Douglas Anderson
` (5 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Douglas Anderson @ 2022-07-07 15:23 UTC (permalink / raw)
To: sjg; +Cc: amstan, mka, Sean Anderson, briannorris, Douglas Anderson, u-boot
If you call the parser and tell it to parse but don't pass arguments
in then it will default to looking at sys.argv. This isn't really what
was intended and seems to have some side effects. Let's not do it.
NOTE: to see some of the side effects, note that this patch breaks
"patman -c1" if you don't have the patch ("patman: Fix implicit
command inserting") before it.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
Changes in v3:
- Rebased atop ("patman: Fix defaults not propagating to subparsers")
tools/patman/settings.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/patman/settings.py b/tools/patman/settings.py
index 4c847fe88fd4..6c5f6c8ed992 100644
--- a/tools/patman/settings.py
+++ b/tools/patman/settings.py
@@ -248,7 +248,7 @@ def _UpdateDefaults(main_parser, config):
defaults = {}
parser_defaults = []
for parser in parsers:
- pdefs = parser.parse_known_args()[0]
+ pdefs = parser.parse_known_args([])[0]
parser_defaults.append(pdefs)
defaults.update(vars(pdefs))
--
2.37.0.rc0.161.g10f37bed90-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 3/5] patman: Make most bool arguments BooleanOptionalAction
2022-07-07 15:23 [PATCH v3 0/5] patman: Small fixes plus remove --no-tree from checkpatch for linux Douglas Anderson
2022-07-07 15:23 ` [PATCH v3 1/5] patman: Fix implicit command inserting Douglas Anderson
2022-07-07 15:23 ` [PATCH v3 2/5] patman: Don't look at sys.argv when parsing settings Douglas Anderson
@ 2022-07-07 15:23 ` Douglas Anderson
2022-07-16 11:55 ` Simon Glass
2022-07-07 15:23 ` [PATCH v3 4/5] patman: By default don't pass "--no-tree" to checkpatch for linux Douglas Anderson
` (4 subsequent siblings)
7 siblings, 1 reply; 12+ messages in thread
From: Douglas Anderson @ 2022-07-07 15:23 UTC (permalink / raw)
To: sjg; +Cc: amstan, mka, Sean Anderson, briannorris, Douglas Anderson, u-boot
For boolean arguments it's convenient to be able to specify both the
argument and its opposite on the command line. This is especially
convenient because you can change the default via the settings file
and being able express the opposite can be the only way to override
things.
Luckily python handles this well--we just need to specify things with
BooleanOptionalAction. We'll do that for all options except
"full-help" (where it feels silly). This uglifies the help text a
little bit but does give maximum flexibility.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
(no changes since v2)
Changes in v2:
- Fix doc string for --ignore-bad-tags
tools/patman/main.py | 55 ++++++++++++++++++++++----------------------
1 file changed, 28 insertions(+), 27 deletions(-)
diff --git a/tools/patman/main.py b/tools/patman/main.py
index e868878ad5a7..56dcde1f9ef3 100755
--- a/tools/patman/main.py
+++ b/tools/patman/main.py
@@ -6,7 +6,7 @@
"""See README for more information"""
-from argparse import ArgumentParser
+from argparse import ArgumentParser, BooleanOptionalAction
import os
import re
import shutil
@@ -39,7 +39,7 @@ parser.add_argument('-c', '--count', dest='count', type=int,
default=-1, help='Automatically create patches from top n commits')
parser.add_argument('-e', '--end', type=int, default=0,
help='Commits to skip at end of patch list')
-parser.add_argument('-D', '--debug', action='store_true',
+parser.add_argument('-D', '--debug', action=BooleanOptionalAction,
help='Enabling debugging (provides a full traceback on error)')
parser.add_argument('-p', '--project', default=project.detect_project(),
help="Project name; affects default option values and "
@@ -49,42 +49,43 @@ parser.add_argument('-P', '--patchwork-url',
help='URL of patchwork server [default: %(default)s]')
parser.add_argument('-s', '--start', dest='start', type=int,
default=0, help='Commit to start creating patches from (0 = HEAD)')
-parser.add_argument('-v', '--verbose', action='store_true', dest='verbose',
- default=False, help='Verbose output of errors and warnings')
+parser.add_argument('-v', '--verbose', action=BooleanOptionalAction,
+ dest='verbose', default=False,
+ help='Verbose output of errors and warnings')
parser.add_argument('-H', '--full-help', action='store_true', dest='full_help',
default=False, help='Display the README file')
subparsers = parser.add_subparsers(dest='cmd')
send = subparsers.add_parser('send')
-send.add_argument('-i', '--ignore-errors', action='store_true',
- dest='ignore_errors', default=False,
- help='Send patches email even if patch errors are found')
+send.add_argument('-i', '--ignore-errors', action=BooleanOptionalAction,
+ dest='ignore_errors', default=False,
+ help='Send patches email even if patch errors are found')
send.add_argument('-l', '--limit-cc', dest='limit', type=int, default=None,
help='Limit the cc list to LIMIT entries [default: %(default)s]')
-send.add_argument('-m', '--no-maintainers', action='store_false',
- dest='add_maintainers', default=True,
- help="Don't cc the file maintainers automatically")
-send.add_argument('-n', '--dry-run', action='store_true', dest='dry_run',
- default=False, help="Do a dry run (create but don't email patches)")
+send.add_argument('-m', '--maintainers', action=BooleanOptionalAction,
+ dest='add_maintainers', default=True,
+ help="cc the file maintainers automatically")
+send.add_argument('-n', '--dry-run', action=BooleanOptionalAction,
+ dest='dry_run', default=False,
+ help="Do a dry run (create but don't email patches)")
send.add_argument('-r', '--in-reply-to', type=str, action='store',
help="Message ID that this series is in reply to")
-send.add_argument('-t', '--ignore-bad-tags', action='store_true',
- default=False,
- help='Ignore bad tags / aliases (default=warn)')
-send.add_argument('-T', '--thread', action='store_true', dest='thread',
+send.add_argument('-t', '--ignore-bad-tags', action=BooleanOptionalAction,
+ default=False, help='Ignore bad tags / aliases')
+send.add_argument('-T', '--thread', action=BooleanOptionalAction, dest='thread',
default=False, help='Create patches as a single thread')
send.add_argument('--cc-cmd', dest='cc_cmd', type=str, action='store',
default=None, help='Output cc list for patch file (used by git)')
-send.add_argument('--no-binary', action='store_true', dest='ignore_binary',
- default=False,
- help="Do not output contents of changes in binary files")
-send.add_argument('--no-check', action='store_false', dest='check_patch',
+send.add_argument('--binary', action=BooleanOptionalAction,
+ dest='ignore_binary', default=False,
+ help="Output contents of changes in binary files")
+send.add_argument('--check', action=BooleanOptionalAction, dest='check_patch',
default=True,
- help="Don't check for patch compliance")
-send.add_argument('--no-tags', action='store_false', dest='process_tags',
- default=True, help="Don't process subject tags as aliases")
-send.add_argument('--no-signoff', action='store_false', dest='add_signoff',
- default=True, help="Don't add Signed-off-by to patches")
+ help="Check for patch compliance")
+send.add_argument('--tags', action=BooleanOptionalAction, dest='process_tags',
+ default=True, help="Process subject tags as aliases")
+send.add_argument('--signoff', action=BooleanOptionalAction, dest='add_signoff',
+ default=True, help="Add Signed-off-by to patches")
send.add_argument('--smtp-server', type=str,
help="Specify the SMTP server to 'git send-email'")
@@ -96,11 +97,11 @@ test_parser.add_argument('testname', type=str, default=None, nargs='?',
status = subparsers.add_parser('status',
help='Check status of patches in patchwork')
-status.add_argument('-C', '--show-comments', action='store_true',
+status.add_argument('-C', '--show-comments', action=BooleanOptionalAction,
help='Show comments from each patch')
status.add_argument('-d', '--dest-branch', type=str,
help='Name of branch to create with collected responses')
-status.add_argument('-f', '--force', action='store_true',
+status.add_argument('-f', '--force', action=BooleanOptionalAction,
help='Force overwriting an existing branch')
# Parse options twice: first to get the project and second to handle
--
2.37.0.rc0.161.g10f37bed90-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 4/5] patman: By default don't pass "--no-tree" to checkpatch for linux
2022-07-07 15:23 [PATCH v3 0/5] patman: Small fixes plus remove --no-tree from checkpatch for linux Douglas Anderson
` (2 preceding siblings ...)
2022-07-07 15:23 ` [PATCH v3 3/5] patman: Make most bool arguments BooleanOptionalAction Douglas Anderson
@ 2022-07-07 15:23 ` Douglas Anderson
2022-07-07 15:23 ` [PATCH v3 5/5] patman: Take project defaults into account for --help Douglas Anderson
` (3 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Douglas Anderson @ 2022-07-07 15:23 UTC (permalink / raw)
To: sjg; +Cc: amstan, mka, Sean Anderson, briannorris, Douglas Anderson, u-boot
When you pass "--no-tree" to checkpatch it disables some extra checks
that are important for Linux. Specifically I want checks like:
warning: DT compatible string "boogie,woogie" appears un-documented
check ./Documentation/devicetree/bindings/
Let's make the default for Linux to _not_ pass --no-tree. We'll have a
config option and command line flag to override.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
(no changes since v1)
tools/patman/checkpatch.py | 11 +++++++----
tools/patman/control.py | 7 ++++---
tools/patman/main.py | 3 +++
tools/patman/settings.py | 3 ++-
4 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/tools/patman/checkpatch.py b/tools/patman/checkpatch.py
index 70ba561c2686..d1b902dd9627 100644
--- a/tools/patman/checkpatch.py
+++ b/tools/patman/checkpatch.py
@@ -186,7 +186,7 @@ def check_patch_parse(checkpatch_output, verbose=False):
return result
-def check_patch(fname, verbose=False, show_types=False):
+def check_patch(fname, verbose=False, show_types=False, use_tree=False):
"""Run checkpatch.pl on a file and parse the results.
Args:
@@ -194,6 +194,7 @@ def check_patch(fname, verbose=False, show_types=False):
verbose: True to print out every line of the checkpatch output as it is
parsed
show_types: Tell checkpatch to show the type (number) of each message
+ use_tree (bool): If False we'll pass '--no-tree' to checkpatch.
Returns:
namedtuple containing:
@@ -210,7 +211,9 @@ def check_patch(fname, verbose=False, show_types=False):
stdout: Full output of checkpatch
"""
chk = find_check_patch()
- args = [chk, '--no-tree']
+ args = [chk]
+ if not use_tree:
+ args.append('--no-tree')
if show_types:
args.append('--show-types')
output = command.output(*args, fname, raise_on_error=False)
@@ -236,13 +239,13 @@ def get_warning_msg(col, msg_type, fname, line, msg):
line_str = '' if line is None else '%d' % line
return '%s:%s: %s: %s\n' % (fname, line_str, msg_type, msg)
-def check_patches(verbose, args):
+def check_patches(verbose, args, use_tree):
'''Run the checkpatch.pl script on each patch'''
error_count, warning_count, check_count = 0, 0, 0
col = terminal.Color()
for fname in args:
- result = check_patch(fname, verbose)
+ result = check_patch(fname, verbose, use_tree=use_tree)
if not result.ok:
error_count += result.errors
warning_count += result.warnings
diff --git a/tools/patman/control.py b/tools/patman/control.py
index b40382388e07..bf426cf7bcf4 100644
--- a/tools/patman/control.py
+++ b/tools/patman/control.py
@@ -64,7 +64,7 @@ def prepare_patches(col, branch, count, start, end, ignore_binary, signoff):
patchstream.insert_cover_letter(cover_fname, series, to_do)
return series, cover_fname, patch_files
-def check_patches(series, patch_files, run_checkpatch, verbose):
+def check_patches(series, patch_files, run_checkpatch, verbose, use_tree):
"""Run some checks on a set of patches
This santiy-checks the patman tags like Series-version and runs the patches
@@ -77,6 +77,7 @@ def check_patches(series, patch_files, run_checkpatch, verbose):
run_checkpatch (bool): True to run checkpatch.pl
verbose (bool): True to print out every line of the checkpatch output as
it is parsed
+ use_tree (bool): If False we'll pass '--no-tree' to checkpatch.
Returns:
bool: True if the patches had no errors, False if they did
@@ -86,7 +87,7 @@ def check_patches(series, patch_files, run_checkpatch, verbose):
# Check the patches, and run them through 'git am' just to be sure
if run_checkpatch:
- ok = checkpatch.check_patches(verbose, patch_files)
+ ok = checkpatch.check_patches(verbose, patch_files, use_tree)
else:
ok = True
return ok
@@ -165,7 +166,7 @@ def send(args):
col, args.branch, args.count, args.start, args.end,
args.ignore_binary, args.add_signoff)
ok = check_patches(series, patch_files, args.check_patch,
- args.verbose)
+ args.verbose, args.check_patch_use_tree)
ok = ok and gitutil.check_suppress_cc_config()
diff --git a/tools/patman/main.py b/tools/patman/main.py
index 56dcde1f9ef3..468ebf814fbc 100755
--- a/tools/patman/main.py
+++ b/tools/patman/main.py
@@ -82,6 +82,9 @@ send.add_argument('--binary', action=BooleanOptionalAction,
send.add_argument('--check', action=BooleanOptionalAction, dest='check_patch',
default=True,
help="Check for patch compliance")
+send.add_argument('--tree', action=BooleanOptionalAction,
+ dest='check_patch_use_tree', default=False,
+ help="If False, pass '--no-tree' to checkpatch")
send.add_argument('--tags', action=BooleanOptionalAction, dest='process_tags',
default=True, help="Process subject tags as aliases")
send.add_argument('--signoff', action=BooleanOptionalAction, dest='add_signoff',
diff --git a/tools/patman/settings.py b/tools/patman/settings.py
index 6c5f6c8ed992..cee18bf01a65 100644
--- a/tools/patman/settings.py
+++ b/tools/patman/settings.py
@@ -23,6 +23,7 @@ _default_settings = {
"u-boot": {},
"linux": {
"process_tags": "False",
+ "check_patch_use_tree": "True",
},
"gcc": {
"process_tags": "False",
@@ -71,7 +72,7 @@ class _ProjectConfigParser(ConfigParser.SafeConfigParser):
>>> config = _ProjectConfigParser("linux")
>>> config.readfp(StringIO(sample_config))
>>> sorted((str(a), str(b)) for (a, b) in config.items("settings"))
- [('am_hero', 'True'), ('process_tags', 'False')]
+ [('am_hero', 'True'), ('check_patch_use_tree', 'True'), ('process_tags', 'False')]
# Check to make sure that settings works with unknown project.
>>> config = _ProjectConfigParser("unknown")
--
2.37.0.rc0.161.g10f37bed90-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 5/5] patman: Take project defaults into account for --help
2022-07-07 15:23 [PATCH v3 0/5] patman: Small fixes plus remove --no-tree from checkpatch for linux Douglas Anderson
` (3 preceding siblings ...)
2022-07-07 15:23 ` [PATCH v3 4/5] patman: By default don't pass "--no-tree" to checkpatch for linux Douglas Anderson
@ 2022-07-07 15:23 ` Douglas Anderson
2022-07-17 8:12 ` Simon Glass
` (2 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Douglas Anderson @ 2022-07-07 15:23 UTC (permalink / raw)
To: sjg; +Cc: amstan, mka, Sean Anderson, briannorris, Douglas Anderson, u-boot
I'd like it so that when you do "patman send --help" and you're using
Linux that it show it the proper defaults for Linux.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
(no changes since v2)
Changes in v2:
- Make comment about parsing three times less nonsensical.
tools/patman/main.py | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/tools/patman/main.py b/tools/patman/main.py
index 468ebf814fbc..ec70d9fa1439 100755
--- a/tools/patman/main.py
+++ b/tools/patman/main.py
@@ -107,14 +107,19 @@ status.add_argument('-d', '--dest-branch', type=str,
status.add_argument('-f', '--force', action=BooleanOptionalAction,
help='Force overwriting an existing branch')
-# Parse options twice: first to get the project and second to handle
-# defaults properly (which depends on project)
-# Use parse_known_args() in case 'cmd' is omitted
+# Parse options several times:
+# - First to get the project.
+# - Second to handle defaults properly (which depends on project). This
+# makes help display the right defaults.
+# - Finally after we have added an implicit command if necessary.
+#
+# Use parse_known_args() for the first two in case 'cmd' is omitted
+argv = [arg for arg in sys.argv[1:] if arg not in ('-h', '--help')]
+args, _ = parser.parse_known_args(argv)
argv = sys.argv[1:]
-args, rest = parser.parse_known_args(argv)
if hasattr(args, 'project'):
settings.Setup(gitutil, parser, args.project, '')
- args, rest = parser.parse_known_args(argv)
+args, rest = parser.parse_known_args(argv)
# If we have a command, it is safe to parse all arguments
if args.cmd:
--
2.37.0.rc0.161.g10f37bed90-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/5] patman: Make most bool arguments BooleanOptionalAction
2022-07-07 15:23 ` [PATCH v3 3/5] patman: Make most bool arguments BooleanOptionalAction Douglas Anderson
@ 2022-07-16 11:55 ` Simon Glass
2022-07-16 17:46 ` Doug Anderson
0 siblings, 1 reply; 12+ messages in thread
From: Simon Glass @ 2022-07-16 11:55 UTC (permalink / raw)
To: Douglas Anderson
Cc: Alexandru M Stan, Matthias Kaehlcke, Sean Anderson, Brian Norris,
U-Boot Mailing List
Hi Doug,
On Thu, 7 Jul 2022 at 09:26, Douglas Anderson <dianders@chromium.org> wrote:
>
> For boolean arguments it's convenient to be able to specify both the
> argument and its opposite on the command line. This is especially
> convenient because you can change the default via the settings file
> and being able express the opposite can be the only way to override
> things.
>
> Luckily python handles this well--we just need to specify things with
> BooleanOptionalAction. We'll do that for all options except
> "full-help" (where it feels silly). This uglifies the help text a
> little bit but does give maximum flexibility.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Fix doc string for --ignore-bad-tags
>
> tools/patman/main.py | 55 ++++++++++++++++++++++----------------------
> 1 file changed, 28 insertions(+), 27 deletions(-)
Sadly this patch triggers an error:
Traceback (most recent call last):
File "./tools/patman/patman", line 9, in <module>
from argparse import ArgumentParser, BooleanOptionalAction
ImportError: cannot import name 'BooleanOptionalAction' from
'argparse' (/usr/lib/python3.8/argparse.py)
I've dropped it and the next one. Can you please take a look?
Regards,
Simon
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/5] patman: Make most bool arguments BooleanOptionalAction
2022-07-16 11:55 ` Simon Glass
@ 2022-07-16 17:46 ` Doug Anderson
2022-07-17 8:17 ` Simon Glass
0 siblings, 1 reply; 12+ messages in thread
From: Doug Anderson @ 2022-07-16 17:46 UTC (permalink / raw)
To: Simon Glass
Cc: Alexandru M Stan, Matthias Kaehlcke, Sean Anderson, Brian Norris,
U-Boot Mailing List
Hi,
On Sat, Jul 16, 2022 at 4:55 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Doug,
>
> On Thu, 7 Jul 2022 at 09:26, Douglas Anderson <dianders@chromium.org> wrote:
> >
> > For boolean arguments it's convenient to be able to specify both the
> > argument and its opposite on the command line. This is especially
> > convenient because you can change the default via the settings file
> > and being able express the opposite can be the only way to override
> > things.
> >
> > Luckily python handles this well--we just need to specify things with
> > BooleanOptionalAction. We'll do that for all options except
> > "full-help" (where it feels silly). This uglifies the help text a
> > little bit but does give maximum flexibility.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > Tested-by: Brian Norris <briannorris@chromium.org>
> > Reviewed-by: Brian Norris <briannorris@chromium.org>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v2)
> >
> > Changes in v2:
> > - Fix doc string for --ignore-bad-tags
> >
> > tools/patman/main.py | 55 ++++++++++++++++++++++----------------------
> > 1 file changed, 28 insertions(+), 27 deletions(-)
>
> Sadly this patch triggers an error:
>
> Traceback (most recent call last):
> File "./tools/patman/patman", line 9, in <module>
> from argparse import ArgumentParser, BooleanOptionalAction
> ImportError: cannot import name 'BooleanOptionalAction' from
> 'argparse' (/usr/lib/python3.8/argparse.py)
>
>
> I've dropped it and the next one. Can you please take a look?
Ugh. I hadn't noticed that it was added in python 3.9. I guess we
don't want to require python 3.9+? Python 3.9 was released October
2020...
-Doug
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 5/5] patman: Take project defaults into account for --help
2022-07-07 15:23 [PATCH v3 0/5] patman: Small fixes plus remove --no-tree from checkpatch for linux Douglas Anderson
` (4 preceding siblings ...)
2022-07-07 15:23 ` [PATCH v3 5/5] patman: Take project defaults into account for --help Douglas Anderson
@ 2022-07-17 8:12 ` Simon Glass
2022-07-17 8:12 ` [PATCH v3 2/5] patman: Don't look at sys.argv when parsing settings Simon Glass
2022-07-17 8:12 ` [PATCH v3 1/5] patman: Fix implicit command inserting Simon Glass
7 siblings, 0 replies; 12+ messages in thread
From: Simon Glass @ 2022-07-17 8:12 UTC (permalink / raw)
To: Douglas Anderson; +Cc: amstan, mka, Sean Anderson, briannorris, u-boot, sjg
I'd like it so that when you do "patman send --help" and you're using
Linux that it show it the proper defaults for Linux.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
(no changes since v2)
Changes in v2:
- Make comment about parsing three times less nonsensical.
tools/patman/main.py | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
Applied to u-boot-dm, thanks!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/5] patman: Fix implicit command inserting
2022-07-07 15:23 [PATCH v3 0/5] patman: Small fixes plus remove --no-tree from checkpatch for linux Douglas Anderson
` (6 preceding siblings ...)
2022-07-17 8:12 ` [PATCH v3 2/5] patman: Don't look at sys.argv when parsing settings Simon Glass
@ 2022-07-17 8:12 ` Simon Glass
7 siblings, 0 replies; 12+ messages in thread
From: Simon Glass @ 2022-07-17 8:12 UTC (permalink / raw)
To: Douglas Anderson; +Cc: amstan, mka, Sean Anderson, briannorris, u-boot, sjg
The logic to insert an implicit command has always been a bit broken
but it was masked by another bug fixed in the patch ("patman: Don't
look at sys.argv when parsing settings"). Specifically, imagine that
you're just calling patman like this:
patman -c1
After the parse_known_args() command then the "-c1" will have been
parsed and we'll have no command. The "rest" variable will be an empty
list. Going into the logic you can see that nargs = 0. The implicit
insertion of send ideally would create an argument list of:
['-c1', 'send']
...but it doesn't because argv[:-0] is the same as argv[:0] and that's
an empty list.
Let's fix this little glitch.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
(no changes since v1)
tools/patman/main.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Applied to u-boot-dm, thanks!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/5] patman: Don't look at sys.argv when parsing settings
2022-07-07 15:23 [PATCH v3 0/5] patman: Small fixes plus remove --no-tree from checkpatch for linux Douglas Anderson
` (5 preceding siblings ...)
2022-07-17 8:12 ` Simon Glass
@ 2022-07-17 8:12 ` Simon Glass
2022-07-17 8:12 ` [PATCH v3 1/5] patman: Fix implicit command inserting Simon Glass
7 siblings, 0 replies; 12+ messages in thread
From: Simon Glass @ 2022-07-17 8:12 UTC (permalink / raw)
To: Douglas Anderson; +Cc: amstan, mka, Sean Anderson, briannorris, u-boot, sjg
If you call the parser and tell it to parse but don't pass arguments
in then it will default to looking at sys.argv. This isn't really what
was intended and seems to have some side effects. Let's not do it.
NOTE: to see some of the side effects, note that this patch breaks
"patman -c1" if you don't have the patch ("patman: Fix implicit
command inserting") before it.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
Changes in v3:
- Rebased atop ("patman: Fix defaults not propagating to subparsers")
tools/patman/settings.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Applied to u-boot-dm, thanks!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/5] patman: Make most bool arguments BooleanOptionalAction
2022-07-16 17:46 ` Doug Anderson
@ 2022-07-17 8:17 ` Simon Glass
0 siblings, 0 replies; 12+ messages in thread
From: Simon Glass @ 2022-07-17 8:17 UTC (permalink / raw)
To: Doug Anderson
Cc: Alexandru M Stan, Matthias Kaehlcke, Sean Anderson, Brian Norris,
U-Boot Mailing List
Hi Doug,
On Sat, 16 Jul 2022 at 11:47, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Sat, Jul 16, 2022 at 4:55 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Doug,
> >
> > On Thu, 7 Jul 2022 at 09:26, Douglas Anderson <dianders@chromium.org> wrote:
> > >
> > > For boolean arguments it's convenient to be able to specify both the
> > > argument and its opposite on the command line. This is especially
> > > convenient because you can change the default via the settings file
> > > and being able express the opposite can be the only way to override
> > > things.
> > >
> > > Luckily python handles this well--we just need to specify things with
> > > BooleanOptionalAction. We'll do that for all options except
> > > "full-help" (where it feels silly). This uglifies the help text a
> > > little bit but does give maximum flexibility.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > Tested-by: Brian Norris <briannorris@chromium.org>
> > > Reviewed-by: Brian Norris <briannorris@chromium.org>
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > (no changes since v2)
> > >
> > > Changes in v2:
> > > - Fix doc string for --ignore-bad-tags
> > >
> > > tools/patman/main.py | 55 ++++++++++++++++++++++----------------------
> > > 1 file changed, 28 insertions(+), 27 deletions(-)
> >
> > Sadly this patch triggers an error:
> >
> > Traceback (most recent call last):
> > File "./tools/patman/patman", line 9, in <module>
> > from argparse import ArgumentParser, BooleanOptionalAction
> > ImportError: cannot import name 'BooleanOptionalAction' from
> > 'argparse' (/usr/lib/python3.8/argparse.py)
> >
> >
> > I've dropped it and the next one. Can you please take a look?
>
> Ugh. I hadn't noticed that it was added in python 3.9. I guess we
> don't want to require python 3.9+? Python 3.9 was released October
> 2020...
Not yet! It might be possible to do a little compatibility thing. We
do sometimes have code that does things the old way and the new way.
But it might be too intrusive in this case?
Regards,
Simon
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-07-17 8:17 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-07 15:23 [PATCH v3 0/5] patman: Small fixes plus remove --no-tree from checkpatch for linux Douglas Anderson
2022-07-07 15:23 ` [PATCH v3 1/5] patman: Fix implicit command inserting Douglas Anderson
2022-07-07 15:23 ` [PATCH v3 2/5] patman: Don't look at sys.argv when parsing settings Douglas Anderson
2022-07-07 15:23 ` [PATCH v3 3/5] patman: Make most bool arguments BooleanOptionalAction Douglas Anderson
2022-07-16 11:55 ` Simon Glass
2022-07-16 17:46 ` Doug Anderson
2022-07-17 8:17 ` Simon Glass
2022-07-07 15:23 ` [PATCH v3 4/5] patman: By default don't pass "--no-tree" to checkpatch for linux Douglas Anderson
2022-07-07 15:23 ` [PATCH v3 5/5] patman: Take project defaults into account for --help Douglas Anderson
2022-07-17 8:12 ` Simon Glass
2022-07-17 8:12 ` [PATCH v3 2/5] patman: Don't look at sys.argv when parsing settings Simon Glass
2022-07-17 8:12 ` [PATCH v3 1/5] patman: Fix implicit command inserting Simon Glass
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).