Skip to content

manifest: Support --active-only argument for --resolve/--freeze#808

Merged
carlescufi merged 2 commits into
zephyrproject-rtos:mainfrom
pdgendt:freeze-resolve-active
May 6, 2025
Merged

manifest: Support --active-only argument for --resolve/--freeze#808
carlescufi merged 2 commits into
zephyrproject-rtos:mainfrom
pdgendt:freeze-resolve-active

Conversation

@pdgendt
Copy link
Copy Markdown
Collaborator

@pdgendt pdgendt commented Apr 17, 2025

EDIT: The discussions in #491 and #667 made it clear that --freeze and --resolve should not have their behavior changed.

Updated the PR to introduce --active-only for --resolve/--freeze.

Running west manifest --freeze expects projects to be cloned, but the purpose of inactive projects is to not clone these explicitly.

Currently it's not possible to call west manifest --freeze on a clean zephyr installment because of the optional projects.

@pdgendt pdgendt force-pushed the freeze-resolve-active branch from 4359950 to a9c4e84 Compare April 17, 2025 06:58
@EricNRS
Copy link
Copy Markdown

EricNRS commented Apr 17, 2025

Thank you @pdgendt. This fixes both of the issues I was seeing where:

  1. west manifest --freeze would throw an exception on the first uncloned repo which was canopennode in my case
  2. west manifest --resolve would include repos that were not cloned in the list

Error before pulling in the change:

$ west manifest --freeze
. . .
RuntimeError: cannot freeze; project canopennode is uncloned

With this change, both are correctly reporting cloned repos without any errors. Discussion in
https://discord.com/channels/720317445772017664/733037524498382881/1362257185597554808

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.06%. Comparing base (07aa937) to head (9b8107d).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #808      +/-   ##
==========================================
+ Coverage   83.99%   84.06%   +0.07%     
==========================================
  Files          11       11              
  Lines        3342     3345       +3     
==========================================
+ Hits         2807     2812       +5     
+ Misses        535      533       -2     
Files with missing lines Coverage Δ
src/west/app/project.py 79.66% <100.00%> (+0.27%) ⬆️
src/west/manifest.py 94.97% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

marc-hb

This comment was marked as outdated.

@pdgendt pdgendt force-pushed the freeze-resolve-active branch from 9f8bd6a to c749dda Compare April 17, 2025 16:42
marc-hb

This comment was marked as resolved.

@pdgendt
Copy link
Copy Markdown
Collaborator Author

pdgendt commented Apr 17, 2025

LGTM

I spent more than 2s looking at the code and I take that back, sorry.

_as_dict_helper() already has an optional pdict argument which is already used to perform filtering. So, this PR scatters the project filtering in two different places; one "contextual" one and another, new, more "hardcoded" place. Does not look great. Would something like this make more sense? (UNTESTED)

I don't think that would work? The pdict isn't filtering? It would throw.

@marc-hb
Copy link
Copy Markdown
Collaborator

marc-hb commented Apr 17, 2025

I don't think that would work? The pdict isn't filtering? It would throw.

Yes my bad, ignore. Sorry for the noise.

@marc-hb
Copy link
Copy Markdown
Collaborator

marc-hb commented Apr 17, 2025

This was already discussed in #491, #667 and probably others. At the time, a new command was suggested instead of changing the semantic of the existing one[*]. Please review past discussions before proceeding with this.

Discussion in
https://discord.com/channels/720317445772017664/733037524498382881/1362257185597554808

Discord is useful but Discord discussions are not public, please avoid using as a reference (try opening this links in private/incognito mode)

[*] Maybe one issue is: different people with different filters get different --freeze outputs? So it's not "deterministic" anymore? I don't know, I don't have time to read all past discussions right now sorry.

@marc-hb
Copy link
Copy Markdown
Collaborator

marc-hb commented Apr 17, 2025

west manifest --freeze would throw an exception on the first uncloned repo

So this never worked in a default or almost default configuration? Whatever happens, it sounds like more test coverage is needed! CodeCov & friends only know so much...

@EricNRS
Copy link
Copy Markdown

EricNRS commented Apr 17, 2025

Here is the original bug report from Discord that prompted this change.

Bug report from the Discord discussion

Any tips on how to get west manifest --freeze to work? When I run it, it complains about the projects in zephyr/submanifests/optional.yaml not being cloned starting with the first in the list of canopennode. Since it is optional software that I am not using, it is not cloned, so the entire freeze operation fails.

I am using the "forest" model of west and have a zephyrproject/applications folder which imports the zephyr repo:

manifest:
  remotes:
    - name: zephyrproject-rtos
      url-base: https://github.com/zephyrproject-rtos

  projects:
    - name: zephyr
      revision: v4.0-mainline-dev
      import: true
    # private application repo configurations go here

$ west manifest --freeze
. . .
RuntimeError: cannot freeze; project canopennode is uncloned

I was able to update west to skip uncloned projects to get a release out. Would still like to know if this is a west issue or a usage/configuration issue on my part.

@pdgendt
Copy link
Copy Markdown
Collaborator Author

pdgendt commented Apr 18, 2025

This was already discussed in #491, #667 and probably others. At the time, a new command was suggested instead of changing the semantic of the existing one[*]. Please review past discussions before proceeding with this.

Thanks for pointing to those discussions, will update this PR to have --freeze-active and --resolve-active instead.

@pdgendt pdgendt force-pushed the freeze-resolve-active branch from c749dda to a5dd277 Compare April 18, 2025 07:01
@pdgendt pdgendt changed the title manifest: freeze/resolve active projects only manifest: Support --freeze-active and --resolve-active arguments Apr 18, 2025
@pdgendt pdgendt force-pushed the freeze-resolve-active branch 4 times, most recently from 14aa628 to b3ee9d0 Compare April 18, 2025 07:49
@marc-hb
Copy link
Copy Markdown
Collaborator

marc-hb commented Apr 23, 2025

I see nothing wrong but I was never on top of filters and Partial imports Incomplete or changing imports are much more complicated than you think , so at least one of the @mbolivar must review. Ideally, everyone who discussed #666 and others should review too.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces two new command-line arguments—--freeze-active and --resolve-active—for generating a manifest that includes only active projects, leaving the original --freeze and --resolve behaviors unchanged.

  • Added new test cases in tests/test_project.py to verify the behavior of active-only manifests.
  • Extended the manifest module to support an "active_only" parameter in its as_dict, as_frozen_dict, and YAML output functions.
  • Updated the CLI parser and run logic in src/west/app/project.py to recognize and process the new active commands.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
tests/test_project.py Introduced tests for --freeze-active and --resolve-active behavior to exclude inactive projects.
src/west/manifest.py Added an "active_only" parameter to as_dict, as_frozen_dict, as_yaml, and related helpers for filtering inactive projects.
src/west/app/project.py Extended argument parsing and command routing to support --freeze-active and --resolve-active options.

@pdgendt pdgendt added this to the v1.4.0 milestone Apr 30, 2025
@carlescufi
Copy link
Copy Markdown
Member

@pdgendt Overall this looks great, but just a question from myself. Why not have something like west manifest --resolve --active and west manifest --freeze --active?

@carlescufi carlescufi requested a review from marc-hb April 30, 2025 13:08
@pdgendt
Copy link
Copy Markdown
Collaborator Author

pdgendt commented Apr 30, 2025

@pdgendt Overall this looks great, but just a question from myself. Why not have something like west manifest --resolve --active and west manifest --freeze --active?

No particular reason, both options are probably valid, I took the comment from here.

@carlescufi
Copy link
Copy Markdown
Member

@pdgendt Overall this looks great, but just a question from myself. Why not have something like west manifest --resolve --active and west manifest --freeze --active?

No particular reason, both options are probably valid, I took the comment from here.

I see, I personally would rather see a new --active-only flag that can be combined with --resolve and --freeze, as well as help text pointing to this new --active-only when the resolving or freezing fails. But I won't block the PR for this. CC @mbolivar @marc-hb

@pdgendt pdgendt force-pushed the freeze-resolve-active branch from b3ee9d0 to cdee9f2 Compare April 30, 2025 13:29
@pdgendt pdgendt changed the title manifest: Support --freeze-active and --resolve-active arguments manifest: Support --active-only argument for --resolve/--freeze Apr 30, 2025
@pdgendt pdgendt requested a review from Copilot April 30, 2025 14:20
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces the new "--active-only" argument for the manifest commands used in both resolve and freeze operations. Key changes include:

  • Adding new test cases to verify that inactive projects are correctly filtered using the active-only flag.
  • Updating manifest representation methods (as_dict, as_frozen_dict, as_yaml) to accept an optional active_only parameter that applies a project filter.
  • Modifying the project command argument parsing to conditionally disable extra validations when active-only is enabled.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
tests/test_project.py Added tests to verify behavior with the new active-only flag.
src/west/manifest.py Integrated the 'active_only' parameter into manifest methods.
src/west/app/project.py Adjusted command handling for resolve and freeze with active-only.

Comment thread tests/test_project.py
Comment thread src/west/manifest.py
Comment thread src/west/app/project.py
pdgendt added 2 commits May 2, 2025 12:03
Running manifest --freeze or --resolve expects projects to be cloned,
add variants that allow for only active projects to be resolved or
frozen.

Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
Add test cases for commands
- manifest --freeze --active-only
- manifest --resolve
- manifest --resolve --active-only

Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
@pdgendt pdgendt force-pushed the freeze-resolve-active branch from cdee9f2 to 9b8107d Compare May 2, 2025 10:04
Copy link
Copy Markdown
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

Looks great!

@carlescufi carlescufi merged commit 2f49b74 into zephyrproject-rtos:main May 6, 2025
27 checks passed
@pdgendt pdgendt deleted the freeze-resolve-active branch May 6, 2025 08:41
@EricNRS
Copy link
Copy Markdown

EricNRS commented May 6, 2025

Thank you @pdgendt for pushing this through and taking care of several old issues!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support west manifest --resolve and --freeze with manifest.project-filter west manifest --freeze fails since it doesn't ignore excluded groups

6 participants