-
Notifications
You must be signed in to change notification settings - Fork 201
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
Conversation
fffd856
to
b76bfdf
Compare
8249583
to
e646fe5
Compare
04e35fe
to
8fd5adf
Compare
There was a problem hiding this 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.
if not next_value: | ||
raise StopIteration | ||
content = next_value | ||
# XXX: Introduce getters for .address/.lifetime/.label, to avoid |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
And make the CI tests pass, using the new 'netplan' Python module bindigns.
Thanks @daniloegea! I've rebased and addressed most of your remarks. I still need to improve my comments/description in |
There was a problem hiding this 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.
Thanks, I've addressed all of your comments. CI failures about Azure kernels are unrelated (a basic local autopkgtest passed): Let's merge this! |
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/
: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 tolibnetplan.so
itself. In theffibuilder.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. Inpython-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
make check
successfully.make check-coverage
).in Launchpad: FR-2780