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
apis: west.manifest.Manifest.projects should not include the manifest itself #327
Comments
what kind of issues are observed with the current behavior. Cause I remember there were long discussion back and forth on how to treat the manifest project. Note, I'm not advocating against changing it, just would like to see a list of issues experienced so that everyone looking at this issue know why to change API. |
Welcome back :). The manifest repository is a repository for sure, and its Python representation should have some Project attributes (like west_commands, etc.), but it's not truly a Project:
So by "not truly a Project", I mean this in a Liskov substitution principle sense. Right now we hack it by setting these fields to None, but I've noticed again and again we've messed up updating the docstrings or code in ManifestProject following changes to Project, leading to errors, which is also a bad sign. From personal experience, I also find myself writing downstream code (nrf/scripts/ncs_west_commands/ncs_commands.py) that wants to iterate over projects and ending up special-casing the manifest repository as a result of crashes on code that naively looped over them. The manifest repository doesn't really have a 'name' in the project sense, and trying to treat it as if it were a project has led to weird edge cases. I think it was a worthy attempt and what we did made sense at the time, but experience has shown it was not a good idea in retrospect. |
For example, |
Thanks, hope I can still keep a little up to date on
I agree that it doesn't contain all the project attributes. Take a look at: #243 Will those kinds of projects be "true projects" in current sense of true project. We should avoid ending in a situation where changing this API results in treating other projects types individually, such as those proposed in #243 .
Should we consider to be able to return an iterator for projects meeting a special criteria.
|
and such issues can happen regardless of what we do - and should have been fixed a long time ago :( |
I'm very glad!
Good point. In this case I think as long as the non-git In fact, making the change here (#327) may lead us to add another section besides That definition for what a "project" is was how I modeled a project in the multimanifest work I am doing and I think it's nice and general for hg, svn, p5, etc. Clone depth we copied from google repo, and various issues (e.g. #307, #319) show it's not thought through properly.
Right, but by requiring explicit inclusion of the manifest repository by having to say So I believe making this change means the API is easier to use correctly if |
and in some cases the manifest project is ALSO a Zephyr module, and it would be bad to have |
For sure, west list should continue to contain the manifest repository. It's just the internal API I want to change. |
Change the url, revision, and clone_depth properties to plain old attributes, changing url from None to the empty string. This will be used to make it possible to type annotate Project in a subsequent commit. This seems like even more evidence that ManifestProject is not a Project in a subtyping sense, should not have been treated like one, and should be removed (zephyrproject-rtos#327). Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Change the url, revision, and clone_depth properties to plain old attributes, changing url from None to the empty string. This will be used to make it possible to type annotate Project in a subsequent commit. This seems like even more evidence that ManifestProject is not a Project in a subtyping sense, should not have been treated like one, and should be removed (#327). Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Clean up the file based on what zephyr's pylintrc would have complained about. There's one remaining issue, which is that ManifestProject doesn't call Project.__init__(), but that should be fixed with zephyrproject-rtos#327 instead. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Clean up the file based on what zephyr's pylintrc would have complained about. There's one remaining issue, which is that ManifestProject doesn't call Project.__init__(), but that should be fixed with #327 instead. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This is an API break. We are still allowed to do that until west 1.0. The west.manifest.Manifest class's path handling has become unmaintainable and we need to overhaul it. The from_data() and from_file() constructors are too clever and try to allow too many use cases, which results in the guts of the manifest construction code often not being able to tell whether it's in a real workspace or not. This makes it difficult to know when we can safely read configuration files or not. That in turn makes it difficult to accurately distinguish between the 'manifest.path' value in the local configuration file and the 'self: path:' value in the manifest data, especially when we can take keyword arguments that say "hey, pretend this is your path". Additionally, there are a few assumptions in the code that pretend that the manifest file itself always lives in the top level directory of the manifest repository. That was a valid assumption up to the point the 'manifest.file' configuration variable was introduced, but it's no longer valid anymore, since 'manifest.file' can point to a subdirectory of the manifest repository. This leaves us with some hacks to find the git repository we live in that shouldn't be necessary and which we can now remove with this overhaul. Rework the whole thing so that we can correctly handle the following situations: - when running in a workspace, you should now use from_file() or a newly introduced from_topdir() to make your Manifest - when running outside of any workspace, use from_data() From now on, we forbid creating a manifest from data "as if" it were in a real workspace. If all you have is data, the abspath attributes will all be None, reflecting reality. Accordingly, rework the Manifest.__init__() method to either take a topdir or a bit of data, but not both. Additionally, now that we have both manifest.path and manifest.file configuration options, it's not usually the right thing to ask for a Manifest from a *file*: what you really usually want is a Manifest from a *workspace*, and for that you just need a topdir. From the topdir, you can create a west.configuration.Configuration, and from there you can read manifest.path and manifest.file to get the complete path to the top level manifest within its repository. For that reason, we remove the source_file keyword argument from __init__() in favor of topdir and a new 'config' argument. For backwards compatibility, it's still allowed to use from_file() to load another manifest file than the one pointed to by topdir/manifest.path/manifest.file. However, this handling code is now just a bit of special case trickery in from_file(), and it also introduces a restriction that the other file is still in a git repository in the workspace. This moves potential sources for error or confusion out of Manifest.__init__() and the functions it calls. (This has proven to be a useful feature in practice; I am aware of west extensions that use it to compare the parsed contents of two different manifest files within the same workspace.) To migrate away from the now-outdated from_file(), introduce a new from_topdir() factory method and use it throughout the tree. This is clearer and more accurate code, which always avoids the hacks left behind for compatibility in from_file(). Finally, add various new instance attributes to the Manifest class that will let us tell the YAML path from the actual path, and the path to the manifest file from the path to the manifest repository in the workspace. These in turn let us remove some hacky code from the 'west list' implementation, and allow us to rework our internals so that ManifestProject is just a legacy remnant at this point, further helping along with zephyrproject-rtos#327. Keep tests up to date, removing obsolete code as needed and updating cases to reflect API breakage. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This is an API break. We are still allowed to do that until west 1.0. The west.manifest.Manifest class's path handling has become unmaintainable and we need to overhaul it. The from_data() and from_file() constructors are too clever and try to allow too many use cases, which results in the guts of the manifest construction code often not being able to tell whether it's in a real workspace or not. This makes it difficult to know when we can safely read configuration files or not. That in turn makes it difficult to accurately distinguish between the 'manifest.path' value in the local configuration file and the 'self: path:' value in the manifest data, especially when we can take keyword arguments that say "hey, pretend this is your path". Additionally, there are a few assumptions in the code that pretend that the manifest file itself always lives in the top level directory of the manifest repository. That was a valid assumption up to the point the 'manifest.file' configuration variable was introduced, but it's no longer valid anymore, since 'manifest.file' can point to a subdirectory of the manifest repository. This leaves us with some hacks to find the git repository we live in that shouldn't be necessary and which we can now remove with this overhaul. Rework the whole thing so that we can correctly handle the following situations: - when running in a workspace, you should now use from_file() or a newly introduced from_topdir() to make your Manifest - when running outside of any workspace, use from_data() From now on, we forbid creating a manifest from data "as if" it were in a real workspace. If all you have is data, the abspath attributes will all be None, reflecting reality. Accordingly, rework the Manifest.__init__() method to either take a topdir or a bit of data, but not both. Additionally, now that we have both manifest.path and manifest.file configuration options, it's not usually the right thing to ask for a Manifest from a *file*: what you really usually want is a Manifest from a *workspace*, and for that you just need a topdir. From the topdir, you can create a west.configuration.Configuration, and from there you can read manifest.path and manifest.file to get the complete path to the top level manifest within its repository. For that reason, we remove the source_file keyword argument from __init__() in favor of topdir and a new 'config' argument. For backwards compatibility, it's still allowed to use from_file() to load another manifest file than the one pointed to by topdir/manifest.path/manifest.file. However, this handling code is now just a bit of special case trickery in from_file(), and it also introduces a restriction that the other file is still in a git repository in the workspace. This moves potential sources for error or confusion out of Manifest.__init__() and the functions it calls. (This has proven to be a useful feature in practice; I am aware of west extensions that use it to compare the parsed contents of two different manifest files within the same workspace.) To migrate away from the now-outdated from_file(), introduce a new from_topdir() factory method and use it throughout the tree. This is clearer and more accurate code, which always avoids the hacks left behind for compatibility in from_file(). Finally, add various new instance attributes to the Manifest class that will let us tell the YAML path from the actual path, and the path to the manifest file from the path to the manifest repository in the workspace. These in turn let us remove some hacky code from the 'west list' implementation, and allow us to rework our internals so that ManifestProject is just a legacy remnant at this point, further helping along with zephyrproject-rtos#327. Keep tests up to date, removing obsolete code as needed and updating cases to reflect API breakage. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This is an API break. We are still allowed to do that until west 1.0. The west.manifest.Manifest class's path handling has become unmaintainable and we need to overhaul it. The from_data() and from_file() constructors are too clever and try to allow too many use cases, which results in the guts of the manifest construction code often not being able to tell whether it's in a real workspace or not. This makes it difficult to know when we can safely read configuration files or not. That in turn makes it difficult to accurately distinguish between the 'manifest.path' value in the local configuration file and the 'self: path:' value in the manifest data, especially when we can take keyword arguments that say "hey, pretend this is your path". Additionally, there are a few assumptions in the code that pretend that the manifest file itself always lives in the top level directory of the manifest repository. That was a valid assumption up to the point the 'manifest.file' configuration variable was introduced, but it's no longer valid anymore, since 'manifest.file' can point to a subdirectory of the manifest repository. This leaves us with some hacks to find the git repository we live in that shouldn't be necessary and which we can now remove with this overhaul. Rework the whole thing so that we can correctly handle the following situations: - when running in a workspace, you should now use from_file() or a newly introduced from_topdir() to make your Manifest - when running outside of any workspace, use from_data() From now on, we forbid creating a manifest from data "as if" it were in a real workspace. If all you have is data, the abspath attributes will all be None, reflecting reality. Accordingly, rework the Manifest.__init__() method to either take a topdir or a bit of data, but not both. Additionally, now that we have both manifest.path and manifest.file configuration options, it's not usually the right thing to ask for a Manifest from a *file*: what you really usually want is a Manifest from a *workspace*, and for that you just need a topdir. From the topdir, you can create a west.configuration.Configuration, and from there you can read manifest.path and manifest.file to get the complete path to the top level manifest within its repository. For that reason, we remove the source_file keyword argument from __init__() in favor of topdir and a new 'config' argument. For backwards compatibility, it's still allowed to use from_file() to load another manifest file than the one pointed to by topdir/manifest.path/manifest.file. However, this handling code is now just a bit of special case trickery in from_file(), and it also introduces a restriction that the other file is still in a git repository in the workspace. This moves potential sources for error or confusion out of Manifest.__init__() and the functions it calls. (This has proven to be a useful feature in practice; I am aware of west extensions that use it to compare the parsed contents of two different manifest files within the same workspace.) To migrate away from the now-outdated from_file(), introduce a new from_topdir() factory method and use it throughout the tree. This is clearer and more accurate code, which always avoids the hacks left behind for compatibility in from_file(). Finally, add various new instance attributes to the Manifest class that will let us tell the YAML path from the actual path, and the path to the manifest file from the path to the manifest repository in the workspace. These in turn let us remove some hacky code from the 'west list' implementation, and allow us to rework our internals so that ManifestProject is just a legacy remnant at this point, further helping along with zephyrproject-rtos#327. Keep tests up to date, removing obsolete code as needed and updating cases to reflect API breakage. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This is an API break. We are still allowed to do that until west 1.0. The west.manifest.Manifest class's path handling has become unmaintainable and we need to overhaul it. The from_data() and from_file() constructors are too clever and try to allow too many use cases, which results in the guts of the manifest construction code often not being able to tell whether it's in a real workspace or not. This makes it difficult to know when we can safely read configuration files or not. That in turn makes it difficult to accurately distinguish between the 'manifest.path' value in the local configuration file and the 'self: path:' value in the manifest data, especially when we can take keyword arguments that say "hey, pretend this is your path". Additionally, there are a few assumptions in the code that pretend that the manifest file itself always lives in the top level directory of the manifest repository. That was a valid assumption up to the point the 'manifest.file' configuration variable was introduced, but it's no longer valid anymore, since 'manifest.file' can point to a subdirectory of the manifest repository. This leaves us with some hacks to find the git repository we live in that shouldn't be necessary and which we can now remove with this overhaul. Rework the whole thing so that we can correctly handle the following situations: - when running in a workspace, you should now use from_file() or a newly introduced from_topdir() to make your Manifest - when running outside of any workspace, use from_data() From now on, we forbid creating a manifest from data "as if" it were in a real workspace. If all you have is data, the abspath attributes will all be None, reflecting reality. Accordingly, rework the Manifest.__init__() method to either take a topdir or a bit of data, but not both. Additionally, now that we have both manifest.path and manifest.file configuration options, it's not usually the right thing to ask for a Manifest from a *file*: what you really usually want is a Manifest from a *workspace*, and for that you just need a topdir. From the topdir, you can create a west.configuration.Configuration, and from there you can read manifest.path and manifest.file to get the complete path to the top level manifest within its repository. For that reason, we remove the source_file keyword argument from __init__() in favor of topdir and a new 'config' argument. For backwards compatibility, it's still allowed to use from_file() to load another manifest file than the one pointed to by topdir/manifest.path/manifest.file. However, this handling code is now just a bit of special case trickery in from_file(), and it also introduces a restriction that the other file is still in a git repository in the workspace. This moves potential sources for error or confusion out of Manifest.__init__() and the functions it calls. (This has proven to be a useful feature in practice; I am aware of west extensions that use it to compare the parsed contents of two different manifest files within the same workspace.) To migrate away from the now-outdated from_file(), introduce a new from_topdir() factory method and use it throughout the tree. This is clearer and more accurate code, which always avoids the hacks left behind for compatibility in from_file(). Finally, add various new instance attributes to the Manifest class that will let us tell the YAML path from the actual path, and the path to the manifest file from the path to the manifest repository in the workspace. These in turn let us remove some hacky code from the 'west list' implementation, and allow us to rework our internals so that ManifestProject is just a legacy remnant at this point, further helping along with zephyrproject-rtos#327. Keep tests up to date, removing obsolete code as needed and updating cases to reflect API breakage. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This is an API break. We are still allowed to do that until west 1.0. The west.manifest.Manifest class's path handling has become unmaintainable and we need to overhaul it. The from_data() and from_file() constructors are too clever and try to allow too many use cases, which results in the guts of the manifest construction code often not being able to tell whether it's in a real workspace or not. This makes it difficult to know when we can safely read configuration files or not. That in turn makes it difficult to accurately distinguish between the 'manifest.path' value in the local configuration file and the 'self: path:' value in the manifest data, especially when we can take keyword arguments that say "hey, pretend this is your path". Additionally, there are a few assumptions in the code that pretend that the manifest file itself always lives in the top level directory of the manifest repository. That was a valid assumption up to the point the 'manifest.file' configuration variable was introduced, but it's no longer valid anymore, since 'manifest.file' can point to a subdirectory of the manifest repository. This leaves us with some hacks to find the git repository we live in that shouldn't be necessary and which we can now remove with this overhaul. Rework the whole thing so that we can correctly handle the following situations: - when running in a workspace, you should now use from_file() or a newly introduced from_topdir() to make your Manifest - when running outside of any workspace, use from_data() From now on, we forbid creating a manifest from data "as if" it were in a real workspace. If all you have is data, the abspath attributes will all be None, reflecting reality. Accordingly, rework the Manifest.__init__() method to either take a topdir or a bit of data, but not both. Additionally, now that we have both manifest.path and manifest.file configuration options, it's not usually the right thing to ask for a Manifest from a *file*: what you really usually want is a Manifest from a *workspace*, and for that you just need a topdir. From the topdir, you can create a west.configuration.Configuration, and from there you can read manifest.path and manifest.file to get the complete path to the top level manifest within its repository. For that reason, we remove the source_file keyword argument from __init__() in favor of topdir and a new 'config' argument. For backwards compatibility, it's still allowed to use from_file() to load another manifest file than the one pointed to by topdir/manifest.path/manifest.file. However, this handling code is now just a bit of special case trickery in from_file(), and it also introduces a restriction that the other file is still in a git repository in the workspace. This moves potential sources for error or confusion out of Manifest.__init__() and the functions it calls. (This has proven to be a useful feature in practice; I am aware of west extensions that use it to compare the parsed contents of two different manifest files within the same workspace.) To migrate away from the now-outdated from_file(), introduce a new from_topdir() factory method and use it throughout the tree. This is clearer and more accurate code, which always avoids the hacks left behind for compatibility in from_file(). Finally, add various new instance attributes to the Manifest class that will let us tell the YAML path from the actual path, and the path to the manifest file from the path to the manifest repository in the workspace. These in turn let us remove some hacky code from the 'west list' implementation, and allow us to rework our internals so that ManifestProject is just a legacy remnant at this point, further helping along with zephyrproject-rtos#327. Keep tests up to date, removing obsolete code as needed and updating cases to reflect API breakage. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This is an API break. We are still allowed to do that until west 1.0. The west.manifest.Manifest class's path handling has become unmaintainable and we need to overhaul it. The from_data() and from_file() constructors are too clever and try to allow too many use cases, which results in the guts of the manifest construction code often not being able to tell whether it's in a real workspace or not. This makes it difficult to know when we can safely read configuration files or not. That in turn makes it difficult to accurately distinguish between the 'manifest.path' value in the local configuration file and the 'self: path:' value in the manifest data, especially when we can take keyword arguments that say "hey, pretend this is your path". Additionally, there are a few assumptions in the code that pretend that the manifest file itself always lives in the top level directory of the manifest repository. That was a valid assumption up to the point the 'manifest.file' configuration variable was introduced, but it's no longer valid anymore, since 'manifest.file' can point to a subdirectory of the manifest repository. This leaves us with some hacks to find the git repository we live in that shouldn't be necessary and which we can now remove with this overhaul. Rework the whole thing so that we can correctly handle the following situations: - when running in a workspace, you should now use from_file() or a newly introduced from_topdir() to make your Manifest - when running outside of any workspace, use from_data() From now on, we forbid creating a manifest from data "as if" it were in a real workspace. If all you have is data, the abspath attributes will all be None, reflecting reality. Accordingly, rework the Manifest.__init__() method to either take a topdir or a bit of data, but not both. Additionally, now that we have both manifest.path and manifest.file configuration options, it's not usually the right thing to ask for a Manifest from a *file*: what you really usually want is a Manifest from a *workspace*, and for that you just need a topdir. From the topdir, you can create a west.configuration.Configuration, and from there you can read manifest.path and manifest.file to get the complete path to the top level manifest within its repository. For that reason, we remove the source_file keyword argument from __init__() in favor of topdir and a new 'config' argument. For backwards compatibility, it's still allowed to use from_file() to load another manifest file than the one pointed to by topdir/manifest.path/manifest.file. However, this handling code is now just a bit of special case trickery in from_file(), and it also introduces a restriction that the other file is still in a git repository in the workspace. This moves potential sources for error or confusion out of Manifest.__init__() and the functions it calls. (This has proven to be a useful feature in practice; I am aware of west extensions that use it to compare the parsed contents of two different manifest files within the same workspace.) To migrate away from the now-outdated from_file(), introduce a new from_topdir() factory method and use it throughout the tree. This is clearer and more accurate code, which always avoids the hacks left behind for compatibility in from_file(). Finally, add various new instance attributes to the Manifest class that will let us tell the YAML path from the actual path, and the path to the manifest file from the path to the manifest repository in the workspace. These in turn let us remove some hacky code from the 'west list' implementation, and allow us to rework our internals so that ManifestProject is just a legacy remnant at this point, further helping along with zephyrproject-rtos#327. Keep tests up to date, removing obsolete code as needed and updating cases to reflect API breakage. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This is an API break. We are still allowed to do that until west 1.0. The west.manifest.Manifest class's path handling has become unmaintainable and we need to overhaul it. The from_data() and from_file() constructors are too clever and try to allow too many use cases, which results in the guts of the manifest construction code often not being able to tell whether it's in a real workspace or not. This makes it difficult to know when we can safely read configuration files or not. That in turn makes it difficult to accurately distinguish between the 'manifest.path' value in the local configuration file and the 'self: path:' value in the manifest data, especially when we can take keyword arguments that say "hey, pretend this is your path". Additionally, there are a few assumptions in the code that pretend that the manifest file itself always lives in the top level directory of the manifest repository. That was a valid assumption up to the point the 'manifest.file' configuration variable was introduced, but it's no longer valid anymore, since 'manifest.file' can point to a subdirectory of the manifest repository. This leaves us with some hacks to find the git repository we live in that shouldn't be necessary and which we can now remove with this overhaul. Rework the whole thing so that we can correctly handle the following situations: - when running in a workspace, you should now use from_file() or a newly introduced from_topdir() to make your Manifest - when running outside of any workspace, use from_data() From now on, we forbid creating a manifest from data "as if" it were in a real workspace. If all you have is data, the abspath attributes will all be None, reflecting reality. Accordingly, rework the Manifest.__init__() method to either take a topdir or a bit of data, but not both. Additionally, now that we have both manifest.path and manifest.file configuration options, it's not usually the right thing to ask for a Manifest from a *file*: what you really usually want is a Manifest from a *workspace*, and for that you just need a topdir. From the topdir, you can create a west.configuration.Configuration, and from there you can read manifest.path and manifest.file to get the complete path to the top level manifest within its repository. For that reason, we remove the source_file keyword argument from __init__() in favor of topdir and a new 'config' argument. For backwards compatibility, it's still allowed to use from_file() to load another manifest file than the one pointed to by topdir/manifest.path/manifest.file. However, this handling code is now just a bit of special case trickery in from_file(), and it also introduces a restriction that the other file is still in a git repository in the workspace. This moves potential sources for error or confusion out of Manifest.__init__() and the functions it calls. (This has proven to be a useful feature in practice; I am aware of west extensions that use it to compare the parsed contents of two different manifest files within the same workspace.) To migrate away from the now-outdated from_file(), introduce a new from_topdir() factory method and use it throughout the tree. This is clearer and more accurate code, which always avoids the hacks left behind for compatibility in from_file(). Finally, add various new instance attributes to the Manifest class that will let us tell the YAML path from the actual path, and the path to the manifest file from the path to the manifest repository in the workspace. These in turn let us remove some hacky code from the 'west list' implementation, and allow us to rework our internals so that ManifestProject is just a legacy remnant at this point, further helping along with #327. Keep tests up to date, removing obsolete code as needed and updating cases to reflect API breakage. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Trying to treat the manifest repository as an ordinary Project was doomed to failure; they're simply too different. We should instead move the valid state that's in ManifestProject today to Manifest attributes, removing MANIFEST_PROJECT_INDEX and the fact that projects[MANIFEST_PROJECT_INDEX] contains the manifest.
This will require a bit of thought into how
west list
should behave, though. Maybe we need to resurrectwest list --all
to include manifest project data like{path}
and{west_commands}
? And perhaps that should abort if youwest list --all -f {url}
or try to access some other attribute that's not defined for the manifest repository.The text was updated successfully, but these errors were encountered: