- 
                Notifications
    You must be signed in to change notification settings 
- Fork 8.2k
scripts: modules: support west group feature #31146
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
Conversation
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.
Looks fine, just one nit.
        
          
                scripts/zephyr_module.py
              
                Outdated
          
        
      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.
This is a bit hacky; you can compare the west.version.__version__ value against west 0.9. See _ensure_min_version() for examples of how to use packaging.version.parse() to make a high level version object which supports >= comparison.
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 know.
But that won't allow for fetching west source code and install manually for testing this feature, as that version would be 0.8.99.
This way allows for testing the group feature with Zephyr by installing west manually.
As soon as west 0.9.0 is released, then this code will be updated.
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.
As soon as west
0.9.0is released, then this code will be updated.
OK, so this is DNM for now until that happens, right? Adding the label.
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.
As soon as west
0.9.0is released, then this code will be updated.OK, so this is DNM for now until that happens, right? Adding the label.
I was actually thinking to have it in master for others to use, but I guess if people build west themselves, then they also know how to apply this patch to Zephyr.
Also, I see you are preparing the 0.9.0 release, #31241, which happened after I opened this PR, so DNM is fine.
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.
Finally got around to look at this again :)
West has introduced support for group tags in: zephyrproject-rtos/west#454 This means that manifest files might start containing groups. Zephyr itself only requires west>=0.7.2 where groups are not supported but other Zephyr based projects might start using the group feature. When using a west version with group support, then only active projects will be processed as Zephyr modules. West versions without group support will consider all projects active. Signed-off-by: Torsten Rasmussen <[email protected]>
b3a80db    to
    d0b3903      
    Compare
  
    | @nashif this should go in in order to properly support west 0.9.0 in Zephyr 2.5. | 
West has introduced support for group tags in:
zephyrproject-rtos/west#454
This means that manifest files might start containing groups.
Zephyr itself only requires west>=0.7.2 where groups are not supported
but other Zephyr based projects might start using the group feature.
When using a west version with group support, then only active projects
will be processed as Zephyr modules.
West versions without group support will consider all projects active.
Signed-off-by: Torsten Rasmussen [email protected]