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

REP 149: Package Manifest Format Three Specification #138

Merged
merged 10 commits into from Nov 11, 2017

Conversation

Projects
None yet
7 participants
@dirk-thomas
Member

dirk-thomas commented Oct 10, 2017

Opening this draft for reference purposes. The goal is to cover:

  • group dependencies #141
  • ABI / compatibility version #142
  • conditional dependencies (e.g. across ROS 1 and ROS 2) #143

In order to not mix the discussion on the different subjects I have created three separate issues. Please comment on the referenced issues for topic specific feedback.

@dirk-thomas dirk-thomas self-assigned this Oct 10, 2017

@nuclearsandwich

This comment has been minimized.

Show comment
Hide comment
@nuclearsandwich

nuclearsandwich Oct 17, 2017

I feel like the abi_version attribute belongs on the <version> element rather than the <package> element. Because of its adjacency with the package manifest format it seems like the property is specific to the package manifest rather than the package itself.

Is there language that defines abi_version compatibility within the scope of the package manifest attribute or is it intended to be discretionary?

nuclearsandwich commented Oct 17, 2017

I feel like the abi_version attribute belongs on the <version> element rather than the <package> element. Because of its adjacency with the package manifest format it seems like the property is specific to the package manifest rather than the package itself.

Is there language that defines abi_version compatibility within the scope of the package manifest attribute or is it intended to be discretionary?

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Oct 17, 2017

Member

I feel like the abi_version attribute belongs on the <version> element rather than the <package> element. Because of its adjacency with the package manifest format it seems like the property is specific to the package manifest rather than the package itself.

Once we have a consensus on the proposed semantic we can shuffle the information around wherever we want it to be.

Is there language that defines abi_version compatibility within the scope of the package manifest attribute or is it intended to be discretionary?

Packages built against a previous version of the package will continue to work. That is generally what ABI compatibility expresses.

Member

dirk-thomas commented Oct 17, 2017

I feel like the abi_version attribute belongs on the <version> element rather than the <package> element. Because of its adjacency with the package manifest format it seems like the property is specific to the package manifest rather than the package itself.

Once we have a consensus on the proposed semantic we can shuffle the information around wherever we want it to be.

Is there language that defines abi_version compatibility within the scope of the package manifest attribute or is it intended to be discretionary?

Packages built against a previous version of the package will continue to work. That is generally what ABI compatibility expresses.

Show outdated Hide outdated rep-0149.rst Outdated
Show outdated Hide outdated rep-0149.rst Outdated
@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Oct 20, 2017

Member

I have updated the REP draft based on the recent discussion of group dependencies as well as ABI / compatibility version.

@ros-infrastructure/ros_team Please take a closer look and feel free to fix / improve spelling / wording where you see fit and propose concrete additions if you think more information / context would be helpful.

Member

dirk-thomas commented Oct 20, 2017

I have updated the REP draft based on the recent discussion of group dependencies as well as ABI / compatibility version.

@ros-infrastructure/ros_team Please take a closer look and feel free to fix / improve spelling / wording where you see fit and propose concrete additions if you think more information / context would be helpful.

::
<package format="2">

This comment has been minimized.

@wjwwood

wjwwood Oct 20, 2017

Contributor

Example should be updated?

@wjwwood

wjwwood Oct 20, 2017

Contributor

Example should be updated?

This comment has been minimized.

@dirk-thomas

dirk-thomas Oct 23, 2017

Member

I am not sure what would be a good example to use both of the new tags. I am open to suggestions.

@dirk-thomas

dirk-thomas Oct 23, 2017

Member

I am not sure what would be a good example to use both of the new tags. I am open to suggestions.

Show outdated Hide outdated rep-0149.rst Outdated
Show outdated Hide outdated rep-0149.rst Outdated
Show outdated Hide outdated rep-0149.rst Outdated
doesn't need to change.
For packages which do specify a `compatibility` version the tool should
probably by default remove the attribute and only after confirmation from
the user offer to keep it.

This comment has been minimized.

@wjwwood

wjwwood Oct 20, 2017

Contributor

The behavior I would expect is:

  • if the compatibility attribute exists:
    • ask the user if it should be changed to the current version or left as-is
  • if not:
    • let they user know that this release will cause downstream packages to be rebuilt (let them know not having compatibility between releases causes extra effort)

Since it is opt in anyways, why would the tool automatically opt out again? It could have an interactive message to make sure the user knows the version change needs to be stable unless this attributed is updated (to notify new maintainers that they package previously had compatibility control).


Is this just because build.ros.org doesn't process this attribute yet? In that case I'd just advocate for a warning (perhaps with a confirmation so they can cancel if they want) that this is the case, but offering to remove it seems unnecessary to me.

@wjwwood

wjwwood Oct 20, 2017

Contributor

The behavior I would expect is:

  • if the compatibility attribute exists:
    • ask the user if it should be changed to the current version or left as-is
  • if not:
    • let they user know that this release will cause downstream packages to be rebuilt (let them know not having compatibility between releases causes extra effort)

Since it is opt in anyways, why would the tool automatically opt out again? It could have an interactive message to make sure the user knows the version change needs to be stable unless this attributed is updated (to notify new maintainers that they package previously had compatibility control).


Is this just because build.ros.org doesn't process this attribute yet? In that case I'd just advocate for a warning (perhaps with a confirmation so they can cancel if they want) that this is the case, but offering to remove it seems unnecessary to me.

This comment has been minimized.

@dirk-thomas

dirk-thomas Oct 23, 2017

Member

Just because the previous release of a package was compatible was earlier versions doesn't imply the next release will be too. I think it would be safer to assume that it is not until the maintainer explicitly says otherwise.

@dirk-thomas

dirk-thomas Oct 23, 2017

Member

Just because the previous release of a package was compatible was earlier versions doesn't imply the next release will be too. I think it would be safer to assume that it is not until the maintainer explicitly says otherwise.

This comment has been minimized.

@wjwwood

wjwwood Oct 23, 2017

Contributor

I would rather see the compatibility version updated each time (same behavior as removing it), rather than removed. So long as the tool is automating it, either way should be fine I just think it would produce a more normal history to continuously update the compatibility version rather than periodically removing it and restoring it. I think it's a reasonable assumption that the default behavior after releasing once with a compatibility version will be to continue using the version even if every release is not compatible.

This seems, however, to be a lot of detail on one tools behavior for this document. It might just be sufficient to say "the tool should default to updating the compatibility version each release, requiring the user to explicitly opt into a backwards-compatible release".

@wjwwood

wjwwood Oct 23, 2017

Contributor

I would rather see the compatibility version updated each time (same behavior as removing it), rather than removed. So long as the tool is automating it, either way should be fine I just think it would produce a more normal history to continuously update the compatibility version rather than periodically removing it and restoring it. I think it's a reasonable assumption that the default behavior after releasing once with a compatibility version will be to continue using the version even if every release is not compatible.

This seems, however, to be a lot of detail on one tools behavior for this document. It might just be sufficient to say "the tool should default to updating the compatibility version each release, requiring the user to explicitly opt into a backwards-compatible release".

Show outdated Hide outdated rep-0149.rst Outdated
Show outdated Hide outdated rep-0149.rst Outdated
Show outdated Hide outdated rep-0149.rst Outdated
Show outdated Hide outdated rep-0149.rst Outdated
Show outdated Hide outdated rep-0149.rst Outdated
@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Oct 23, 2017

Member

Thank, I applied most of the feedback in bca3965.

Member

dirk-thomas commented Oct 23, 2017

Thank, I applied most of the feedback in bca3965.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 8, 2017

Member

The REP draft now also describes the condition expression which can be used on dependency and membership tags.

@ros-infrastructure/ros_team Please take another close look and feel free to fix / improve spelling / wording where you see fit and propose concrete additions if you think more information / context would be helpful. After that I would like to get feedback from the community on this. I might create three separate issues for the three different parts of the REP to keep each thread focus on one topic.

Member

dirk-thomas commented Nov 8, 2017

The REP draft now also describes the condition expression which can be used on dependency and membership tags.

@ros-infrastructure/ros_team Please take another close look and feel free to fix / improve spelling / wording where you see fit and propose concrete additions if you think more information / context would be helpful. After that I would like to get feedback from the community on this. I might create three separate issues for the three different parts of the REP to keep each thread focus on one topic.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 10, 2017

Member

@clalancette @dhood @Karsten1987 @mikaelarguedas @nuclearsandwich @sloretz @tfoote @wjwwood Please provide feedback on the current state of the draft today. If you have no comments and think the draft doesn't need any changes please at least add a reaction to this comment for visibility.

I plan to send an announcement to discourse later today to get feedback from the ROS community if there are no objections.

Member

dirk-thomas commented Nov 10, 2017

@clalancette @dhood @Karsten1987 @mikaelarguedas @nuclearsandwich @sloretz @tfoote @wjwwood Please provide feedback on the current state of the draft today. If you have no comments and think the draft doesn't need any changes please at least add a reaction to this comment for visibility.

I plan to send an announcement to discourse later today to get feedback from the ROS community if there are no objections.

@nuclearsandwich

This comment has been minimized.

Show comment
Hide comment
@nuclearsandwich

nuclearsandwich Nov 10, 2017

In general I think the draft is ready for community feedback. 👍

I have one comment / proposal related to the Environment Variables section.

The package exporting the necessary environment should be a dependency of almost all ROS 2 packages to ensure that the information is available even when only some packages are installed. The package rcl seems to be a good place for this.

Another possibility would be to make the ros-workspace package that we've been using to bootstrap the beta releases more official and give it this responsibility. Its only root dependencies are ament_package and ament_cmake_core its sole purpose is providing the base environment and setup files.

Right now bloom has a hack in place to make it a dependency of every other package for ROS 2 distros. I would love to either retire it in favor of an accepted root workspace package, rcl or otherwise, or clean it up and canonicalize its status.

Package compatibility support will make rebuilding rcl as a root package less impactful but rcl is actually quite high level in the package hierarchy. These are its direct upstreams on the buildfarm:

  • ament_cmake
  • ament_cmake_gtest
  • ament_cmake_nose
  • ament_cmake_ros
  • ament_lint_auto
  • ament_lint_common
  • example_interfaces
  • launch
  • rcl_interfaces
  • rcutils
  • rmw
  • rmw_implementation
  • rmw_implementation_cmake
  • rosidl_default_runtime
  • rosidl_generator
  • std_msgs

We could, of course, keep using the ros-workspace package to do what its doing and just add the ROS_DISTRO and ROS_VERSION envars to rcl, but it makes more sense to me to provide the environment variables in one consistent place.

nuclearsandwich commented Nov 10, 2017

In general I think the draft is ready for community feedback. 👍

I have one comment / proposal related to the Environment Variables section.

The package exporting the necessary environment should be a dependency of almost all ROS 2 packages to ensure that the information is available even when only some packages are installed. The package rcl seems to be a good place for this.

Another possibility would be to make the ros-workspace package that we've been using to bootstrap the beta releases more official and give it this responsibility. Its only root dependencies are ament_package and ament_cmake_core its sole purpose is providing the base environment and setup files.

Right now bloom has a hack in place to make it a dependency of every other package for ROS 2 distros. I would love to either retire it in favor of an accepted root workspace package, rcl or otherwise, or clean it up and canonicalize its status.

Package compatibility support will make rebuilding rcl as a root package less impactful but rcl is actually quite high level in the package hierarchy. These are its direct upstreams on the buildfarm:

  • ament_cmake
  • ament_cmake_gtest
  • ament_cmake_nose
  • ament_cmake_ros
  • ament_lint_auto
  • ament_lint_common
  • example_interfaces
  • launch
  • rcl_interfaces
  • rcutils
  • rmw
  • rmw_implementation
  • rmw_implementation_cmake
  • rosidl_default_runtime
  • rosidl_generator
  • std_msgs

We could, of course, keep using the ros-workspace package to do what its doing and just add the ROS_DISTRO and ROS_VERSION envars to rcl, but it makes more sense to me to provide the environment variables in one consistent place.

@tfoote

tfoote approved these changes Nov 10, 2017

A few suggestions but otherwise lgtm to circulate

Show outdated Hide outdated rep-0149.rst Outdated
Show outdated Hide outdated rep-0149.rst Outdated
Show outdated Hide outdated rep-0149.rst Outdated
Show outdated Hide outdated rep-0149.rst Outdated
@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 11, 2017

Member

I will merge the draft as-is since the generated HTML page is much easier for users to read.

Member

dirk-thomas commented Nov 11, 2017

I will merge the draft as-is since the generated HTML page is much easier for users to read.

@dirk-thomas dirk-thomas merged commit bf66539 into master Nov 11, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@dirk-thomas dirk-thomas deleted the rep149 branch Nov 11, 2017

@peci1

This comment has been minimized.

Show comment
Hide comment
@peci1

peci1 Nov 11, 2017

Would it be possible to put online some example, e.g. how would the std_msgs package.xml change utilizing the new possibilities? Without an example, it is still too abstract to me...

peci1 commented Nov 11, 2017

Would it be possible to put online some example, e.g. how would the std_msgs package.xml change utilizing the new possibilities? Without an example, it is still too abstract to me...

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 11, 2017

Member

@peci1 The message packages don't use any of the features yet.

rmw_implementation e.g. uses the group dependencies which can serve as an example: https://github.com/ros2/rmw_implementation/pull/32/files#diff-e20e58462fdb7b39b7dba23ae765bab7R23

Specific implementations like rmw_opensplice_cpp can then declare themselves as being a member of that group: https://github.com/ros2/rmw_opensplice/pull/210/files#diff-416ef50d50e6a77d8de4e69360278131R38

If a package wants to target ROS 1 as well as ROS 2 from the same code base it can use conditional dependencies to avoid branching just for the manifest. This could look like the following:

<build_depend condition="$ROS_VERSION==1">message_generation</build_depend>
<exec_depend condition="$ROS_VERSION==1">message_runtime</exec_depend>

<build_depend condition="$ROS_VERSION==2">rosidl_default_generators</build_depend>
<build_depend condition="$ROS_VERSION==2">rosidl_default_runtime</build_depend>
<depend condition="$ROS_VERSION==2">builtin_interfaces</depend>

Whatever would be necessary on CMake and source code level is not in the scope of this PR though.

Member

dirk-thomas commented Nov 11, 2017

@peci1 The message packages don't use any of the features yet.

rmw_implementation e.g. uses the group dependencies which can serve as an example: https://github.com/ros2/rmw_implementation/pull/32/files#diff-e20e58462fdb7b39b7dba23ae765bab7R23

Specific implementations like rmw_opensplice_cpp can then declare themselves as being a member of that group: https://github.com/ros2/rmw_opensplice/pull/210/files#diff-416ef50d50e6a77d8de4e69360278131R38

If a package wants to target ROS 1 as well as ROS 2 from the same code base it can use conditional dependencies to avoid branching just for the manifest. This could look like the following:

<build_depend condition="$ROS_VERSION==1">message_generation</build_depend>
<exec_depend condition="$ROS_VERSION==1">message_runtime</exec_depend>

<build_depend condition="$ROS_VERSION==2">rosidl_default_generators</build_depend>
<build_depend condition="$ROS_VERSION==2">rosidl_default_runtime</build_depend>
<depend condition="$ROS_VERSION==2">builtin_interfaces</depend>

Whatever would be necessary on CMake and source code level is not in the scope of this PR though.

Every dependency can be conditional on a condition expression.
If the condition expression evaluate to "true" the dependency is being used
and considered as if it doesn't have a condition attribute.
If the condition expression evaluate to "false" the dependency is being

This comment has been minimized.

@peci1

peci1 Nov 11, 2017

Shouldn't there be a better definition of "evaluates"? In which language? In which semantics? In what context (e.g. what variables are available and how to set them)? Is it possible to compare against strings? Should strings be quoted? Why no "<", "<=", ">", ">=" ?

It would seem nice to me if there would be an agreement about this in-XML conditional expressions language and expressivity. Right now I know about this, about roslaunch, and about Xacro; maybe there are even more tools evaluating conditions inside XML files. It starts to be difficult to keep track about where I can use what...

@peci1

peci1 Nov 11, 2017

Shouldn't there be a better definition of "evaluates"? In which language? In which semantics? In what context (e.g. what variables are available and how to set them)? Is it possible to compare against strings? Should strings be quoted? Why no "<", "<=", ">", ">=" ?

It would seem nice to me if there would be an agreement about this in-XML conditional expressions language and expressivity. Right now I know about this, about roslaunch, and about Xacro; maybe there are even more tools evaluating conditions inside XML files. It starts to be difficult to keep track about where I can use what...

This comment has been minimized.

@dirk-thomas

dirk-thomas Nov 12, 2017

Member

Shouldn't there be a better definition of "evaluates"?

The string describes an expression which can be "evaluated" to a boolean value. I am not sure what kind of definition you would like to see. Can you please propose a text which clarifies your expectation.

In which language?

Evaluating could happen in any language. E.g. ament_package provides a function in Python which parses the condition and can evaluate it based on some input variables.

In which semantics? In what context (e.g. what variables are available and how to set them)?

Any tool evaluating these conditions has to define that. E.g. ament_tools passes all environment variables in. If a tool like rosdep needs to also support to be invoked without environment being set already then it could offer command line options like --ros-version (similar to the existing option --ros-distro.

Is it possible to compare against strings? Should strings be quoted? Why no "<", "<=", ">", ">=" ?

All variables are interpreted as strings. The same as e.g. environment variables are being used in a shell. Since there is no typing beyond strings the meaning of comparison operators is less useful. Since there wasn't a use case requiring this those operators where not specified in the current draft.

It would seem nice to me if there would be an agreement about this in-XML conditional expressions language and expressivity. Right now I know about this, about roslaunch, and about Xacro; maybe there are even more tools evaluating conditions inside XML files. It starts to be difficult to keep track about where I can use what...

For complex conditions in XML would be one way. Imo the way roslaunch exposes those especially with the desire to express even more and more complicated conditions is not a sustainable way. The prototype of launch files in ROS 2 doesn't use XML for that reason. A script like Python is much better suited to express complex conditions and logic to decide if a node should be started or what the arguments / parameters should be. Pushing that complexity into XML is from my point of view not desired.

@dirk-thomas

dirk-thomas Nov 12, 2017

Member

Shouldn't there be a better definition of "evaluates"?

The string describes an expression which can be "evaluated" to a boolean value. I am not sure what kind of definition you would like to see. Can you please propose a text which clarifies your expectation.

In which language?

Evaluating could happen in any language. E.g. ament_package provides a function in Python which parses the condition and can evaluate it based on some input variables.

In which semantics? In what context (e.g. what variables are available and how to set them)?

Any tool evaluating these conditions has to define that. E.g. ament_tools passes all environment variables in. If a tool like rosdep needs to also support to be invoked without environment being set already then it could offer command line options like --ros-version (similar to the existing option --ros-distro.

Is it possible to compare against strings? Should strings be quoted? Why no "<", "<=", ">", ">=" ?

All variables are interpreted as strings. The same as e.g. environment variables are being used in a shell. Since there is no typing beyond strings the meaning of comparison operators is less useful. Since there wasn't a use case requiring this those operators where not specified in the current draft.

It would seem nice to me if there would be an agreement about this in-XML conditional expressions language and expressivity. Right now I know about this, about roslaunch, and about Xacro; maybe there are even more tools evaluating conditions inside XML files. It starts to be difficult to keep track about where I can use what...

For complex conditions in XML would be one way. Imo the way roslaunch exposes those especially with the desire to express even more and more complicated conditions is not a sustainable way. The prototype of launch files in ROS 2 doesn't use XML for that reason. A script like Python is much better suited to express complex conditions and logic to decide if a node should be started or what the arguments / parameters should be. Pushing that complexity into XML is from my point of view not desired.

This comment has been minimized.

@peci1

peci1 Nov 16, 2017

Well, it seems to me that what's in this specification is just the syntax, but not the semantics of the expression.

There's e.g. not specified the meaning of the parentheses (of course it is "apparent", but unless written down, somebody may interpret it differently).

I suggest adding the following paragraph (after the list of tokens):

An expression syntactically correct by the previous definition will be evaluated as follows. First, all variables are substituted by their values and treated as strings. All literals are also treated as strings. The resulting expression is then evaluated as a Python 3 interpreter would evaluate it.

Beware, in Python "1" != 1! Empty string evaluates to False.

Then I suggest adding a few hints about error handling. I just don't know which of these to add:

  1. When a syntactically incorrect condition expression is encountered, the processing tool should stop with an error.
  2. When a syntactically incorrect condition expression is encountered, the condition attribute is ignored and the processing tool should produce a warning or alert the developer (not necessarily the user).

Also, error handling for the variables should be defined:

  1. When a variable is used in the condition expression, whose value is not known to the processing tool, it should stop processing and issue an error.
  2. When a variable is used in the condition expression, whose value is not known to the processing tool, it should ignore the condition attribute and issue a warning to the developer.
  3. When a variable is used in the condition expression, whose value is not known to the processing tool, it is substituted with and empty string (and a warning is issued to the developer).

As a final suggestion, an example could be given.

As an example, a dependency might only be needed in ROS 1 build environment. Such dependency could read <depend condition="$ROS_VERSION==1">roscpp</depend>, where the value of $ROS_VERSION can be taken from the shell environment by catkin/ament.

I still think more comparison operators would be better, to be e.g. able to write $ROS_DISTRO >= kinetic. However I know that this would greatly increase the complexity of this specification. And it is not wise to just let there be any Python expression, because then even the C++ processing tools would need to emulate/access the Python interpreter. So, I am for a constrained syntax, but maybe not that much constrained.

@peci1

peci1 Nov 16, 2017

Well, it seems to me that what's in this specification is just the syntax, but not the semantics of the expression.

There's e.g. not specified the meaning of the parentheses (of course it is "apparent", but unless written down, somebody may interpret it differently).

I suggest adding the following paragraph (after the list of tokens):

An expression syntactically correct by the previous definition will be evaluated as follows. First, all variables are substituted by their values and treated as strings. All literals are also treated as strings. The resulting expression is then evaluated as a Python 3 interpreter would evaluate it.

Beware, in Python "1" != 1! Empty string evaluates to False.

Then I suggest adding a few hints about error handling. I just don't know which of these to add:

  1. When a syntactically incorrect condition expression is encountered, the processing tool should stop with an error.
  2. When a syntactically incorrect condition expression is encountered, the condition attribute is ignored and the processing tool should produce a warning or alert the developer (not necessarily the user).

Also, error handling for the variables should be defined:

  1. When a variable is used in the condition expression, whose value is not known to the processing tool, it should stop processing and issue an error.
  2. When a variable is used in the condition expression, whose value is not known to the processing tool, it should ignore the condition attribute and issue a warning to the developer.
  3. When a variable is used in the condition expression, whose value is not known to the processing tool, it is substituted with and empty string (and a warning is issued to the developer).

As a final suggestion, an example could be given.

As an example, a dependency might only be needed in ROS 1 build environment. Such dependency could read <depend condition="$ROS_VERSION==1">roscpp</depend>, where the value of $ROS_VERSION can be taken from the shell environment by catkin/ament.

I still think more comparison operators would be better, to be e.g. able to write $ROS_DISTRO >= kinetic. However I know that this would greatly increase the complexity of this specification. And it is not wise to just let there be any Python expression, because then even the C++ processing tools would need to emulate/access the Python interpreter. So, I am for a constrained syntax, but maybe not that much constrained.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 16, 2017

Member

@peci1 Thank you for your feedback. I have applied some of it in 71ccf1f.

Then I suggest adding a few hints about error handling.

I am not sure those "rules" make sense for each and every tool. Therefore I would rather not specify this in the REP but let each tool decide what behavior makes most sense based on the context.

I still think more comparison operators would be better, to be e.g. able to write $ROS_DISTRO >= kinetic.

Especially for the ROS distro name the operator can't be implemented easily. Simply because there is no knowledge about the order of the names. It would require a new release of a software package which defines those names whenever a new distro is being released (currently that is not the case).

In general instead of defining an "open-ended" interval you can invert the logic and specify the known inverted set of distro names. In this case $ROS_DISTRO != indigo and $ROS_DISTRO != jade. While it is more verbose it doesn't require any further knowledge / update in the future.

Member

dirk-thomas commented Nov 16, 2017

@peci1 Thank you for your feedback. I have applied some of it in 71ccf1f.

Then I suggest adding a few hints about error handling.

I am not sure those "rules" make sense for each and every tool. Therefore I would rather not specify this in the REP but let each tool decide what behavior makes most sense based on the context.

I still think more comparison operators would be better, to be e.g. able to write $ROS_DISTRO >= kinetic.

Especially for the ROS distro name the operator can't be implemented easily. Simply because there is no knowledge about the order of the names. It would require a new release of a software package which defines those names whenever a new distro is being released (currently that is not the case).

In general instead of defining an "open-ended" interval you can invert the logic and specify the known inverted set of distro names. In this case $ROS_DISTRO != indigo and $ROS_DISTRO != jade. While it is more verbose it doesn't require any further knowledge / update in the future.

@jack-oquin

This comment has been minimized.

Show comment
Hide comment
@jack-oquin

jack-oquin Nov 17, 2017

Contributor

The order of ROS distros would be alphabetical, one supposes.

Contributor

jack-oquin commented Nov 17, 2017

The order of ROS distros would be alphabetical, one supposes.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 17, 2017

Member

The order of ROS distros would be alphabetical, one supposes.

That assumption might hold for a while. But at some point as you can currently see for Ubuntu that won't be the case anymore.

Member

dirk-thomas commented Nov 17, 2017

The order of ROS distros would be alphabetical, one supposes.

That assumption might hold for a while. But at some point as you can currently see for Ubuntu that won't be the case anymore.

to particular versions. For each comparison operator an attribute
can be used. Two of these attributes can be used together to
describe a version range.

This comment has been minimized.

@peci1

peci1 Nov 20, 2017

It might come handy to mention that rosdep currently ignores the version_* tags. Or add a link to issue ros-infrastructure/rosdep#325 . Again, an example use-case might help. Here, we could mention that Debian package version constraints can be generated from these tags.

@peci1

peci1 Nov 20, 2017

It might come handy to mention that rosdep currently ignores the version_* tags. Or add a link to issue ros-infrastructure/rosdep#325 . Again, an example use-case might help. Here, we could mention that Debian package version constraints can be generated from these tags.

This comment has been minimized.

@dirk-thomas

dirk-thomas Dec 3, 2017

Member

This part as not changed since the previous REP. Therefore I won't focus on this in this REP but concentrate on the newly added attributes and tags. Please consider providing a PR to amend the existing REPs to add some example use case.

@dirk-thomas

dirk-thomas Dec 3, 2017

Member

This part as not changed since the previous REP. Therefore I won't focus on this in this REP but concentrate on the newly added attributes and tags. Please consider providing a PR to amend the existing REPs to add some example use case.

@jack-oquin

This comment has been minimized.

Show comment
Hide comment
@jack-oquin

jack-oquin Nov 20, 2017

Contributor

The order of ROS distros would be alphabetical, one supposes.

That assumption might hold for a while. But at some point as you can currently see for Ubuntu that won't be the case anymore.

Ten years from now, when that becomes a problem, we could apply something like a rot13 adjustment on the leading letter.

Not to belabor a minor point, but I do think writing $ROS_DISTRO >= kinetic would be useful.

Contributor

jack-oquin commented Nov 20, 2017

The order of ROS distros would be alphabetical, one supposes.

That assumption might hold for a while. But at some point as you can currently see for Ubuntu that won't be the case anymore.

Ten years from now, when that becomes a problem, we could apply something like a rot13 adjustment on the leading letter.

Not to belabor a minor point, but I do think writing $ROS_DISTRO >= kinetic would be useful.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Dec 4, 2017

Member

@jack-oquin Please see #146 which add these comparison operators.

Member

dirk-thomas commented Dec 4, 2017

@jack-oquin Please see #146 which add these comparison operators.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jan 24, 2018

Member

Please review ros/ros_environment#1 which creates the new ROS package described in this REP.

Member

dirk-thomas commented Jan 24, 2018

Please review ros/ros_environment#1 which creates the new ROS package described in this REP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment