Skip to content
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

Replace unmaintained netifaces library to avoid local wheel builds #875

Merged
merged 2 commits into from Jan 12, 2024

Conversation

lalten
Copy link
Contributor

@lalten lalten commented Jan 10, 2024

netifaces is unmaintained: al45tair/netifaces#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.

Signed-off-by: Laurenz Altenmüller <laurenz.altenmueller@fernride.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

lalten added a commit to lalten/rosdistro that referenced this pull request Jan 10, 2024
@lalten
Copy link
Contributor Author

lalten commented Jan 10, 2024

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 ros/rosdistro#39511

Signed-off-by: Laurenz Altenmüller <laurenz.altenmueller@fernride.com>
@lalten
Copy link
Contributor Author

lalten commented Jan 11, 2024

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

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks. I'm happy to get rid of one additional dependency. I'll run full CI on this next.

@clalancette
Copy link
Contributor

CI:

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

@lalten
Copy link
Contributor Author

lalten commented Jan 12, 2024

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 fujitatomoya merged commit 426d6e9 into ros2:rolling Jan 12, 2024
3 checks passed
@fujitatomoya
Copy link
Collaborator

@lalten thanks for the contribution.

@clalancette
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants