ros2 / sros2

tools to generate and distribute keys for SROS 2
Apache License 2.0
89 stars 44 forks source link

Declare missing dependency on python3-importlib-resources #249

Closed cottsay closed 3 years ago

cottsay commented 3 years ago

Requires ros/rosdistro#28001.

Used here: https://github.com/ros2/sros2/blob/74da58f7433833562226f1cf05113229cee11005/sros2/sros2/policy/__init__.py#L19-L25

The source code supports using the importlib_resources package, but on all of the platforms we're running regular builds for, we have Python 3.7 Python 3.9, which provides the package as part of Python itself.

codecov[bot] commented 3 years ago

Codecov Report

Merging #249 (feb7e49) into master (58dc4b4) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #249   +/-   ##
=======================================
  Coverage   77.25%   77.25%           
=======================================
  Files          25       25           
  Lines         664      664           
  Branches       55       55           
=======================================
  Hits          513      513           
  Misses        131      131           
  Partials       20       20           
Flag Coverage Δ
unittests 77.25% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sros2/sros2/policy/schemas/__init__.py
sros2/sros2/verb/__init__.py
sros2/sros2/verb/create_enclave.py
sros2/sros2/policy/templates/__init__.py
sros2/sros2/__init__.py
sros2/sros2/keystore/_permission.py
sros2/sros2/keystore/_keystore.py
sros2/sros2/verb/create_permission.py
sros2/sros2/policy/templates/dds/__init__.py
sros2/sros2/api/_policy.py
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 74da58f...feb7e49. Read the comment docs.

mikaelarguedas commented 3 years ago

thanks for the fix.

This seems to introduce pip dependencies on some platforms listed in REP-2000 (Debian Buster). Does this mean that sros2 will not be package-able on those platforms after this fix ?

cottsay commented 3 years ago

Does this mean that sros2 will not be package-able on those platforms after this fix?

It appears that is the case. It actually won't matter, though, because ament_package itself requires importlib_resources, so nothing is going to work until the package is available.

I wasn't aware of this - I'll bring it up at the next ROS 2 meeting and we'll discuss the impact this will have on Galactic. You can hold this PR if you like, but there are a few other packages in ros2.repos that are already taking this dependency.

mikaelarguedas commented 3 years ago

It appears that is the case. It actually won't matter, though, because ament_package itself requires importlib_resources, so nothing is going to work until the package is available.

Fair enough, :+1: for this then and we'll see if a follow-up is needed based on the result of the ROS 2 meeting discussion

cottsay commented 3 years ago

I'll bring it up at the next ROS 2 meeting and we'll discuss the impact this will have on Galactic

In the ROS 2 meeting we decided to target Bullseye for a variety of reasons, importlib_resources among them. It's also worth noting that we haven't built any binary packages for Debian in ROS 2 to-date.

mikaelarguedas commented 3 years ago

importlib_resources among them. It's also worth noting that we haven't built any binary packages for Debian in ROS 2 to-date.

:+1:, it's still good to know we're not standing in the way of people that may want to build debs for their own applications

kyrofa commented 3 years ago

The source code supports using the importlib_resources package, but on all of the platforms we're running regular builds for, we have Python 3.7, which provides the package as part of Python itself.

Post-Foxy, which supported platforms use a Python prior to 3.7?

cottsay commented 3 years ago

My initial comment was incorrect - it was introduced in Python 3.9, so anything earlier than that will need the package. Which is pretty much all supported platforms.

kyrofa commented 3 years ago

Ahhh, now I'm less confused :) .