West commands tweaks #1749
West commands tweaks #1749
Conversation
This is going away in west 0.7. We can just use f-strings instead. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
No functional changes. I just find these more readable than str.format(). Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
|
All checks are passing now. Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages. |
mbolivar
force-pushed
the
west-commands-tweaks
branch
2 times, most recently
from
30447ab
to
602d659
Compare
Changes that are required to get through CI due to additional linting added since this file was originally merged. I needed to use a couple of "disable" comments where the linter is wrong. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar
force-pushed
the
west-commands-tweaks
branch
from
602d659
to
870b60d
Compare
|
CI is green; should be good to go |
| def check_west_version(): | ||
| min_ver = '0.6.99' | ||
| if (packaging.version.parse(west_ver) < | ||
| packaging.version.parse(min_ver)): | ||
| log.die('this command currently requires a pre-release west ' | ||
| f'(your west version is {west_ver}, ' | ||
| f'but must be at least {min_ver}') |
There was a problem hiding this comment.
Just a thought.
Should this be built into west itself, so that an extension command can simply state.
west_required_version and this way ensure that west is correct version.
I think this could be useful also for Zephyr, to ensure west is up to date, as well as anyone else doing extension commands.
There was a problem hiding this comment.
Yes, this could be added to core zephyr, but honestly this is generic code that is not really west-specific. west already exports its __version__.
There was a problem hiding this comment.
This is built into west... as of west's master branch :). So we can't start to rely on it until that is released. That's what the manifest schema version feature is about.
There was a problem hiding this comment.
I think @tejlmand meant the check_west_version() function. That is not built into west.
There was a problem hiding this comment.
It's true that the west version can move independently of the manifest schema version, but in practice users are only going to need a minimum version of west if either the manifest schema changes or the APIs change. In this case it's because of APIs. And I don't think we'll be updating APIs without also having a change to a manifest schema in practice, since it wouldn't be necessary, so I think practically this is the same thing.
But we can certainly add this to core west if we like.
There was a problem hiding this comment.
Nice.
Only reason for commenting here, was because this was where I spotted it, and wanted to hear opinions before rasing it as an enhancement issue.
| # TODO: does the fact that this is needed mean the west commit | ||
| # which names the manifest project 'manifest' unconditionally | ||
| # was wrong to do so? | ||
| if zp.name == 'manifest': |
There was a problem hiding this comment.
if all imported manifest project are named 'manifest', we could end up into some mess as all those are name manifest, although they are in different imported projects.
Is the reason for manifest name, that the toplevel project may not have self::path defined, and thus we cannot know the name (unless we simply use the folder name, if self::path is not set)
For imported projects, we could use the name specified in the manifest for the project being imported.
Should manifest be an attribute on a project, to indicate that it is an imported project and holds a manifest file itself ?
Each manifest project has a
There was a problem hiding this comment.
I don't follow here, we don't have a project named manifest in our manifest. Why is zp.name possibly set to manifest?
There was a problem hiding this comment.
Well, as you know, I want to get rid of ManifestProject entirely; see zephyrproject-rtos/west#327.
This is yet more evidence that having the manifest itself as an object inside manifest.projects is a confusing mistake in my opinion :).
But for now, we do, and it's always named "manifest", which is a reserved name that no project can possibly have. The manifest.py parsing code checks that and rejects any attempt to name a project 'manifest'.
Either way I think this is more of a question about west than this particular PR, right?
There was a problem hiding this comment.
Let's discuss this in west itself, since as @mbolivar describes, there are already plans to get rid of this project in memory as part of the project list.
There was a problem hiding this comment.
@carlescufi completely agree.
Was not aware that the project name manifest was actually a side effect / would be fixed by zephyrproject-rtos/west#327.
The first commit is necessary to keep this working with west's master branch.
The second one is just fiddling around.
The third one is needed so #1750 can be rebased on top and get a clean bill of health from pylint.