Skip to content
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

Allow userdata to be defined under self in manifest repository #567

Conversation

RyanLindemanCAE
Copy link
Contributor

The purpose of this PR is to allow for specifying userdata in the manifest repository. One of the primary reasons I am making the switch from the Google repo tool to the Zephyr west tool is to allow for specifying the firmware version information using the userdata capability in the manifest file. I also wanted to be able to combine my manifest repository with my project repository. I soon discovered that userdata is allowed for projects but excluded for no seemingly good reason for the manifest repository.

@RyanLindemanCAE
Copy link
Contributor Author

Let me know if I need to bump the schema version number. I wasn't sure that I needed to since I thought version '0.10' was the last official version. I can see that it was bumped recently so I'm thinking I might be able to slip this change in without bumping it again.

@mbolivar-nordic
Copy link
Contributor

Yes, this needs a new schema version. We only bump the schema version when we make backwards-incompatible changes, which is not every release.

@RyanLindemanCAE
Copy link
Contributor Author

Done. Please let me know if I did it wrong.

@RyanLindemanCAE
Copy link
Contributor Author

@mbolivar-nordic - Any suggestions on how to get this PR moving towards a merge?
Thanks!

@RyanLindemanCAE
Copy link
Contributor Author

@mbolivar-nordic - Another ping? Any way I can get this moving?

@mbolivar-nordic
Copy link
Contributor

@RyanLindemanCAE sorry, #572 was kind of a hot potato that I had to deal with. Could you please rebase on top of that now that I've merged #571, which fixed #572, resolving the conflicts? I'll take another look and merge once that's done.

@RyanLindemanCAE RyanLindemanCAE force-pushed the add-userdata-to-manifest-project branch from 68ce8cb to b03476e Compare March 27, 2022 02:09
@RyanLindemanCAE RyanLindemanCAE force-pushed the add-userdata-to-manifest-project branch from b03476e to f06add5 Compare March 27, 2022 03:14
@RyanLindemanCAE
Copy link
Contributor Author

@mbolivar-nordic I have done as you asked, please review my changes.

@RyanLindemanCAE
Copy link
Contributor Author

It would be nice to find a way to "merge" the userdata values found in lower level manifest files similar to how you merge west_commands but I wasn't sure how to handle that since userdata can be either a string or key/value pair.
What do you think?

@RyanLindemanCAE
Copy link
Contributor Author

@mbolivar-nordic - The more I think about this, the more I think we need a slightly better solution then what is currently implemented. For example, if project A (the main manifest) has userdata defined under self and includes project B (using self.imports) which also has userdata defined under self then how would someone access both of these userdata definitions?
One thought is to have manifest.userdata be a dictionary where the key is the (relative?) path to the manifest file and the self.userdata in that file. Then project A and B could be distinguished using their relative manifest path as the key but this seems different than the project based userdata. Do you have any suggestions or ideas?

Copy link
Contributor

@mbolivar-nordic mbolivar-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 so much for the update. It looks like you meant to add a userdata attribute or property to the Manifest class itself, rather than just saving it in the ManifestProject. I have no objection to adding it to ManifestProject if it is useful to you, but having an attribute in the Manifest class makes good sense to me as the "recommended" place to get this, since that will be a nice long-term place to get this information after we successfully phase out the ManifestProject class (see #327). Did you forget to add it?

src/west/manifest.py Show resolved Hide resolved
…oving ManifestProject class eventually and strengthen related tests
@RyanLindemanCAE
Copy link
Contributor Author

Thanks so much for the update. It looks like you meant to add a userdata attribute or property to the Manifest class itself, rather than just saving it in the ManifestProject. I have no objection to adding it to ManifestProject if it is useful to you, but having an attribute in the Manifest class makes good sense to me as the "recommended" place to get this, since that will be a nice long-term place to get this information after we successfully phase out the ManifestProject class (see #327). Did you forget to add it?

When I freeze my manifest file as part of a release it uses the ManifestProject to get the "self" details for the frozen file. When you remove ManifestProject, I expect you will use the Manifest class to obtain the "self" details during a freeze operation. Hopefully that helps explain why I think userdata should be in ManifestProject too for now. It also makes it easier to iterate through all "projects" seeking out userdata information. Thoughts or comments are welcome.

@mbolivar-nordic mbolivar-nordic merged commit ca52ee5 into zephyrproject-rtos:main Apr 4, 2022
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.

None yet

2 participants