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

Add Python bindings for libnetplan #385

Merged
merged 9 commits into from
Aug 16, 2023
Merged

Add Python bindings for libnetplan #385

merged 9 commits into from
Aug 16, 2023

Conversation

slyon
Copy link
Collaborator

@slyon slyon commented Jul 26, 2023

Description

Implementation of public Python bindings for libnetplan.so, using CFFI in API mode, as described in spec FO0114. The diff is fairly big, but it contains lots of refactorings, effectively moving 700 LOC of our old/internal ctypes-bindings over into a public module and porting them to make use of CFFI. Going through the commits one by one should make the review manageable:

8184c2b parse-nm: improve some error passing
59ee33c CI: Add python3-cffi build-dependency
032e356 include: Extend public API functions
9a84892 src: make internal API functions/structures available
4bfe29b bindings: python-cffi build skeleton
fdad4e0 bindings: Initial implementation of CFFI Python bindings
40c4f47 cli:tools:test: Make use of new cffi python bindings
a21d9f4 cli: drop legacy/internal ctypes bindings, now unused
8fd5adf examples: Add an example of how to use Netplan's CFFI bindings

The most interesting bits are porbably those commits, where the new module is introduced, to be installed into usr/lib/python3/dist-packages/netplan/:

  • 4bfe29b bindings: python-cffi build skeleton
  • fdad4e0 bindings: Initial implementation of CFFI Python bindings

We're using the file in python-cffi/netplan/_build_cffi.py to build a _netplan_cffi.cpython-311-x86_64-gnu.so (internal) binary module, that is linked to libnetplan.so itself. In the ffibuilder.cdef(...) we declare any functions/constants/structures that we want to make available from libnetplan.so to the Python bindings. The compiler will match those against Netplan's headers at build time. In python-cffi/netplan/{parser,state,netdef}.py we're then making use of those declarations to define our high-level, Python-native bindings.

We're still making some limited use of ctypes in our tests (test_libnetplan.py) in order to validate some raw library functionality, which are not necessarily exposed through the Python bindings.

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad: FR-2780

@slyon slyon force-pushed the bindings branch 9 times, most recently from 04e35fe to 8fd5adf Compare August 10, 2023 10:25
@slyon slyon marked this pull request as ready for review August 10, 2023 10:33
@slyon slyon requested a review from daniloegea August 10, 2023 10:33
@slyon slyon mentioned this pull request Aug 10, 2023
5 tasks
Copy link
Collaborator

@daniloegea daniloegea left a comment

Choose a reason for hiding this comment

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

Nice work! Implementing bindings with cffi is much nicer than ctypes as far as could see.

In my opinion this could be merged. I just added a few non-blocking comments. But maybe you'd like to address some of them, such as a deprecation warning from meson and including veths and dummies in the netdef class.

python-cffi/netplan/meson.build Outdated Show resolved Hide resolved
python-cffi/netplan/meson.build Outdated Show resolved Hide resolved
python-cffi/netplan/meson.build Outdated Show resolved Hide resolved
python-cffi/netplan/__init__.py Outdated Show resolved Hide resolved
python-cffi/netplan/netdef.py Outdated Show resolved Hide resolved
if not next_value:
raise StopIteration
content = next_value
# XXX: Introduce getters for .address/.lifetime/.label, to avoid
Copy link
Collaborator

Choose a reason for hiding this comment

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

thought: hmm it will be even worse with routes...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes.. we should build proper API around it, eventually. And keep it internal until that's done.

python-cffi/netplan/state.py Outdated Show resolved Hide resolved
examples/cffi-bindings.py Show resolved Hide resolved
@slyon
Copy link
Collaborator Author

slyon commented Aug 15, 2023

Thanks @daniloegea! I've rebased and addressed most of your remarks. I still need to improve my comments/description in examples/cffi-bindings.py and python-cffi/netplan/meson.build but maybe you could already double-check the logical changes I did so far.

@slyon slyon requested a review from daniloegea August 15, 2023 16:05
Copy link
Collaborator

@daniloegea daniloegea left a comment

Choose a reason for hiding this comment

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

Looks good!

Only one thing I'd like to be changed before merging: the new gcovr.cfg file probably should use the filter configuration to include the source code instead of exclude with a hardcoded build directory. The test coverage-c will fail if you use a different build directory.

gcovr.cfg Outdated Show resolved Hide resolved
@slyon
Copy link
Collaborator Author

slyon commented Aug 16, 2023

Thanks, I've addressed all of your comments. CI failures about Azure kernels are unrelated (a basic local autopkgtest passed):
E: Package 'linux-modules-extra-5.15.0-1042-azure' has no installation candidate

Let's merge this!

@slyon slyon merged commit 7c5f2dc into canonical:main Aug 16, 2023
10 of 13 checks passed
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.

2 participants