Add rosdep keys for pyparsing and pytest. #16549
Conversation
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.
|
I don't think this is sufficient since we need a newer version of |
|
ooh I hadn't realized we needed 3.2. pytest 3.2 is available but masked in gentoo and 3.3 is the default on arch. We could just drop the keys for fedora, debian, and ubuntu or move them out to a python3-pytest-pip rosdep 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.
|
Trying not to let this wither. I've removed the key for platforms where pytest is too old to be used for ROS 2 and added a separate Bionic lists a suitable python3-pytest but I'm not sure whether now is the time to add bionic keys or not. I also apparently had this pull request mistargted and it is now correctly pointing at master. |
Since the code name is fixed you can add entries for it anytime. |
rosdep/python.yaml
Outdated
| ubuntu: [python3-pyparsing] | ||
| python3-pytest: | ||
| arch: [python-pytest] | ||
| python3-pytest-pip: |
There was a problem hiding this comment.
Which package(s) is/are going to use this key?
There was a problem hiding this comment.
I have no explicit use case for this key, I was attempting to follow observed convention that keys using pip were separate from the regular rosdep key.
It's possible that a platform or OS variable could be used with Package format 3 dependency conditions to select the pip dependency on platforms where it is not available from the default provider.
There was a problem hiding this comment.
I would suggest not to add rosdep keys which are not needed / used by any package but wait with such an addition until that is the case.
There was a problem hiding this comment.
Does it matter that dropping this would mean that no rosdep dependency key for ubuntu xenial (the supported platform) satisfies the test dependencies for ROS 2?
There was a problem hiding this comment.
Before you said the key isn't used anywhere. Under that assumption I recommended to not add it (since it would not be used anywhere).
Can you please clarify which package currently uses the key (or is going to be updated to use this key)?
There was a problem hiding this comment.
<depend condition="$OS_CODENAME == bionic">python3-pytest</depend>
<depend condition="$OS_CODENAME != bionic">python3-pytest-pip</depend>
There was a problem hiding this comment.
That would imply that the package can't be released with bloom into distributions other than bionic.
There was a problem hiding this comment.
Right, I forgot that bloom hard-stops on pip dependencies.
There was a proposed change to allow them to be skipped instead. ros-infrastructure/bloom#412
There was a problem hiding this comment.
Skipping them breaks the expectation that all dependencies are resolved when installing a debian package.
There was a problem hiding this comment.
Skipping them breaks the expectation that all dependencies are resolved when installing a debian package.
It isn't my aim to revisit that PR discussion here. I had to remind myself of bloom's behavior and cross-linked that PR as it was part of that investigation.
If this key isn't useful. I'll remove it.
Bionic is the first release that will include python3-pytest 3.2 for ROS 2.
rosdep/python.yaml
Outdated
| python3-pytest: | ||
| arch: [python-pytest] | ||
| ubuntu: | ||
| bionic: [python3-pytest] |
There was a problem hiding this comment.
I would suggest to just use ubuntu: [python3-pytest] in order to not need to maintain this in the future. The specific version requirement of ament_tools is just an arbitrary line in the sand. Any other package might have a different threshold.
There was a problem hiding this comment.
This is contrary to your first piece of feedback on this pull request.
There was a problem hiding this comment.
My first comment was in the context of your proposal to add the rosdep keys in order to be used for the ROS 2 packages. In that context I mentioned that this will not work since we require a newer version.
During this discussion we have established that the ROS 2 packages can't use this new rosdep key since there is no Debian package available which is new enough. And in the context of just adding these rosdep keys for other packages I think it should not be limited to bionic.
rosdep/python.yaml
Outdated
| ubuntu: [python3-pyparsing] | ||
| python3-pytest: | ||
| arch: [python-pytest] | ||
| ubuntu: [python3-pytest] |
There was a problem hiding this comment.
We encourage to add rules for debian and not only ubuntu.
arch fedora and gentoo are optional but apreciated
Adding it as pip since it's not a rosdep key yet ros/rosdistro#16549
|
Ubuntu links in description point to python2 version of the packages. Here are the links for the pythn3 packages: python3-pyparsing: https://packages.ubuntu.com/search?keywords=python3-pyparsing&searchon=names&exact=1&suite=all§ion=all @nuclearsandwich should the debian and gentoo rules for pytest be readded? Or have they been left out on purpose? |
Now that I'm not trying to add rules solely to satisfy ROS 2, I think they can be re-added. The Fedora package was both very old and in EPEL rather than the primary Fedora repos so I don't think it should be included. |
agreed 👍 that's why I scoped my comment to Debian and Gentoo |
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
Package references:
pyparsing
pytest
(2) Fedora's python3-pytest is an EPEL-only package.
I'm not sure if it should be included.