ros2 / ros2cli

ROS 2 command line interface tools
Apache License 2.0
166 stars 158 forks source link

Replace unmaintained `netifaces` library to avoid local wheel builds #875

Closed lalten closed 6 months ago

lalten commented 6 months ago

netifaces is unmaintained: https://github.com/al45tair/netifaces/issues/78. It does not provide official prebuilt wheels for Python > 3.9: https://pypi.org/project/netifaces/#files

https://pypi.org/project/psutil/ is an alternative (available in rosdep as python3-psutil) but the interface is a little different.

The reason I'm making this change is that we want to upgrade to a Python>3.9 but not provide a compiler inside our CI container (Unrelated to this PR, but we're using Bazel with a standalone hermetic compiler toolchain and https://github.com/mvukov/rules_ros2). Not having a host compiler for Python wheel builds makes it more difficult to build netifaces.

There are only two netifaces usages in ros2cli:

LocalXMLRPCServer It just needs a list of available IPv4s (only AF_INET, no AF_INET6). ifaddr can provide a one-line replacement for the get_local_ipaddrs() function:

❯ ipython3
Python 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0]
Type 'copyright', 'credits' or 'license' for more information
IPython 7.31.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import netifaces, psutil, socket

In [2]: def get_local_ipaddrs():
   ...:     iplist = []
   ...:     interfaces = netifaces.interfaces()
   ...:     for interface in interfaces:
   ...:         addrs = netifaces.ifaddresses(interface)
   ...:         if netifaces.AF_INET in addrs.keys():
   ...:             for value in addrs[netifaces.AF_INET]:
   ...:                 iplist.append(value['addr'])
   ...:     return iplist
   ...:

In [3]: get_local_ipaddrs()
Out[3]: ['127.0.0.1', '172.16.103.157', '172.17.0.1']

In [4]: [addr.address for _, addrs in psutil.net_if_addrs().items() for addr in addrs if addr.family == socket.AF_INET]
Out[4]: ['127.0.0.1', '172.16.103.157', '172.17.0.1']

NetworkAwareNode It must restart if network interfaces change. Currently it's checking for changes by comparing a (default)dict containing netiface's data. In my ifaddr replacement it will instead compare a set of repr-strings that contain all data ifaddr provides.

❯ ipython3
Python 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0]
Type 'copyright', 'credits' or 'license' for more information
IPython 7.31.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import netifaces, psutils, functools

In [2]: from collections import defaultdict

In [3]: def get_interfaces_ip_addresses():
   ...:     addresses_by_interfaces = defaultdict(functools.partial(defaultdict, set))
   ...:     for interface_name in netifaces.interfaces():
   ...:         for kind, info_list in netifaces.ifaddresses(interface_name).items():
   ...:             for info in info_list:
   ...:                 addresses_by_interfaces[kind][interface_name].add(info['addr'])
   ...:     print(f'Addresses by interfaces: {addresses_by_interfaces}')
   ...:     return addresses_by_interfaces
   ...:

In [4]: get_interfaces_ip_addresses()
Addresses by interfaces: defaultdict(..., {17: defaultdict(<class 'set'>, {'lo': {'00:00:00:00:00:00'}, 'enp5s0': {'d4:5d:64:xx:xx:xx'}, 'docker0': {'02:42:01:8e:b3:0b'}}), 2: defaultdict(<class 'set'>, {'lo': {'127.0.0.1'}, 'enp5s0': {'172.16.103.157'}, 'docker0': {'172.17.0.1'}}), 10: defaultdict(<class 'set'>, {'lo': {'::1'}, 'enp5s0': {'fe80::5c44:433c:8cf8:a7c7%enp5s0'}, 'docker0': {'fe80::42:1ff:fe8e:b30b%docker0'}})})
Out[4]:
defaultdict(functools.partial(<class 'collections.defaultdict'>, <class 'set'>),
            {17: defaultdict(set,
                         {'lo': {'00:00:00:00:00:00'},
                          'enp5s0': {'d4:5d:64:xx:xx:xx'},
                          'docker0': {'02:42:01:8e:b3:0b'}}),
             2: defaultdict(set,
                         {'lo': {'127.0.0.1'},
                          'enp5s0': {'172.16.103.157'},
                          'docker0': {'172.17.0.1'}}),
             10: defaultdict(set,
                         {'lo': {'::1'},
                          'enp5s0': {'fe80::5c44:433c:8cf8:a7c7%enp5s0'},
                          'docker0': {'fe80::42:1ff:fe8e:b30b%docker0'}})})

In [5]: psutil.net_if_addrs()
Out[5]:
{'lo': [snicaddr(family=<AddressFamily.AF_INET: 2>, address='127.0.0.1', netmask='255.0.0.0', broadcast=None, ptp=None),
  snicaddr(family=<AddressFamily.AF_INET6: 10>, address='::1', netmask='ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff', broadcast=None, ptp=None),
  snicaddr(family=<AddressFamily.AF_PACKET: 17>, address='00:00:00:00:00:00', netmask=None, broadcast=None, ptp=None)],
 'enp5s0': [snicaddr(family=<AddressFamily.AF_INET: 2>, address='172.16.103.157', netmask='255.255.252.0', broadcast='172.16.103.255', ptp=None),
  snicaddr(family=<AddressFamily.AF_INET6: 10>, address='fe80::5c44:433c:8cf8:a7c7%enp5s0', netmask='ffff:ffff:ffff:ffff::', broadcast=None, ptp=None),
  snicaddr(family=<AddressFamily.AF_PACKET: 17>, address='d4:5d:64:57:12:27', netmask=None, broadcast='ff:ff:ff:ff:ff:ff', ptp=None)],
 'docker0': [snicaddr(family=<AddressFamily.AF_INET: 2>, address='172.17.0.1', netmask='255.255.0.0', broadcast='172.17.255.255', ptp=None),
  snicaddr(family=<AddressFamily.AF_INET6: 10>, address='fe80::42:1ff:fe8e:b30b%docker0', netmask='ffff:ffff:ffff:ffff::', broadcast=None, ptp=None),
  snicaddr(family=<AddressFamily.AF_PACKET: 17>, address='02:42:01:xx:xx:xx', netmask=None, broadcast='ff:ff:ff:ff:ff:ff', ptp=None)]}

Note that I haven't tested this, but this should work fine on Windows as well.

lalten commented 6 months ago

So 2 years ago, in #687, I switched ros2doctor from using python3-ifcfg to using python3-psutil for network configuration. If it is possible, I think we should prefer to switch to using python3-psutil here to reduce our dependencies. @lalten can you take a look at the python3-psutil APIs and see if we can accomplish the same thing?

Oh very cool, I had no idea psutil could do network things. I will give it a try. If that works out we also don't need https://github.com/ros/rosdistro/pull/39511

lalten commented 6 months ago

I changed to psutil, it works great and is very compatible to previous behavior. Would be happy about a review @clalancette :)

clalancette commented 6 months ago

CI:

lalten commented 6 months ago

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Looks green! Can this be merged then?

Is it possible to land this in the next Humble patch release as well?

fujitatomoya commented 6 months ago

@lalten thanks for the contribution.

clalancette commented 6 months ago

Is it possible to land this in the next Humble patch release as well?

I'd like to keep this in Rolling for a while before we do that, to see if any problems come up.

Even so, I actually don't see a compelling reason to do it. netifaces is available in Ubuntu 22.04 (where Humble is supported), and will remain so for the lifetime of the distribution. So I guess I just don't see the need to make this lateral move and risk regressions.