Skip to content

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+52 −4
Open

scripts: flash: Add west config for flash skip rebuild #96115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 35 additions & 0 deletions doc/develop/west/build-flash-debug.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,22 @@ only the image from this domain::

.. _west-debugging:

Configuration Options
=====================

You can :ref:`configure <west-config-cmd>` ``west flash`` using these options.

.. NOTE: docs authors: keep this table sorted alphabetically

.. list-table::
:widths: 10 30
:header-rows: 1

* - Option
- Description
* - ``flash.rebuild``
- Boolean, default ``true``. If ``false``, do not rebuild on west flash.

Debugging: ``west debug``, ``west debugserver``
***********************************************

Expand Down Expand Up @@ -683,6 +699,25 @@ to debug::

.. _west-runner:

Configuration Options
=====================

You can :ref:`configure <west-config-cmd>` ``west debug`` and
:ref:`configure <west-config-cmd>` ``west debugserver`` using these options.

.. NOTE: docs authors: keep this table sorted alphabetically

.. list-table::
:widths: 10 30
:header-rows: 1

* - Option
- Description
* - ``debug.rebuild``
- Boolean, default ``true``. If ``false``, do not rebuild on west debug.
* - ``debugserver.rebuild``
- Boolean, default ``true``. If ``false``, do not rebuild on west debugserver.

Flash and debug runners
***********************

Expand Down
21 changes: 17 additions & 4 deletions scripts/west_commands/run_common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,9 @@ def add_parser_common(command, parser_adder=None, parser=None):
group.add_argument('-r', '--runner',
help='override default runner from --build-dir')
group.add_argument('--skip-rebuild', action='store_true',
help='do not refresh cmake dependencies first')
help='(deprecated) do not refresh cmake dependencies first')
group.add_argument('--rebuild', action=argparse.BooleanOptionalAction,
help='manually specify to refresh cmake dependencies or not')
group.add_argument('--domain', action='append',
help='execute runner only for given domain')

Expand Down Expand Up @@ -244,8 +246,7 @@ def do_run_common(command, user_args, user_runner_args, domain_file=None):
)

build_dir = get_build_dir(user_args)
if not user_args.skip_rebuild:
rebuild(command, build_dir, user_args)
rebuild(command, build_dir, user_args)

domains = get_domains_to_process(build_dir, user_args, domain_file)

Expand Down Expand Up @@ -568,6 +569,18 @@ def load_cmake_cache(build_dir, args):
log.die(f'no CMake cache found (expected one at {cache_file})')

def rebuild(command, build_dir, args):
Copy link
Contributor

@thorsten-klein thorsten-klein Oct 2, 2025 •
edited
Loading

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.

if args.rebuild is False:
return

if args.skip_rebuild:
log.wrn("--skip-rebuild is deprecated. Please use --no-rebuild instead")
return

rebuild_config = config.getboolean(command.name, 'rebuild', fallback=True)

if not rebuild_config and not args.rebuild:
return
Comment on lines +581 to +582
Copy link
Contributor

@pdgendt pdgendt Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is a bit easier to read and understand (the config should only be taken into account if the command line argument isn't specified):

Suggested change
if not rebuild_config and not args.rebuild:
return
if args.rebuild is None and rebuild_config is False:
return

Copy link
Contributor

@marc-hb marc-hb Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For situations like this I find it even clearer to add some new rebuild_wanted() function. This gives more "early-return" possibilities which means fewer booleans and fewer conditionals and less mental load.

Something like this:

def rebuild_wanted():
    if args.rebuild is not None:
       return args.rebuild

    if rebuild_config is not None:
       return rebuild_config

    return True

def rebuild(command, build_dir, args):
    if not rebuild_wanted()
       return
    ...

Copy link
Member Author

@decsny decsny Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

"not" will be true if it is None or False. I'm not clear why you want to change from that.

Copy link
Contributor

Choose a reason for hiding this comment

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

They have the same result, but the way I suggested it, is how I would explain it in words:
The CLI argument needs to be omitted and the configuration option needs to be set to False

It might be just me, but I had to read your statement a few times more to understand.

Copy link
Contributor

@marc-hb marc-hb Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

"not" will be true if it is None or False. I'm not clear why you want to change from that.

Generally speaking you cannot treat None and False the same, they are not the same thing. None falls back to the lower precedence, False does not.
EDIT: so, any code that treats None and False the same forces the reader to wonder whether it's valid in this particular situation or not.

Copy link
Contributor

@thorsten-klein thorsten-klein Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

args.rebuild can be None (if not specified on command line) or False (if --no-rebuild was specified). Therefore I think using 'not args.rebuild' is correct here.

I would just change the order (but it is only personal favor):
if not args.rebuild and not config_rebuild:

Or even better what @marc-hb proposed: add a new method rebuild_wanted()


_banner(f'west {command.name}: rebuilding')
try:
zcmake.run_build(build_dir)
Expand Down Expand Up @@ -723,7 +736,7 @@ def dump_context(command, args, unknown_args):
get_all_domain = True

# Re-build unless asked not to, to make sure the output is up to date.
if build_dir and not args.skip_rebuild:
if build_dir:
rebuild(command, build_dir, args)

domains = get_domains_to_process(build_dir, args, None, get_all_domain)
Expand Down
Loading