-
Notifications
You must be signed in to change notification settings - Fork 8k
scripts: flash: Add west config for flash skip rebuild #96115
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.
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
base: main
Are you sure you want to change the base?
scripts: flash: Add west config for flash skip rebuild #96115
Conversation
What is the use case? When would someone want to flash an outdated image? Or is this because the no-op build is too slow? It shouldn't be... EDIT: |
cef1c25
to
865161f
Compare
865161f
to
cb373e6
Compare
The skip rebuild option already exists. I am just adding a west config option for it so that someone doesn't have to type it every time if they generally don't want it to behave that way. So I don't think it's in the scope of the PR to debate the utility of that already-existing option. I can say though that my use case is that I often am having builds on multiple different SHAs due to I am trying to compare behaviors or something at different points in history, and using different build directory to store the builds so that they don't overwrite each other on the default path. And checking out to a different SHA causes the image to get rebuilt if it's from a different SHA, so this is really annoying because I have to keep doing git checkout just to flash/debug the proper image or else be typing |
cb373e6
to
3ec8406
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The skip rebuild option already exists.
I just noticed now... I had not yet because you're adding/duplicating the config
logic in a different place than the command-line flag logic. Please keep all that logic in the same place. You're also adding documentation for the config
far away from --skip-rebuild
help, is that necessary and why?
So I don't think it's in the scope of the PR to debate the utility of that already-existing option.
While less so, it can still be valid to revisit the value of a feature when changing it. Not every feature was a good idea and extending bad ones is not either.
Also, things evolve and what was good ages ago may not still be today. This particular feature was added 7 years ago in a giant commit and totally different environment and I couldn't find any rationale for it:
https://github.com/zephyrproject-rtos/west/commits/b1477f67d69d
EDIT: this was only the 6th west commit of all time!
West was in its very first year back then and in a "frantic" development phase. @mbolivar , any memories of --skip-rebuild
?
I can say though that my use case using different build directory to store the builds so that they don't overwrite each other on the default path.
Thanks for sharing! I'm still not sure what is the best way to deal with it but it does sound like a common enough use case.
I'm not sure where to put it then, I just did a similar thing to how config.get is used elsewhere in this file such as for
I am following the same style as the Also, the reason I even found this doc page to put it on at all, is because this page: https://docs.zephyrproject.org/latest/develop/west/config.html#built-in-configuration-options says that the doc of the west config options for the custom commands is on their specific page, so I looked up west flash on the doc site search, and this page seemed like the best result, since it had documented the west build configs also |
A big difference between
Forgetting that Finally, why not just this instead? --- a/scripts/west_commands/run_common.py
+++ b/scripts/west_commands/run_common.py
@@ -142,7 +142,7 @@ def add_parser_common(command, parser_adder=None, parser=None):
help=argparse.SUPPRESS)
group.add_argument('-r', '--runner',
help='override default runner from --build-dir')
- group.add_argument('--skip-rebuild', action='store_true',
+ group.add_argument('-s', '--skip-rebuild', action='store_true',
help='do not refresh cmake dependencies first')
group.add_argument('--domain', action='append',
help='execute runner only for given domain') It's not like this particular |
? if not user_args.skip_rebuild:
rebuild(command, build_dir, user_args) # Re-build unless asked not to, to make sure the output is up to date.
if build_dir and not args.skip_rebuild:
rebuild(command, build_dir, args)
I don't think
Fair enough... I don't have a strong opinion there but if not in the same place for some reason, then the documentations of |
scripts/west_commands/run_common.py
Outdated
log.die(f'no CMake cache found (expected one at {cache_file})') | ||
|
||
def rebuild(command, build_dir, args): | ||
skip_rebuild_config = config.get('flash', 'skip-rebuild', fallback='false') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against this proposal, but currently the flash.skip-rebuild
option would also be used for debug
/simulate
/robot
, which feels wrong.
It can be "fixed" with
skip_rebuild_config = config.get('flash', 'skip-rebuild', fallback='false') | |
skip_rebuild_config = config.get(command.name, 'skip-rebuild', fallback='false') |
But this also can become somewhat of a footgun.
An alternative (already existing) solution is to create an alias (can be set globally too):
$ west config alias.flash -- "flash --skip-rebuild"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I agree this should have better granularity.
That said, I think having the config option can make sense.
Personally I often use the west debugserver --skip-rebuild
because I don't want the debugserver to start messing with my build folder.
So principle is accepted.
but doesn't that go for any Or if setting some fixed CMake args using: A west config option will not be present in the config files unless the user as explicitly set it, which makes it the users responsibility. It's simple, don't use it if you don't like it. As for the So granularity should be improved, so that if being able to use |
scripts/west_commands/run_common.py
Outdated
log.die(f'no CMake cache found (expected one at {cache_file})') | ||
|
||
def rebuild(command, build_dir, args): | ||
skip_rebuild_config = config.get('flash', 'skip-rebuild', fallback='false') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also suggest to use getboolean
skip_rebuild_config = config.get('flash', 'skip-rebuild', fallback='false') | |
skip_rebuild_config = config.getboolean(command.name, 'skip-rebuild', fallback='false') |
Nice! @decsny can you please try this? Or something like this if you prefer not shadowing:
It depends: they don't all have the same cost when forgetting them; they are not all "footguns". Forgetting that you configured
Right, this one looks like a pretty big footgun. But unlike this PR, there is no command-line or other alternative for
Agreed, this is another footgun but the effect of this one is probably easier to spot in the build logs? Anyway, we can debate and compare footguns until the cows come home but already having an arsenal is not a good enough reason to add yet another, new footgun.
Footguns are never "simple", they is always a benefit/risk trade-off. For this particular one, the benefit seems extremely low. In every Zephyr survey I keep seeing this coming back: "make Zephyr more newbie-friendly". Also, @pdgendt already added the "ultimate swiss army footgun" a.k.a. PS: my "request for changes" is about the current implementation. |
This comment was marked as resolved.
This comment was marked as resolved.
This could be related to my use case that users want to keep multiple different builds and switch between them: In case of OP's use case it could make sense to extend |
I guess that depends on the user. Regarding the rebuild / no rebuild, then I would say it's just as simple to spot:
vs:
Though if approving the config setting,
But there is one thing we should have if supporting For example I can enable sysbuild by default, but deselect it on a specific build, like this:
So if we're going to support
Side-note: if introducing
Like anything else, I would say it depends. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though I do like the west config flash.skip-rebuild true
, then for such a setting it must be possible to overwrite the config setting directly on command line when invoking west flash
.
For example like this (I want my rebuild):
$ west flash --force-rebuild
Note, instead of implementing --force-rebuild
, then it's probably better to rename option so we have --rebuild
/ --no-rebuild
or similar.
See also west config flash.skip-rebuild true
I think |
I disagree, we should use We even have an extendable zephyr/scripts/west_commands/runners/core.py Lines 380 to 383 in 27fa721
|
I always need to re-read and double-check such double negations first. but then I prefer the |
I don't like double negations either and I like consistency too, unfortunately I don't have a strong opinion for those names, my previous comment was all about not renaming the "true" and "false" booleans to "skip" and "force". That's even more confusing that double negations because it looks like that "skip" and "force" could refer to a different type of "build". Double-negations are confusing and time-consuming but they are at least not ambiguous. |
Sorry, I never intended to propose
I think that's a fine thing, and if we want to cleanup later then we can start deprecating |
If I understand the convergence of the discussion was to introduce |
Consensus makes sense to me. For the record,
@marc-hb consider the use case "As a developer, I have to flash multiple instances of the same board with the same binary, and I want to ensure that there are no differences in the different binaries flashed. I have an old build directory I would like to use for this purpose. My working tree has diverged significantly from when I built it." |
@marc-hb although to be honest, the truth is that @SebastianBoe wanted us to rebuild by default, and he was the build system maintainer back then so I thought it best not to argue even though I disagreed. My personal preference would have been for the --skip-rebuild behavior to be the default. |
I'm actually not sure what you're referring here though because one of the things I did on the original commit here is refactor to try to put all the logic in the same place, since there was some duplication across the file, maybe you can be more specific of what you mean? |
3ec8406
to
35945f6
Compare
Add a west config option to skip rebuilds by default or not when doing west flash. Signed-off-by: Declan Snyder <[email protected]>
35945f6
to
98408b4
Compare
This comment was marked as spam.
This comment was marked as spam.
@decsny I think the idea is to make sure that |
Yes - and good luck reviewing and maintaining the correct precedence if the code for one were far away from the other.
I noticed that one of your very first versions https://github.com/zephyrproject-rtos/zephyr/blob/cef1c2527e0686f6dcf60174aa192c7fbae98d32/scripts/west_commands/run_common.py still had some "skip" code in two places (lines 250 and 730). You force-pushed and regrouped everything that while I was making that comment #96115 (review). I didn't check again and did not notice the change, apologies! |
ok, I dont remember if that was the case, I didnt even remember i pushed multiple times anyways, I updated the PR for the multiple command and symmetrical cli as requested, please check if the logic is how you imagined |
group.add_argument('-s', '--skip-rebuild', "--no-rebuild", action='store_true', | ||
help='do not refresh cmake dependencies first') | ||
group.add_argument('--force-rebuild', '--rebuild', action='store_true', | ||
help='force a refresh of the cmake dependencies before running') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think those should be in a mutual exclusive group.
group.add_argument('--skip-rebuild', action='store_true', | ||
group.add_argument('-s', '--skip-rebuild', "--no-rebuild", action='store_true', | ||
help='do not refresh cmake dependencies first') | ||
group.add_argument('--force-rebuild', '--rebuild', action='store_true', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the --force-rebuild
.
We should just have --rebuild
/ --no-rebuild
as the primary flags. And we're only keeping --skip-rebuild
for backwards compatibility reasons.
group.add_argument('--force-rebuild', '--rebuild', action='store_true', | |
group.add_argument('--rebuild', action='store_true', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need default=None
here, so that we can determine later if the user has specified the argument (True
/False
) or if it was not specified (None
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need default=None here,...
I think so too, otherwise how would we know when to consider/ignore the west config
? See below.
# Therefore, only two cases in which we skip: | ||
|
||
# user manually specified skip rebuild, so we skip | ||
if args.skip_rebuild: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small hint:
I’d suggest moving this line to the very top of the function. We can return directly (without considering the config at all) if the user explicitly requested --no-rebuild
.
def rebuild(command, build_dir, args): | ||
rebuild_config = config.get(command.name, 'rebuild', fallback='true').lower() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the documentation it says Boolean
, so I would expect getboolean
here?
rebuild_config = config.get(command.name, 'rebuild', fallback='true').lower() | ||
|
||
# Truth table -> | ||
# config rebuild | manual option | rebuild? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me the table is not so intuitive to understand.
Maybe it would be better to use the real variable names and list all combinations rebuild_config
(True
/ False
), args.skip_rebuild
(True
/ False
) and args.rebuild
with (True
/ False
/ None
).
# rebuild_config | args.skip_rebuild | args.rebuild | rebuild?
# ...
If the table is too long it can also be moved to the test (or the parameterized test cases represent this table), so this long comment does not need to be placed within the productive code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a truth table is not needed for this, it gives too much to think about.
AFAIK things should work like this:
- "--skip-rebuild" is deprecated. Could print a warning.
- using "--skip-rebuild" and any new option at the same time aborts with an error message. Eliminates "old vs new" precedence questions.
- "--skip-rebuild" must always have the exact same behavior as "--no-rebuild" (warning aside)
- As usual, the CLI has precedence. The
west config
is ignored when anything is passed on the command line (which is why the CLI default should probably be None).
1 and 2 have a error/warning message which is better than comments
4 is obvious.
3 should be explained in the --help.
So, no comment needed?
So, no truth table comment needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments are of course needed here.
But the full truth table (2x2x3=12 lines long) would be fine for me to live in the test (which is still missing). Just personal favor.
except FileNotFoundError: | ||
log.die(f'no CMake cache found (expected one at {cache_file})') | ||
|
||
def rebuild(command, build_dir, args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe some parameterized test for this function would make sense to test all combinations of args.rebuild
(True
,False
,None
), args.skip_rebuild
(True
/ False
) and config_rebuild
(True
/ False
/ None
) versus the truth table.
group.add_argument('--skip-rebuild', action='store_true', | ||
group.add_argument('-s', '--skip-rebuild', "--no-rebuild", action='store_true', | ||
help='do not refresh cmake dependencies first') | ||
group.add_argument('--force-rebuild', '--rebuild', action='store_true', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need default=None
here, so that we can determine later if the user has specified the argument (True
/False
) or if it was not specified (None
).
return | ||
|
||
# config says to skip, and user did not manually override, so skip | ||
if rebuild_config == 'false' and not args.force_rebuild: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be bool
.
group.add_argument('-r', '--runner', | ||
help='override default runner from --build-dir') | ||
group.add_argument('--skip-rebuild', action='store_true', | ||
group.add_argument('-s', '--skip-rebuild', "--no-rebuild", action='store_true', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use argparse.BooleanOptionalAction
and you get the negative --no-...
option for free, no need to declare both.
Add a west config option to skip rebuilds by default or not when doing west flash.