-
Notifications
You must be signed in to change notification settings - Fork 137
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
REP153: adding metadata to the index file #181
Conversation
3a3cd9c
to
7d382d4
Compare
This looks like a reasonable decision to help reintegrate the rosdistro repositories 👍 |
rep-0153.rst
Outdated
distributions have been moved from the forked repository into the | ||
'ros/rosdistro' repository [5]_ the new field is being used to annotate the ROS | ||
2 distributions. | ||
Any client accessing trying to access the data with an old 'rosdistro' |
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.
"accessing trying to access" -> "accessing"
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.
Thanks for pointing it out. I ended up using "trying to access" in 10ab7e1 since the access wouldn't be successful.
The same fix will be necessary in older REPs (141 and 143) - I will change those separately.
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.
lgtm, one small suggestion on wording.
rep-0153.rst
Outdated
|
||
The intention is to annotate ROS version with their major version (``1`` or | ||
``2``) and avoid the need to externally define and update a classification of | ||
ROS distribution names as ROS 1 or ROS 2. |
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.
It seems to me that all that is needed to be specified here is the toolchain used for the given distro (e.g. catkin vs colcon?). It says so here ros2/ros_buildfarm_config#12 ("The ros_version is necessary to decide about the installation of the catkin or ros-workspace package without the need to hardcode ROS distro names in the ros_buildfarm code")
This could be added to the REP as an example for the usage, btw.
One can imagine the possibility of distributions from 2 ROS major versions sharing the same toolchain one day. And more easily one can imagine a situation where distributions from the same ROS major version use two different toolchains (such as rosbuild and catkin for ROS1, exagerrating the technical issues only slightly).
So merely storing the ROS major version as an integer seems limiting, whereas storing the toolchain name seems to solve the problem at hand sufficiently and transparently.
Even if you have other resons to want to store ROS version numbers in the ROS distribution files, you still have to justify why not using the full semantic versioning, or version strings (given whatever purpose might arise in the future might have to discriminate between minor versions). Integers make for poor identifiers.
Apart from that, using 'ros_' as a prefix for a key tightly binds rosdistro as a tool to the ROS ecosystem and the ROS brand, which is slightly against the philosophy of creating flexible open-source tools that can be re-used for various projects (however unlikely that may seem right now).
If you still persist that you really, really just need the ROS major version as an integer, at least name the key "major_version".
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.
I was curious about this as well. The language in the Discourse post seems to suggest that adding the version number is the only way to resolve the current situation:
For each ROS distribution the major ROS version should be annotated
It could be unfortunate phrasing, but was there a reason for not specifying the build infrastructure that should be used and instead relying on a version number to do that implicitly?
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.
Also when adding something to the distro files, the distribution.yaml should also be considered as useful location for additional data about distributions. Splitting the data concerning one distribution between index.yaml and distribution.yaml can cause further unnecessary headaches.
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.
It could be unfortunate phrasing, but was there a reason for not specifying the build infrastructure that should be used and instead relying on a version number to do that implicitly?
@gavanderhoorn The ros_version
does not imply a 1-to-1 mapping to a build tool. The major version of a distro is an "invariant". With it come differences in the distribution layout. E.g. the need to install the catkin
Debian package in order to have setup files under /opt/ros/<distro>
in ROS 1 or existence of a ros-<distro>-ros-workspace
package in RO2. Those are independent of the used build tool. The version information could also be used to distinguish / group the distros on the ROS index site](https://index.ros.org/packages/).
The choice of the build it not fixed for a distro. Each buildfarm can choose a build tool for a given distro - see e.g. ros-infrastructure/ros_buildfarm_config#117 which build ROS 1 Melodic with colcon
. Therefore the decision about the build tool belongs into the ros_buildfarm_config
rather then the rosdistro.
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.
Even if you have other resons to want to store ROS version numbers in the ROS distribution files, you still have to justify why not using the full semantic versioning, or version strings (given whatever purpose might arise in the future might have to discriminate between minor versions).
@tkruse "Semantic versioning" of ROS distros? That sounds bizarre to me since the definition of semver would imply that every distro must be a new major version (since at least something will be
a breaking change) which would make all other parts of the version irrelevant. Imo a versioning scheme across such a large number of federated packages doesn't make any sense. Please feel free to propose something if you think otherwise. Atm I don't foresee a use case not satisfied by a single number. If you have one in mind please bring it up and changing the type can certainly be considered.
using 'ros_' as a prefix for a key tightly binds rosdistro as a tool to the ROS ecosystem and the ROS brand, which is slightly against the philosophy of creating flexible open-source tools that can be re-used for various projects
...
... at least name the key "major_version".
I am not too worried about a "ros_" prefix in a database named rosdistro
which is accessed by a Python package named rosdistro
. The terminology of "major version" comes with a semantic meaning I am not sure is adequate in this case. Maybe there are other suggestions for naming the newly added key?
Splitting the data concerning one distribution between index.yaml and distribution.yaml can cause further unnecessary headaches.
On distribution entry in the index file can have N distribution files. Therefore the version information should go into the index file. Otherwise nothing would prevent you from having multiple contradicting distribution files for a single distro.
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.
I meant to have a version string that not merely indicates the "major" ros version, but also contains an increasing number for each distribution (such that Melodic "1.13" is known to be the next after Lunar "1.12", even when the alphabet runs out after 'Z'). This can be a version string/list, or just having separate major_version and minor_version keys.
The ros_version does not imply a 1-to-1 mapping to a build tool. The major version of a distro
is an "invariant". E.g. the need to install the catkin Debian package in order to have setup files
under /opt/ros/ in ROS 1 or existence of a ros--ros-workspace package in ROS2
Taking your example: with ROS2 Ardent and Bouncy, you need the existence of a ros--ros-workspace package. But maybe with a future ROS2 distribution you would need something else. By relying on a ros_version, you establish a 1-to-1 mapping between the major version and something. Instead, it would be cleaner if there was no logic like "if ros_version == 1 then install catkin debian package". Rather, that decision in the code should depend on a different discriminator, which can either be explicit, or a version key that denominates the version of the prerequisites.
Explicit woud be like "prerequisites: ['catkin_debian_package']", a version key would be like "distribution_format: 'ROS1-catkin'". Using the ROS major version for anything else than display purposes establishes 1-on-1 relationships.
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.
Note my concerns are not functionally urgent, so no need to have a longer discussion about it.
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.
Taking your example: with ROS2 Ardent and Bouncy, you need the existence of a ros--ros-workspace package. But maybe with a future ROS2 distribution you would need something else. By relying on a ros_version, you establish a 1-to-1 mapping between the major version and something.
We have handled similar case in the past by simply enumerating the names of distributions which need different handling. As long as that list is fixed and doesn't grow it is not a problem to hard code. The problem only arises when the list needs to be updated with every newly released distro.
Rather, that decision in the code should depend on a different discriminator
Without knowing what future conditions might need to decide on we can't foresee what atomic information to embed. For now we know we need to distinguish ROS 1 and ROS 2 distros and don't have any use case where the "major" version isn't sufficient.
have a version string that not merely indicates the "major" ros version, but also contains an increasing number for each distribution (such that Melodic "1.13" is known to be the next after Lunar "1.12", even when the alphabet runs out after 'Z').
A minor version as describes doesn't seem to provide significant value (beside replacing the alphabetic order constraint which will only be relevant in 10+ years).
Maybe an alternative would be to add another field for the release date (beside the "major" version). That would enable the same sorting as the proposed "minor" version but also allow to establish an chronological order between a ROS 1 and ROS 2 distro.
(I might drift towards feature creep now:) thinking about the release timeline even the timeframe of support could be valuable and/or if a distro is an LTS release or not. Currently that information is only available in textual form in a REP and we have to hard code information like that (e.g. to determine which distro to recommend etc.). Also timeline diagrams are currently being manually updated. Having these information specified in the index would allow these cases to be automated...
Regarding the primary specification update annotating each rosdistro with a
I think there's a benefit to new metadata in the rosdistro index, or if there are concerns about cluttering an at-a-glance index file with data creating a separate manifest.yaml or metadata.yaml file that is 1:1 rather than the distribution.yaml file of which there can be arbitrarily many. But given that this specification update is targeting an immediate need I don't know that we want to wait while discussing other required and optional fields and formats. So if we grow the scope of this change at all, I'd suggest only the creation of an additional optional data section we could use to experiment with some or all of these possibilities before the next iteration of this specification. In addition to Dirk's ideas about additional data @tfoote and I have mused about indicating the parent distro for an addon rosdistro or indicating the corresponding ROS 1 pair distro for a ROS 2 distro (e.g. Ardent->Kinetic, Bouncy->Melodic) for the ROS 1 bridge and other interoperating packages. |
Thinking about this in the more abstract the flag for "ros_version" is really about labeling the type of the distro for the ability of tools to do type specific logic. At the moment we have two types ROS 1 and ROS 2. I also see this type as being non-sequential thus the use of a numbering scheme is not likely to be useful for defining range functionality
This also would be quite valuable for extending rosdistros for local instances of buildfarms. This has not been fully fleshed out and I'd suggest avoiding scope creep on this change. |
@nuclearsandwich Bumping the format version of any of the files requires users to update their Therefore imo we should consider adding more information right now or not do it in the near future to keep the spec stable for several years (if possible). |
@tfoote The rational for this name / type sounds reasonable to me. (I would slightly alter the key name to |
After more offline discussion with @tfoote we think a second field named
|
Yeah I think this gives us the flexibility to make other epoch changes without creating confusion about the relationship between the value here and how we refer to ROS as a community.
Unless we relax the urgency of this REP I don't think it's wise to add anything but what we need to address the use-case in front of us right now if we want to "live with" the resulting spec for several years. |
Will the status values be predefined or any arbitrary string? |
I wouldn't restrict the field to a fixed list of possible values but mention at least the two we have an immediate use case for. Other values should explicitly be accepted by the implementation. Future values can be amended to the REP when we establish a certain semantic. |
Modifying the proposed additions to use the
If Since this is a distribution index is the |
Generally I'm in favor of making the keys shorter. However in this context I'd argue for consistency with the other keys. We also use straight 'type' to refer to the file type which could cause confusion. However, this brings to mind the thought that since these values are inherent to the distribution it might make sense to put it inside the distribution.yaml instead of in the index.yaml as it's more inherent to the management of the distro, which technically could be in more than one index.yaml. (For example pointing in from an external buildfarm, and if the distribution is not expected to work under the other type it should be embedded at the lower level. Likewise the status of the distribution file basically reflects on the contents of the distribution.yaml file and thus should be embedded there not externally referenced. (So if you're looking at just the distribution.yaml you'll know if it's EOL without having to know which index it's linked from. If you want to privately extend support for an EOL distro it would be recommended to fork it and change it's status and name. Not just point a new index to it and change the status externally as that doesn't represent the updates.) For status keys I was thinking that |
See above comment why I don't think the distribution file is the right place: #181 (comment) |
What are the use-cases for multiple distribution files (links to historic context are fine responses)? Should we either discuss making distribution files 1:1 with index entries or else add a separate manifest or metadata file that is 1:1? |
E.g. someone can create a forked rosdistro referencing the upstream distribution file and adding one custom one with custom repos. |
That makes sense and is good behavior to preserve. I'm not too troubled by more information being added directly to the index so I'll just mention again that if others are troubled by it then a separate manifest.yaml or metadata.yaml file per entry could be specified but unless we're talking about screenfuls of data I don't think it's necessary. |
8dde4f0
to
7f9c1af
Compare
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.
lgtm
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.
Overall looks good to me. I think the abstract needs either a typo fix or a full update and have left specific comments to that effect.
Co-Authored-By: dirk-thomas <dirk-thomas@users.noreply.github.com>
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.
LGTM
rep-0153.rst
Outdated
|
||
For the following use cases additional metadata is currently necessary: | ||
|
||
A. The ROS buildfarm has to distinguish ROS 1 distributions from ROS 2 |
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.
Not sure why this is rendering in the preview as a numbered list.
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.
I didn't think that standard markdown had lettered lists.
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.
Using numbers as of 94e8e78.
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.
LGTM
In 61ac196 a note was added that moving forward unknown keys should be ignored to allow adding additional information without the need to bump the version. |
Thanks again for the feedback and review. We will start the rollout of this now. I will keep this PR open a bit longer until the rollout is complete before merging it. |
Rollout process:
The next steps will be buildfarm specific... |
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.
Updated approval with the clause to allow unknown keys.
I updated the compatibility section to describe the chosen approach of creating a separate It also mentions that a new patch release of |
Since the changes have been rolled out successfully I am going to merge this. |
@dirk-thomas: where did this requirement/realisation come from? Internal discussion? It's not exactly elegant to have these separate files and I don't remember seeing this approach used with previous changes to the |
The discussion happened on the referenced PR ros-infrastructure/rosdistro#124. |
Related to ros2/ros_buildfarm_config#12.
Since the major ROS version is specific to a distribution it should be annotated in the index file (rather than in the buildfarm configuration).
The current implementation would fail to parse newer files containing the
ros_version
(with the same format version) due to this check: https://github.com/ros-infrastructure/rosdistro/blob/df28723d0d2e12b52cd81867d03ed4c979187e3a/src/rosdistro/index.py#L80Therefore leveling the version of the index format is the cleaner way to go. Then users get a clear error message that they need a new rosdistro version to parse the new format.
This will pave the path to reintegrate the forked ROS 2 rosdistro into the ROS (1) rosdistro repo.