Generalize flag handling #1976
Generalize flag handling #1976
Conversation
theotherjimmy
force-pushed
the
memap-args
branch
from
89f83dd to
8e70b79
Compare
|
cc @mlnx |
|
@theotherjimmy thanks for looking into this. It looks good to me. |
theotherjimmy
force-pushed
the
memap-args
branch
from
724a14f to
d9f1028
Compare
|
Well, I did it anyway. Let's see if this breaks anything. |
theotherjimmy
changed the title
|
+1 from me, this is nice stuff |
|
+1 |
theotherjimmy
force-pushed
the
memap-args
branch
from
ad7abf4 to
33c050c
Compare
|
Working on more porting of argument parsing to the more liberal style. Targets:
|
| for i in sorted(self.modules): | ||
| for k in self.print_sections: | ||
| csv_module_section += [i+k] | ||
| csv_sizes += [self.modules[i][k]] |
There was a problem hiding this comment.
This look like pretty significant changes to memap.py? Should this be in another PR?
There was a problem hiding this comment.
Like I said, it only looks significant. I just moved each export format to it's own function and called them with a 'python case statement' (a dict lookup). I can move it to another PR if that's more appropriate.
There was a problem hiding this comment.
I just tried doing some builds from master and from this PR and it looks like the output was affected.
Master
Building project detect (RZ_A1H, ARM)
Compile: main.cpp
Compile: test_env.cpp
Link: detect
Elf2Bin: detect
+-----------+-------+-------+--------+
| Module | .text | .data | .bss |
+-----------+-------+-------+--------+
| Misc | 40479 | 246 | 626804 |
| Subtotals | 40479 | 246 | 626804 |
+-----------+-------+-------+--------+
Static RAM memory (data + bss): 627050
Heap: 0
Stack: 0
Total RAM memory (data + bss + heap + stack): 627050
Total Flash memory (text + data + misc): 40725
PR
Building project detect (RZ_A1H, ARM)
Compile: main.cpp
Compile: test_env.cpp
Link: detect
Elf2Bin: detect
+-----------+-------+-------+------+
| Module | .text | .data | .bss |
+-----------+-------+-------+------+
| Subtotals | 0 | 0 | 0 |
+-----------+-------+-------+------+
Static RAM memory (data + bss): 0
Heap: 0
Stack: 0
Total RAM memory (data + bss + heap + stack): 0
Total Flash memory (text + data + misc): 0
So even if it shouldn't change anything, it looks like it is. So perhaps this change should be in another PR?
theotherjimmy
force-pushed
the
memap-args
branch
from
dd252be to
5ccd550
Compare
|
At first glance, these changes look good, thanks @theotherjimmy. One thing that stands out to me is in I'd like to test this locally too, but so far it's looking good. |
|
outputs! and |
|
Better looking outputs: and and and |
|
Looks good. I'm not completely sold on the |
I'm curious, confusing for the user of the tools or the developer of the tools? |
|
cc @screamerbg |
|
|
||
| if __name__ == '__main__': | ||
| # Parse Options | ||
| parser = get_default_options_parser() | ||
| parser.add_option("-p", | ||
| type="int", | ||
| group = parser.add_mutually_exclusive_group(required=True) |
There was a problem hiding this comment.
This required causes an issue when the -L <--list-tests> option is specified:
$ python tools\make.py -L
usage: make.py [-h] [-m MCU] [-t TOOLCHAIN] [-c] [-o OPTIONS]
(-p PROGRAM | -n PROGRAM) [-j JOBS] [-v] [--silent] [-D MACROS]
[-S] [-f GENERAL_FILTER_REGEX] [--automated] [--host HOST_TEST]
[--extra EXTRA] [--peripherals PERIPHERALS]
[--dep DEPENDENCIES] [--source SOURCE_DIR]
[--duration DURATION] [--build BUILD_DIR] [-N ARTIFACT_NAME]
[-d DISK] [-s SERIAL] [-b BAUD] [-L] [--rtos] [--rpc] [--eth]
[--usb_host] [--usb] [--dsp] [--fat] [--ublox] [--testlib]
[-l LINKER_SCRIPT]
make.py: error: one of the arguments -p -n is required
There was a problem hiding this comment.
To add this comment, here are the valid combinations of arguments for make.py (the top level bullets are exclusive):
-L <--list-tests>- This should not require any other arguments to be set (ex.-m,-t, etc)-S <--supported-toolchains>- This should not require any other arguments to be set (ex.-m,-t, etc)- NOTE: This command also has historically accepted the
-f <--filter>option as well, so this capability should not be lost if at all possible.
- NOTE: This command also has historically accepted the
- One of the following commands (exclusively). All of the following should require all the build-related arguments (ex.
-m,-t, etc):- Exactly one instance of
-n - Exactly one instance of
-p - At least one instance of
--source(potentially more than one)
- Exactly one instance of
There was a problem hiding this comment.
requiring the -m and -t would break -S and -L with the current implementation. Really all of these mutually exclusive arguments should be subcommands.
|
One thing I noticed is if you don't supply |
| @@ -42,24 +42,24 @@ | |||
| # Parse Options | |||
| parser = get_default_options_parser() | |||
There was a problem hiding this comment.
This call is pulling in the following unneeded options: -m, -t, -c <--clean, and -o <--options>. Perhaps this should just be a new instantiation of argparse?
This should probably be in a separate PR though.
There was a problem hiding this comment.
Can you add a PR to fix this @bridadan ?
theotherjimmy
force-pushed
the
memap-args
branch
from
58d15bc to
3ff2f49
Compare
|
Instead of predicting what you meant and doing it for you, the parsers now predict and tell you what you should have typed. For example: or |
theotherjimmy
force-pushed
the
memap-args
branch
from
3ff2f49 to
7b3ef21
Compare
|
Why moving away from autocorrecting name casing? That change seemed like a great way to resolve naming issues between tools. |
Great! |
Too much magic. Perhaps solve the issue between tools by fixing the tools? |
|
And FYI, here is how git handles it It doesn't auto-correct it for you. It teaches you by showing you the right name, but you still have to type it. |
|
No I agree it would be magic to change arguments that are not character-equivalent. But being case insensitive isn't magic. There's no penalty to avoiding confusion between |
|
Maybe magic in mbed-cli in particular when the --build option is not specified to test adds a penalty. |
|
build is green; @sg- LGTM. |
|
I think I took it to far by saying that I hate SHIFT + anything!! |
|
Agree that case sensitivity makes much more sense than '-' and '', but then we should propagate across TARGET, TOOLCHAIN_ and FEATURE_ as well? |
|
I still think
At this point no for backwards compatibility. But the convention for macro names is all uppercase. |
affects all flags in:
All flags that accept a member of list of things now validate that the argument passed is in the list after converting the argument to the appropriate case and replacing "-" with "_" or vice versa, whichever is correct.
All flags that accept file paths (except for build directories) now validate that the file or directory exists.
for example:
and
python tools/build.py -t gcc-arm -m k64fis the same aspython tools/build.py -t GCC_ARM -m K64Fis the same aspython tools/bulid.py -t GcC-ArM -m K64f