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: conditional dependencies #143

Open
dirk-thomas opened this Issue Nov 10, 2017 · 8 comments

Comments

Projects
None yet
2 participants
@dirk-thomas
Member

dirk-thomas commented Nov 10, 2017

The draft for REP 149 contains multiple different parts. In order to not mix the discussion on those topics in a single ticket this issue is meant to focus on discussing the introduced conditional dependencies represented by the condition attribute.

@NikolausDemmel

This comment has been minimized.

Show comment
Hide comment
@NikolausDemmel

NikolausDemmel Nov 12, 2017

Contributor

Please excuse my ignorance. I have not followed the discussion, but only just now read the current draft. Some feedback:

  1. Some bits that should maybe be red, but arent: a) the condition attribute in build_depend b) the section on Environment Variables.
  2. The current description of the condition attribute does not make it clear what kind of variables we are talking about here? I.e. how do these variables get their value? Environment variables?
  3. If now environment has been source some tools might require that the necessary information is being specified explicitly when being invoked. --> 2 typos now --> no, source --> sourced.
  4. The section about environment variables mentions that something like ROS_VERSION should be exported by roslib / rcl. The sentence above from that section mentions that this would only work once an environment has already been sourced. This would mean that a from-source build in a fresh environment would only work if you manually set all variables used by all conditions in all package files. The description of the condition attribute should somehow mention or reference this fact. I.e. it should maybe be encouraged to be limited its use to common variables like ROS_VERSION or ROS_DISTRO.
  5. I guess it would be possible to make the tools processing package.xml smarter. For example it is conceivable that catkin_tools parses the package.xml once in the fresh environment, extracts a dependency tree without the conditional dependencies, starts building packages and reevaluates the package.xml from some package in an environment which sources the env hooks of all its unconditional dependencies, thus updating its dependecy tree. But it sounds like this gets really messy, and also tools that previously didn't care about the environment hooks like rosdep would now get much more complicated.
Contributor

NikolausDemmel commented Nov 12, 2017

Please excuse my ignorance. I have not followed the discussion, but only just now read the current draft. Some feedback:

  1. Some bits that should maybe be red, but arent: a) the condition attribute in build_depend b) the section on Environment Variables.
  2. The current description of the condition attribute does not make it clear what kind of variables we are talking about here? I.e. how do these variables get their value? Environment variables?
  3. If now environment has been source some tools might require that the necessary information is being specified explicitly when being invoked. --> 2 typos now --> no, source --> sourced.
  4. The section about environment variables mentions that something like ROS_VERSION should be exported by roslib / rcl. The sentence above from that section mentions that this would only work once an environment has already been sourced. This would mean that a from-source build in a fresh environment would only work if you manually set all variables used by all conditions in all package files. The description of the condition attribute should somehow mention or reference this fact. I.e. it should maybe be encouraged to be limited its use to common variables like ROS_VERSION or ROS_DISTRO.
  5. I guess it would be possible to make the tools processing package.xml smarter. For example it is conceivable that catkin_tools parses the package.xml once in the fresh environment, extracts a dependency tree without the conditional dependencies, starts building packages and reevaluates the package.xml from some package in an environment which sources the env hooks of all its unconditional dependencies, thus updating its dependecy tree. But it sounds like this gets really messy, and also tools that previously didn't care about the environment hooks like rosdep would now get much more complicated.
@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 12, 2017

Member
  1. Some bits that should maybe be red, but arent: a) the condition attribute in build_depend b) the section on Environment Variables.

Thanks for pointing it out. Fixed in 672acb2.

  1. The current description of the condition attribute does not make it clear what kind of variables we are talking about here? I.e. how do these variables get their value? Environment variables?

It is up to the software evaluating the condition what to pass in. E.g. ament_tools passes all environment variables in.

  1. If now environment has been source some tools might require that the necessary information is being specified explicitly when being invoked. --> 2 typos now --> no, source --> sourced.

Thanks for pointing it out. Fixed in 392285d.

  1. The section about environment variables mentions that something like ROS_VERSION should be exported by roslib / rcl. The sentence above from that section mentions that this would only work once an environment has already been sourced. This would mean that a from-source build in a fresh environment would only work if you manually set all variables used by all conditions in all package files. The description of the condition attribute should somehow mention or reference this fact. I.e. it should maybe be encouraged to be limited its use to common variables like ROS_VERSION or ROS_DISTRO.

Restricting the use to common variable might indeed be a good idea. For a from-source build it doesn't necessarily mean that you have to define those manually. The tools building a workspace usually don't consider the dependencies from the manifests beyond deciding the topological order. If additional dependencies are declared (which should be ignored due to their condition) that would only affect the order in case those dependencies actually exist which is often not the case.

  1. I guess it would be possible to make the tools processing package.xml smarter. For example it is conceivable that catkin_tools parses the package.xml once in the fresh environment, extracts a dependency tree without the conditional dependencies, starts building packages and reevaluates the package.xml from some package in an environment which sources the env hooks of all its unconditional dependencies, thus updating its dependecy tree. But it sounds like this gets really messy, and also tools that previously didn't care about the environment hooks like rosdep would now get much more complicated.

The extra logic necessary for the conditions is certainly making various tools more complicated. Sadly we couldn't come up with an easier design proposal to achieve the goals describe by the use cases.

Member

dirk-thomas commented Nov 12, 2017

  1. Some bits that should maybe be red, but arent: a) the condition attribute in build_depend b) the section on Environment Variables.

Thanks for pointing it out. Fixed in 672acb2.

  1. The current description of the condition attribute does not make it clear what kind of variables we are talking about here? I.e. how do these variables get their value? Environment variables?

It is up to the software evaluating the condition what to pass in. E.g. ament_tools passes all environment variables in.

  1. If now environment has been source some tools might require that the necessary information is being specified explicitly when being invoked. --> 2 typos now --> no, source --> sourced.

Thanks for pointing it out. Fixed in 392285d.

  1. The section about environment variables mentions that something like ROS_VERSION should be exported by roslib / rcl. The sentence above from that section mentions that this would only work once an environment has already been sourced. This would mean that a from-source build in a fresh environment would only work if you manually set all variables used by all conditions in all package files. The description of the condition attribute should somehow mention or reference this fact. I.e. it should maybe be encouraged to be limited its use to common variables like ROS_VERSION or ROS_DISTRO.

Restricting the use to common variable might indeed be a good idea. For a from-source build it doesn't necessarily mean that you have to define those manually. The tools building a workspace usually don't consider the dependencies from the manifests beyond deciding the topological order. If additional dependencies are declared (which should be ignored due to their condition) that would only affect the order in case those dependencies actually exist which is often not the case.

  1. I guess it would be possible to make the tools processing package.xml smarter. For example it is conceivable that catkin_tools parses the package.xml once in the fresh environment, extracts a dependency tree without the conditional dependencies, starts building packages and reevaluates the package.xml from some package in an environment which sources the env hooks of all its unconditional dependencies, thus updating its dependecy tree. But it sounds like this gets really messy, and also tools that previously didn't care about the environment hooks like rosdep would now get much more complicated.

The extra logic necessary for the conditions is certainly making various tools more complicated. Sadly we couldn't come up with an easier design proposal to achieve the goals describe by the use cases.

@NikolausDemmel

This comment has been minimized.

Show comment
Hide comment
@NikolausDemmel

NikolausDemmel Nov 13, 2017

Contributor

The current description of the condition attribute does not make it clear what kind of variables we are talking about here? I.e. how do these variables get their value? Environment variables?

It is up to the software evaluating the condition what to pass in. E.g. ament_tools passes all environment variables in.

I think the REP should mention this, at least as an example. Maybe something like "Tools may populate the values for these variables in different ways, but typically they are evaluated as environment variables."

The section about environment variables mentions that something like ROS_VERSION should be exported by roslib / rcl. The sentence above from that section mentions that this would only work once an environment has already been sourced. This would mean that a from-source build in a fresh environment would only work if you manually set all variables used by all conditions in all package files. The description of the condition attribute should somehow mention or reference this fact. I.e. it should maybe be encouraged to be limited its use to common variables like ROS_VERSION or ROS_DISTRO.

Restricting the use to common variable might indeed be a good idea. For a from-source build it doesn't necessarily mean that you have to define those manually. The tools building a workspace usually don't consider the dependencies from the manifests beyond deciding the topological order. If additional dependencies are declared (which should be ignored due to their condition) that would only affect the order in case those dependencies actually exist which is often not the case.

I might be wrong, but I think that for catkin_tools at least in the default setting for build-isolation, it builds every package in a separate devel space, and it sources only exactly those devel spaces of packages that the current package depends upon, so there determining the exact set of dependencies is critical for build success. If this breaks the from-source build of desktop_full with catkin_tools, it needs to be communicated, but of course it would be nice if the workflow at least from desktop_full could stay the same without defining additional environment variables / command line parameters a-priori.

Maybe the REP should not restrict the use of variables, but simply encourage self-restriction. It could be still worth to restrict it for packages in desktop-full, to make the from-source build workflow work as it does now.

Do we expect this to be used for system dependencies? We could also encourage to avoid using conditions for system dependencies, otherwise rosdep might not work for clean from-source builds, at least not with the current workflow.

I guess it would be possible to make the tools processing package.xml smarter. For example it is conceivable that catkin_tools parses the package.xml once in the fresh environment, extracts a dependency tree without the conditional dependencies, starts building packages and reevaluates the package.xml from some package in an environment which sources the env hooks of all its unconditional dependencies, thus updating its dependecy tree. But it sounds like this gets really messy, and also tools that previously didn't care about the environment hooks like rosdep would now get much more complicated.

The extra logic necessary for the conditions is certainly making various tools more complicated. Sadly we couldn't come up with an easier design proposal to achieve the goals describe by the use cases.

I don't really have a better suggestion, except for maybe limiting the usage to a fixed list of variables, such that the various tools have chance to use some other means to provide these values, e.g. command-line parameters, some other heuristics to determine what the user likely wants. If the tools are kept general enough, e.g. always falling back to environment variables, then people can still do whatever they want with this internally, even if it is prohibited for packages in desktop_full.

Contributor

NikolausDemmel commented Nov 13, 2017

The current description of the condition attribute does not make it clear what kind of variables we are talking about here? I.e. how do these variables get their value? Environment variables?

It is up to the software evaluating the condition what to pass in. E.g. ament_tools passes all environment variables in.

I think the REP should mention this, at least as an example. Maybe something like "Tools may populate the values for these variables in different ways, but typically they are evaluated as environment variables."

The section about environment variables mentions that something like ROS_VERSION should be exported by roslib / rcl. The sentence above from that section mentions that this would only work once an environment has already been sourced. This would mean that a from-source build in a fresh environment would only work if you manually set all variables used by all conditions in all package files. The description of the condition attribute should somehow mention or reference this fact. I.e. it should maybe be encouraged to be limited its use to common variables like ROS_VERSION or ROS_DISTRO.

Restricting the use to common variable might indeed be a good idea. For a from-source build it doesn't necessarily mean that you have to define those manually. The tools building a workspace usually don't consider the dependencies from the manifests beyond deciding the topological order. If additional dependencies are declared (which should be ignored due to their condition) that would only affect the order in case those dependencies actually exist which is often not the case.

I might be wrong, but I think that for catkin_tools at least in the default setting for build-isolation, it builds every package in a separate devel space, and it sources only exactly those devel spaces of packages that the current package depends upon, so there determining the exact set of dependencies is critical for build success. If this breaks the from-source build of desktop_full with catkin_tools, it needs to be communicated, but of course it would be nice if the workflow at least from desktop_full could stay the same without defining additional environment variables / command line parameters a-priori.

Maybe the REP should not restrict the use of variables, but simply encourage self-restriction. It could be still worth to restrict it for packages in desktop-full, to make the from-source build workflow work as it does now.

Do we expect this to be used for system dependencies? We could also encourage to avoid using conditions for system dependencies, otherwise rosdep might not work for clean from-source builds, at least not with the current workflow.

I guess it would be possible to make the tools processing package.xml smarter. For example it is conceivable that catkin_tools parses the package.xml once in the fresh environment, extracts a dependency tree without the conditional dependencies, starts building packages and reevaluates the package.xml from some package in an environment which sources the env hooks of all its unconditional dependencies, thus updating its dependecy tree. But it sounds like this gets really messy, and also tools that previously didn't care about the environment hooks like rosdep would now get much more complicated.

The extra logic necessary for the conditions is certainly making various tools more complicated. Sadly we couldn't come up with an easier design proposal to achieve the goals describe by the use cases.

I don't really have a better suggestion, except for maybe limiting the usage to a fixed list of variables, such that the various tools have chance to use some other means to provide these values, e.g. command-line parameters, some other heuristics to determine what the user likely wants. If the tools are kept general enough, e.g. always falling back to environment variables, then people can still do whatever they want with this internally, even if it is prohibited for packages in desktop_full.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 13, 2017

Member

Maybe something like "Tools may populate the values for these variables in different ways, but typically they are evaluated as environment variables."

👍 I added this in bc54d4a.

I might be wrong, but I think that for catkin_tools at least in the default setting for build-isolation, it builds every package in a separate devel space, and it sources only exactly those devel spaces of packages that the current package depends upon, so there determining the exact set of dependencies is critical for build success. If this breaks the from-source build of desktop_full with catkin_tools, it needs to be communicated, but of course it would be nice if the workflow at least from desktop_full could stay the same without defining additional environment variables / command line parameters a-priori.

Might it fail? Yes, it could in very specific cases. But even if the build tool "ignores" the condition attribute it would "just work" in a lot of cases. Why? Because extra dependencies should not do any harm. And the build tool doesn't install them if they are missing but only considers what is already available in the workspace.

Do we expect this to be used for system dependencies? We could also encourage to avoid using conditions for system dependencies, otherwise rosdep might not work for clean from-source builds, at least not with the current workflow.

I can see use cases where that would be definitely be beneficial. E.g. neither of the following packages would be necessary which only exist to map dependencies differently for different ROS distros: gl_dependency, qwt_dependency, webkit_dependency.

rosdep should be able to work just fine once it has been updated to support the information provided in format 3. If you haven't sourced any environment you already need to pass the ROS distro name explicitly using --rosdistro=.... That will provide enough information to define the ROS_DISTRO variable as well as the ROS_VERSION variable which will be known for each ROS distro.

Member

dirk-thomas commented Nov 13, 2017

Maybe something like "Tools may populate the values for these variables in different ways, but typically they are evaluated as environment variables."

👍 I added this in bc54d4a.

I might be wrong, but I think that for catkin_tools at least in the default setting for build-isolation, it builds every package in a separate devel space, and it sources only exactly those devel spaces of packages that the current package depends upon, so there determining the exact set of dependencies is critical for build success. If this breaks the from-source build of desktop_full with catkin_tools, it needs to be communicated, but of course it would be nice if the workflow at least from desktop_full could stay the same without defining additional environment variables / command line parameters a-priori.

Might it fail? Yes, it could in very specific cases. But even if the build tool "ignores" the condition attribute it would "just work" in a lot of cases. Why? Because extra dependencies should not do any harm. And the build tool doesn't install them if they are missing but only considers what is already available in the workspace.

Do we expect this to be used for system dependencies? We could also encourage to avoid using conditions for system dependencies, otherwise rosdep might not work for clean from-source builds, at least not with the current workflow.

I can see use cases where that would be definitely be beneficial. E.g. neither of the following packages would be necessary which only exist to map dependencies differently for different ROS distros: gl_dependency, qwt_dependency, webkit_dependency.

rosdep should be able to work just fine once it has been updated to support the information provided in format 3. If you haven't sourced any environment you already need to pass the ROS distro name explicitly using --rosdistro=.... That will provide enough information to define the ROS_DISTRO variable as well as the ROS_VERSION variable which will be known for each ROS distro.

@NikolausDemmel

This comment has been minimized.

Show comment
Hide comment
@NikolausDemmel

NikolausDemmel Nov 13, 2017

Contributor

Might it fail? Yes, it could in very specific cases. But even if the build tool "ignores" the condition attribute it would "just work" in a lot of cases. Why? Because extra dependencies should not do any harm. And the build tool doesn't install them if they are missing but only considers what is already available in the workspace.

Fair enough. Including additional dependencies when in doubt sounds reasonable for a build tool like catkin, I agree. For other tools I don't know, but you guys will probably figure out a sensible way to handle the default cases for bloom and friends.

I don't want to dwell on this too much and also not overcomplicate things. Maybe we could just add something like the following to the "condition" tag description:

Package maintainers are encouraged to keep the range of variables used in conditions minimal. For most use cases ROS_VERSION and ROS_DISTRO should suffice. See "Environment Variables" below.

Contributor

NikolausDemmel commented Nov 13, 2017

Might it fail? Yes, it could in very specific cases. But even if the build tool "ignores" the condition attribute it would "just work" in a lot of cases. Why? Because extra dependencies should not do any harm. And the build tool doesn't install them if they are missing but only considers what is already available in the workspace.

Fair enough. Including additional dependencies when in doubt sounds reasonable for a build tool like catkin, I agree. For other tools I don't know, but you guys will probably figure out a sensible way to handle the default cases for bloom and friends.

I don't want to dwell on this too much and also not overcomplicate things. Maybe we could just add something like the following to the "condition" tag description:

Package maintainers are encouraged to keep the range of variables used in conditions minimal. For most use cases ROS_VERSION and ROS_DISTRO should suffice. See "Environment Variables" below.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 13, 2017

Member

you guys will probably figure out a sensible way to handle the default cases for bloom and friends.

bloom will know which ROS distro to release in and based on that also the ROS version. Assuming the conditions only use that it can decide on which dependencies to use and which ones to skip.

Maybe we could just add something like the following to the "condition" tag description:

Package maintainers are encouraged to keep the range of variables used in conditions minimal. For most use cases ROS_VERSION and ROS_DISTRO should suffice. See "Environment Variables" below.

I think this is a good idea. I will bring it up in the next meeting and see if we can add that. Thank you for all the feedback.

Member

dirk-thomas commented Nov 13, 2017

you guys will probably figure out a sensible way to handle the default cases for bloom and friends.

bloom will know which ROS distro to release in and based on that also the ROS version. Assuming the conditions only use that it can decide on which dependencies to use and which ones to skip.

Maybe we could just add something like the following to the "condition" tag description:

Package maintainers are encouraged to keep the range of variables used in conditions minimal. For most use cases ROS_VERSION and ROS_DISTRO should suffice. See "Environment Variables" below.

I think this is a good idea. I will bring it up in the next meeting and see if we can add that. Thank you for all the feedback.

@NikolausDemmel

This comment has been minimized.

Show comment
Hide comment
@NikolausDemmel

NikolausDemmel Nov 13, 2017

Contributor

Thanks Dirk!

Contributor

NikolausDemmel commented Nov 13, 2017

Thanks Dirk!

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Nov 14, 2017

Member

We talked about the possible environment variables and lean toward only to recommend using a specific list of "known" environment variables. The reasoning is that tools need to support setting them / passing them potentially as command line arguments. Beside ROS_VERSION and ROS_DISTRO we also consider having a variable ROS_PYTHON_VERSION but since the scope and exact usage of that isn't decided yet we will probably not add it to the current draft of the REP. I will update the REP with this soonish.

Member

dirk-thomas commented Nov 14, 2017

We talked about the possible environment variables and lean toward only to recommend using a specific list of "known" environment variables. The reasoning is that tools need to support setting them / passing them potentially as command line arguments. Beside ROS_VERSION and ROS_DISTRO we also consider having a variable ROS_PYTHON_VERSION but since the scope and exact usage of that isn't decided yet we will probably not add it to the current draft of the REP. I will update the REP with this soonish.

nuclearsandwich added a commit to nuclearsandwich/rosdistro that referenced this issue Dec 7, 2017

Add rosdep keys for pyparsing and pytest.

mikaelarguedas added a commit to ros/rosdistro that referenced this issue Jan 10, 2018

Add rosdep keys for pyparsing and pytest. (#16549)
* Add rosdep keys for pyparsing and pytest.

pytest and pyparsing are needed for ROS 2 development.

pytest is the new recommended test runner for ament_python packages (as far as I understand).

pyparsing is used in the current implementation of the conditional dependencies [[1]] for package.xml format 3

[1]: ros-infrastructure/rep#143

Package references:

* pyparsing
    * arch https://www.archlinux.org/packages/extra/any/python-pyparsing/
    * debian https://packages.debian.org/stable/python3-pyparsing
    * fedora https://apps.fedoraproject.org/packages/python3-pyparsing
    * gentoo https://packages.gentoo.org/packages/dev-python/pyparsing
    * ubuntu
        * trusty https://packages.ubuntu.com/trusty/python-pyparsing
        * xenial https://packages.ubuntu.com/xenial/python-pyparsing
        * artful https://packages.ubuntu.com/artful/python-pyparsing
        * bionic https://packages.ubuntu.com/bionic/python-pyparsing

* pytest
    * arch https://www.archlinux.org/packages/community/any/python-pytest/
    * debian https://packages.debian.org/stable/python3-pytest
    * fedora (2)  https://apps.fedoraproject.org/packages/python3-pytest/sources/
    * gentoo https://packages.gentoo.org/packages/dev-python/pytest
    * ubuntu
        * trusty https://packages.ubuntu.com/trusty/python-pytest
        * xenial https://packages.ubuntu.com/xenial/python-pytest
        * artful https://packages.ubuntu.com/artful/python-pytest
        * bionic https://packages.ubuntu.com/bionic/python-pytest

(2) Fedora's python3-pytest is an EPEL-only package.
I'm not sure if it should be included.

* Drop gentoo key.

I don't know whether gentoo has pip support via rosdep or if it's
possible to specfiy keys for masked packages.
For now I am just dropping it.

* Create a -pip key for python3-pytest on platforms where it is too old for ROS 2.

* Add python3-pytest key for Ubunbu Bionic.

Bionic is the first release that will include python3-pytest 3.2 for ROS 2.

* Remove python3-pytest-pip key.

* Don't bother scoping pytest to bionic.

* Re-add python3-pytest keys for debian and gentoo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment