Skip to content

[tools] Prevent trace-backs from incomplete args #2393

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 5, 2016
Merged

[tools] Prevent trace-backs from incomplete args #2393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a0c8a09
Unify look of argument errors
theotherjimmy Aug 4, 2016
163fa59
Prevent traceback
theotherjimmy Aug 4, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion tools/build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
from tools.build_api import static_analysis_scan, static_analysis_scan_lib, static_analysis_scan_library
from tools.build_api import print_build_results
from tools.settings import CPPCHECK_CMD, CPPCHECK_MSG_FORMAT
from utils import argparse_filestring_type
from utils import argparse_filestring_type, args_error
from tools.settings import CPPCHECK_CMD, CPPCHECK_MSG_FORMAT, CLI_COLOR_MAP
from utils import argparse_filestring_type, argparse_dir_not_parent

Expand Down Expand Up @@ -167,6 +167,9 @@
# Get toolchains list
toolchains = options.tool if options.tool else TOOLCHAINS

if options.source_dir and not options.build_dir:
args_error(parser, "argument --build is required by argument --source")

if options.color:
# This import happens late to prevent initializing colorization when we don't need it
import colorize
Expand Down
6 changes: 3 additions & 3 deletions tools/get_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
if __name__ == '__main__':
# Parse Options
parser = get_default_options_parser(add_clean=False, add_options=False)
parser.add_argument("--source", dest="source_dir", type=argparse_filestring_type,
parser.add_argument("--source", dest="source_dir", type=argparse_filestring_type, required=True,
default=[], help="The source (input) directory", action="append")
parser.add_argument("--prefix", dest="prefix", action="append",
default=[], help="Restrict listing to parameters that have this prefix")
Expand All @@ -48,12 +48,12 @@

# Target
if options.mcu is None :
args_error(parser, "[ERROR] You should specify an MCU")
args_error(parser, "argument -m/--mcu is required")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove [ERROR] ? wasn't that an intention warning/error/etc ? Does this PR clean all [ERROR] messages ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

args_error already prints an error message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

args_error already prints an error message.

Yes, that's obvious from the outputs you shared but it changes from [ERROR] to command: error:. I am not aware though how was that used (if there any tool that was parsing this or was just an unification of messages we print to have a format. That was my concern, looks fine to me as it is now ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that makes sense. Thanks for the clarification. I only changed things that were "argument errors".

target = options.mcu[0]

# Toolchain
if options.tool is None:
args_error(parser, "[ERROR] You should specify a TOOLCHAIN")
args_error(parser, "argument -t/--toolchain is required")
toolchain = options.tool[0]

options.prefix = options.prefix or [""]
Expand Down
10 changes: 8 additions & 2 deletions tools/make.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,14 +207,20 @@

# Target
if options.mcu is None :
args_error(parser, "[ERROR] You should specify an MCU")
args_error(parser, "argument -m/--mcu is required")
mcu = options.mcu[0]

# Toolchain
if options.tool is None:
args_error(parser, "[ERROR] You should specify a TOOLCHAIN")
args_error(parser, "argument -t/--tool is required")
toolchain = options.tool[0]

if (options.program is None) and (not options.source_dir):
args_error(parser, "one of -p, -n, or --source is required")

if options.source_dir and not options.build_dir:
args_error(parser, "argument --build is required when argument --source is provided")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this corrrect? Do we really require the user to specify a build dir if he specifies one or more source dirs? Feels a bit weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, We do. I agree, it does feel a bit weird. If you don't specify a build directory, you get a traceback about None not having startswith, I think.


if options.color:
# This import happens late to prevent initializing colorization when we don't need it
import colorize
Expand Down
12 changes: 11 additions & 1 deletion tools/project.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from tools.tests import test_known, test_name_known
from tools.targets import TARGET_NAMES
from tools.libraries import LIBRARIES
from utils import argparse_filestring_type, argparse_many
from utils import argparse_filestring_type, argparse_many, args_error
from utils import argparse_force_lowercase_type, argparse_force_uppercase_type, argparse_dir_not_parent
from project_api import setup_project, perform_export, print_results, get_lib_symbols

Expand Down Expand Up @@ -131,6 +131,16 @@

# source_dir = use relative paths, otherwise sources are copied
sources_relative = True if options.source_dir else False
# Target
if not options.mcu:
args_error(parser, "argument -m/--mcu is required")

# Toolchain
if not options.ide:
args_error(parser, "argument -i is required")

if (options.program is None) and (not options.source_dir):
args_error(parser, "one of -p, -n, or --source is required")

for mcu in options.mcu:
# Program Number or name
Expand Down
7 changes: 3 additions & 4 deletions tools/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,12 @@

# Target
if options.mcu is None :
args_error(parser, "[ERROR] You should specify an MCU")
args_error(parser, "argument -m/--mcu is required")
mcu = options.mcu[0]

# Toolchain
if options.tool is None:
args_error(parser, "[ERROR] You should specify a TOOLCHAIN")
args_error(parser, "argument -t/--tool is required")
toolchain = options.tool[0]

# Find all tests in the relevant paths
Expand Down Expand Up @@ -152,8 +152,7 @@
else:
# Build all tests
if not options.build_dir:
print "[ERROR] You must specify a build path"
sys.exit(1)
args_error(parser, "argument --build is required")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used return code 1 on exit, but args_error always uses 2. Not sure if this is actually a problem or not, but it might break some code that checks for the return code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used return code 1 on exit, but args_error always uses 2. Not sure if this is actually a problem or not, but it might break some code that checks the return code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering that we don't have a guide on return codes of the tools, They are on shaky ground if they check return codes.

I agree that technically this is a breaking change.


base_source_paths = options.source_dir

Expand Down
5 changes: 2 additions & 3 deletions tools/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,9 +282,8 @@ def args_error(parser, message):
parser - the ArgumentParser object that parsed the command line
message - what went wrong
"""
print "\n\n%s\n\n" % message
parser.print_help()
sys.exit()
parser.error(message)
sys.exit(2)


def construct_enum(**enums):
Expand Down