Skip to content

[nrf noup] Use ZAP CLI for version check#600

Merged
ArekBalysNordic merged 1 commit into
nrfconnect:masterfrom
kendallgoto:patch-1
May 9, 2025
Merged

[nrf noup] Use ZAP CLI for version check#600
ArekBalysNordic merged 1 commit into
nrfconnect:masterfrom
kendallgoto:patch-1

Conversation

@kendallgoto
Copy link
Copy Markdown
Contributor

Both zap --version and zap-cli --version do the same thing more or less, but zap is heavier to run since it starts Electron just to do --version. Since the user may only care about zap-cli commands (e.g. west zap-generate), requiring that all west zap-* commands call zap just to get a version check increases the amount of resources and dependencies required.

Instead, just use zap-cli to get the version.

@kendallgoto kendallgoto requested a review from a team as a code owner April 24, 2025 00:42
@NordicBuilder
Copy link
Copy Markdown

none

Note: This comment is automatically posted and updated by the Contribs GitHub Action.

Copy link
Copy Markdown
Contributor

@Damian-Nordic Damian-Nordic left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I agree it makes sense.

@kendallgoto
Copy link
Copy Markdown
Contributor Author

Do I need to open a manifest PR in sdk-nrf as per the failing check or are you able to take it from here? Thanks!

@adigie
Copy link
Copy Markdown
Member

adigie commented Apr 25, 2025

Hi @kendallgoto, you have to sign Contributor License Agreement before we can merge this. Please force push to this PR (and #601) to trigger the CLAassistant.

Both `zap --version` and `zap-cli --version` do the same thing more or less, but `zap` is heavier to run since it starts Electron just to do `--version`. Since the user may only care about `zap-cli` commands (e.g. `west zap-generate`), requiring that all `west zap-*` commands call `zap` just to get a version check increases the amount of resources and dependencies required

Signed-off-by: Kendall Goto <kendall@level.co>
@ArekBalysNordic ArekBalysNordic merged commit ea99382 into nrfconnect:master May 9, 2025
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants