New python bindings #1087

rjarry · 2020-05-18T12:57:12Z

Hi @michalvasko @rkrejci @Dajvid,

As promised, here is a first draft of the integration of https://github.com/rjarry/libyang-cffi.

Few notes:

I tried to fit in with cmake as much as possible but I am not very proficient with it. There may be atrocities to fix.
Python distutils/setuptools command line interface is horrible. The only "sane" way to ensure that arguments are taken into account is to use a custom "user configuration file" (generated .pydistutils.cfg) and force a value to HOME during the build so that distutils/setuptools load it. This approach was borrowed from Piotr Ożarowski's pybuild which is used extensively in Debian and Ubuntu to generate Python .deb packages.

What is missing:

Write actual examples
More unit tests?
API functions not exported
Python doc strings
Integration in the packages subfolder (for automatic upload to pypi.org)

What do you think?

lgtm-com · 2020-05-18T13:04:14Z

This pull request introduces 1 alert when merging bb7695a into 9267156 - view on LGTM.com

new alerts:

1 for Unused import

python/cffi/source.c

rjarry · 2020-05-18T14:10:42Z

As I expected, the static build does not work...

I'll disable it for the time being.

packages/CMakeLists.txt

python/CMakeLists.txt

michalvasko · 2020-05-18T14:53:09Z

So source.c contains all the internal functions you needed directly in the bindings?

rjarry · 2020-05-18T14:59:20Z

So source.c contains all the internal functions you needed directly in the bindings?

Yes. Most of them are trivial and could be implemented directly in python. However, for the sake of performance, I did some of the work in C. Maybe there are some that could be integrated in the C library:

https://github.com/CESNET/libyang/pull/1087/files#diff-8b2dff95a233a8f66dea33417af39c83R9
https://github.com/CESNET/libyang/pull/1087/files#diff-8b2dff95a233a8f66dea33417af39c83R43
https://github.com/CESNET/libyang/pull/1087/files#diff-8b2dff95a233a8f66dea33417af39c83R105

The most critical one (I guess) is ly_toprint() which is mandatory for the python dict data format. Would it be possible to export this function in the API? Or maybe another version of it.

https://github.com/CESNET/libyang/pull/1087/files#diff-8b2dff95a233a8f66dea33417af39c83R251
https://github.com/CESNET/libyang/pull/1087/files#diff-8b2dff95a233a8f66dea33417af39c83R131
https://github.com/CESNET/libyang/pull/1087/files#diff-bb57130e2bf44e477f946061dda45891R235

rjarry · 2020-05-19T12:39:02Z

@michalvasko would that be OK to export lyd_toprint() in the public symbols for libyang.so?

Also, would that be OK to add one utility function:

/**
 * @brief Return the data path with list key placeholders.
 *
 * @param[in] node Schema node for which to get the data path.
 * @param[in] placeholder String that will be inserted in place of the list key values.
 * @return NULL on error, on success the buffer for the resulting path is
*          allocated and caller is supposed to free it with free().
 */
char *lys_data_path_pattern(const struct lys_node *node, const char *placeholder);

Example of use for the Python bindings:

p = lib.lys_data_path_pattern(self._snode, b'"%s"')
p = ffi.string(p).decode('utf-8')
# /module1:container/module2:list[key1="%s"]/sublist[key2="%s"]/leaf
p %= ('val1', 'val2')
# /module1:container/module2:list[key1="val1"]/sublist[key2="val2"]/leaf

p = lib.lys_data_path_pattern(self._snode, b'%r')
p = ffi.string(p).decode('utf-8')
# /module1:container/module2:list[key1=%r]/sublist[key2=%r]/leaf
p %= ('val1', 'val2')
# /module1:container/module2:list[key1='val1']/sublist[key2='val2']/leaf

p = lib.lys_data_path_pattern(self._snode, b'{!r}')
p = ffi.string(p).decode('utf-8')
# /module1:container/module2:list[key1={!r}]/sublist[key2={!r}]/leaf
p = p.format('val1', 'val2')
# /module1:container/module2:list[key1='val1']/sublist[key2='val2']/leaf

rjarry · 2020-05-20T08:55:00Z

@michalvasko I have made some additional commits to address what I mentioned about new functions in the C library. Please tell me if you prefer that I push them into a separate pull request.

Also, I'd like to support ENABLE_STATIC in the python bindings but there is an link error. I am not sure how to fix it:

x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -g -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -I/tmp/build/python/include -I/usr/include/python3.5m -c /tmp/build/python/temp.linux-x86_64-3.5/_libyang.c -o /tmp/build/python/temp.linux-x86_64-3.5/tmp/build/python/temp.linux-x86_64-3.5/_libyang.o -Werror -std=c99
x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -Wl,-Bsymbolic-functions -Wl,-z,relro -g -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 /tmp/build/python/temp.linux-x86_64-3.5/tmp/build/python/temp.linux-x86_64-3.5/_libyang.o -L/tmp/build -lyang -o /tmp/build/python/lib.linux-x86_64-3.5/_libyang.cpython-35m-x86_64-linux-gnu.so
/usr/bin/ld: /tmp/build/libyang.a(common.c.o): relocation R_X86_64_32 against `.rodata' can not be used when making a shared object; recompile with -fPIC
/tmp/build/libyang.a: error adding symbols: Bad value
collect2: error: ld returned 1 exit status

... for schema nodes. Refs #1087 Signed-off-by: Robin Jarry <robin.jarry@6wind.com>

michalvasko · 2020-05-20T09:14:41Z

Hi,
sorry it took me so long to get to this, I do not know what to do first. But at least you nicely refactored it since I last saw it so I have added the 2 functions into libyang, please pull it and adjust your code accordingly. As for the static build, I guess it is not really a priority but I will try to look at that. No idea when that will be but it should not be blocking your work, I hope.

Regards,
Michal

rjarry · 2020-05-20T09:38:13Z

Thanks Michal, I rebased on the latest devel and changed the python code accordingly.

rkrejci · 2020-05-21T08:08:15Z

Hi, would it be possible to keep also the SWIG bindings as an option? There are some ongoing projects already using this API (even our own libnetconf2) and I don't want to break them. I think that a good thing for this is your change of the Python package name, so they can be distinguished. They can be separated options in CMake and probably even both can be built if required.

michalvasko · 2020-05-21T08:12:00Z

@rkrejci I would find that really confusing. What about adding an option that will allow changing to the old swig bindings, by default the CFFI ones would be compiled and installed?

rkrejci · 2020-05-21T08:20:19Z

@michalvasko That's probably what I'm proposing - having an extra option for SWIG as well as for CFFI bindings, the default one can be the CFFI (as it is now set in the PR) and if required the SWIG Python bindings can be enabled the same way as e.g. the java or javascript bindings.

But currently, the SWIG Python bindings are being removed completely.

michalvasko · 2020-05-21T08:26:07Z

Right, not allowing to build both bindings was my main point.

rkrejci · 2020-05-21T08:34:26Z

@michalvasko well, by default. But I don't see any reason why to disable such a setup when I want to build both bindings to support old and new projects depending on different libyang bindings. If I understand it correctly, the old bindings are built as Python yang package while the CFFI bindings are Python libyang package, so they are different even by name. Is it right @rjarry ?

michalvasko · 2020-05-21T08:41:10Z

Like I said, I would simply find it confusing. You could then even use both bindings in a single project, for example. But, as I will likely not be using them or maintaining them, it is not my decision to make.

rjarry · 2020-05-21T08:42:14Z

Hi @rkrejci,

Sure I can leave the SWIG python bindings. However, leaving both bindings enabled by default may be confusing. Building both python bindings should be possible but not the default in my opinion.

The question is: which binding is enabled by default? And with which cmake option?

I am on vacation today, I'll have a closer look tomorrow.

rkrejci · 2020-05-21T09:03:26Z

@rjarry sure, only one of them will be the default, but I'm not sure which one it should be. I probably slightly tend to have SWIG bindings as the default one. They are there for some time already and if someone is already using python bindings, he will expect the SWIG version. On the other hand, in README or somewhere in doc I would strongly recommend trying the CFFI version since it is expected to be more Pythonic (so it should provide better user experience).

rjarry · 2020-05-22T11:35:18Z

@rkrejci I have pushed a new version with SWIG bindings left as they are. The default is to build only the legacy bindings. The new CFFI bindings can be enabled (independently from any other SWIG binding) with GEN_PYTHON_CFFI_BINDINGS.

Following this, I have not modified anything in packages since I don't know where you want to go with this.

rjarry · 2020-05-25T11:38:12Z

Hi Michal,

I have pushed some modifications in the packaging scripts. I only tested the build-deb target on Debian 10 (buster). I did not test the build-rpm target as I don't know what distributions you want to target (and I'm not sure which ones I have at hand).

Also, I have modified .travis.yml to handle automatic uploads on https://pypi.org when new commits are made on the master branch.

rjarry · 2020-05-27T14:25:15Z

Hi @rkrejci @michalvasko,

I have removed the __token__ username argument for twine as you requested.

Did you have any more comments on the changes?

michalvasko · 2020-05-27T15:01:20Z

Hi,
as for me, it seems good, have you actually build the package using these scripts?

Other than that, I suppose you could have come up with better names for the test modules but you have your name there :) Also, maybe some simple examples would be nice.

Regards,
Michal

rjarry · 2020-05-27T16:12:43Z

Hi Michal,

I have built the deb packages only. Not the rpms.

About the test module names, I can change them but I am not sure what names would be more appropriate.

I have put minimalist examples in the README file. I can add other examples if you want but I figured that the unit tests are good examples.

rjarry · 2020-05-27T16:20:53Z

Haha you were talking about YANG module names! I thought you were talking about python files.

Well a bit of fantasy cannot hurt from time to time :)

michalvasko · 2020-05-28T06:52:48Z

Hi,
okay, as far as I am concerned, it can be merged.

Regards,
Michal

rjarry · 2020-05-28T11:38:57Z

Glad to hear that, let me know if there is something to do on my end.

EditorConfig is a file format and collection of text editor plugins for maintaining consistent coding styles between different editors and IDEs. Initialize the file following the current coding style in existing files. In order for this file to be taken into account (unless they use an editor with built-in EditorConfig support), developers will have to install a plugin. Link: https://editorconfig.org/ Link: https://github.com/editorconfig/editorconfig-emacs Link: https://github.com/editorconfig/editorconfig-vim Signed-off-by: Robin Jarry <robin.jarry@6wind.com>

Signed-off-by: Robin Jarry <robin.jarry@6wind.com>

This is the integration of https://github.com/rjarry/libyang-cffi In the long term, it is intended to replace the existing SWIG bindings. Some highlights: * Uses CFFI for interaction with libyang.so. * More "pythonic" API. Should be easier to use for Python developers. * All high-level code is in Python. Should ease maintenance. * Virtualenv/pip friendly. * Schema diff feature. * New "print" data format: python dict. To enable the generation, add the -DGEN_PYTHON_CFFI_BINDINGS=ON cmake option. It does not depend on SWIG nor on the CPP bindings. It may be enabled even if -DGEN_LANGUAGE_BINDINGS is OFF. Signed-off-by: Robin Jarry <robin.jarry@6wind.com>

rjarry · 2020-06-01T13:11:27Z

Hi Michal,

I have fixed a minor problem. Seen by code review. Also, I rebased the commits on the latest devel.

When do you think this can be merged?

rkrejci · 2020-06-01T15:05:14Z

Hi Robin, sorry, I'm still blocking it wanting to look at that.

I tried to compile and import the module, but I'm getting the following error. Am I doing something wrong? The build process seemed fine to me.

>>> import libyang
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: dynamic module does not define module export function (PyInit_libyang)

rjarry · 2020-06-01T15:48:53Z

Hi Radek,

it looks like you have the C libyang.so library file somewhere into $PYTHONPATH (or in the current directory). Since .so files are used in priority over standard python modules, the interpreter tries to load libyang.so as a compiled Python extension which obviously fails since it is a C library.

Maybe you are executing python3 from inside the build dir?

rjarry · 2020-06-01T20:18:30Z

If you want to try the bindings without installing them you need to export the proper env vars. As it is done for the test commands:

https://github.com/6WIND/libyang/blob/6e6475d8d5ad175c78d523b73a0304ddc59e116b/python/CMakeLists.txt#L77-L80

rkrejci · 2020-06-02T06:20:56Z

ok, I fine with it and going to merge it

rkrejci · 2020-06-02T06:37:34Z

Thanks for your work, Robin!

rjarry · 2020-06-02T07:03:17Z

Thanks Radek! And thank you Michal for the review!

rjarry · 2020-06-02T07:13:37Z

@rkrejci @michalvasko it looks like the deployment does not work: https://travis-ci.org/github/CESNET/libyang/jobs/693729586#L3715-L3832

There are multiple errors:

https://travis-ci.org/github/CESNET/libyang/jobs/693729586#L3737-L3738

https://travis-ci.org/github/CESNET/libyang/jobs/693729586#L3812-L3817

https://travis-ci.org/github/CESNET/libyang/jobs/693729586#L3829

I'm sorry about that, it looks like it is because of commit f7a859a. I don't know how to test this and fix it properly. Can you help?

rjarry · 2020-06-02T07:21:19Z

The twine error seems related to this: pypa/twine#342

rjarry · 2020-06-02T07:27:38Z

The codecov error seems related to this: codecov/codecov-python#261

michalvasko · 2020-06-02T07:31:42Z

Hi,
yes, everything broke for various reasons :( You think you can fix at least the twine problem? codecov is not that important right now.

Regards,
Michal

rjarry · 2020-06-02T07:32:58Z

Yes I'll submit another PR with the fix.

rjarry commented May 18, 2020

View reviewed changes

python/cffi/source.c Outdated Show resolved Hide resolved

rjarry force-pushed the python branch from bb7695a to 952ff63 Compare May 18, 2020 13:11

michalvasko reviewed May 18, 2020

View reviewed changes

packages/CMakeLists.txt Outdated Show resolved Hide resolved

michalvasko reviewed May 18, 2020

View reviewed changes

packages/CMakeLists.txt Outdated Show resolved Hide resolved

michalvasko reviewed May 18, 2020

View reviewed changes

python/CMakeLists.txt Outdated Show resolved Hide resolved

rjarry changed the base branch from devel to cffi_bindings May 19, 2020 12:55

rjarry force-pushed the python branch from 30e1d81 to bfc2d49 Compare May 20, 2020 08:39

michalvasko added a commit that referenced this pull request May 20, 2020

schema tree FETAURE function for getting data path format string pattern

c831064

... for schema nodes. Refs #1087 Signed-off-by: Robin Jarry <robin.jarry@6wind.com>

rjarry force-pushed the python branch from bfc2d49 to a365ad9 Compare May 20, 2020 09:29

rjarry changed the base branch from cffi_bindings to devel May 20, 2020 09:29

rjarry force-pushed the python branch from a365ad9 to 445aa31 Compare May 20, 2020 09:46

rjarry marked this pull request as ready for review May 20, 2020 10:57

rjarry force-pushed the python branch from 445aa31 to 26173d6 Compare May 22, 2020 11:13

rjarry force-pushed the python branch 2 times, most recently from e0801b0 to a1294ec Compare May 25, 2020 11:27

rjarry force-pushed the python branch from a1294ec to dd3e691 Compare May 27, 2020 14:23

rjarry force-pushed the python branch from dd3e691 to 76e16b7 Compare June 1, 2020 13:09

rjarry added 3 commits June 1, 2020 15:09

travis: use deploy section

f7a859a

Signed-off-by: Robin Jarry <robin.jarry@6wind.com>

rjarry force-pushed the python branch from 76e16b7 to 6e6475d Compare June 1, 2020 13:10

rkrejci merged commit 82d7e33 into CESNET:devel Jun 2, 2020

rjarry deleted the python branch June 2, 2020 07:03

rjarry mentioned this pull request Jun 17, 2020

Python Bindings : Xpath_Ctx - Should it be used with Python? sysrepo/sysrepo#2021

Closed

New python bindings #1087

New python bindings #1087

Conversation

rjarry commented May 18, 2020 • edited

lgtm-com bot commented May 18, 2020

rjarry commented May 18, 2020

michalvasko commented May 18, 2020

rjarry commented May 18, 2020

rjarry commented May 19, 2020 • edited

rjarry commented May 20, 2020

michalvasko commented May 20, 2020 • edited

rjarry commented May 20, 2020

rkrejci commented May 21, 2020

michalvasko commented May 21, 2020

rkrejci commented May 21, 2020

michalvasko commented May 21, 2020

rkrejci commented May 21, 2020

michalvasko commented May 21, 2020

rjarry commented May 21, 2020

rkrejci commented May 21, 2020

rjarry commented May 22, 2020

rjarry commented May 25, 2020

rjarry commented May 27, 2020

michalvasko commented May 27, 2020

rjarry commented May 27, 2020

rjarry commented May 27, 2020

michalvasko commented May 28, 2020

rjarry commented May 28, 2020

rjarry commented Jun 1, 2020

rkrejci commented Jun 1, 2020

rjarry commented Jun 1, 2020 • edited

rjarry commented Jun 1, 2020 • edited

rkrejci commented Jun 2, 2020

rkrejci commented Jun 2, 2020

rjarry commented Jun 2, 2020

rjarry commented Jun 2, 2020 • edited

rjarry commented Jun 2, 2020

rjarry commented Jun 2, 2020

michalvasko commented Jun 2, 2020

rjarry commented Jun 2, 2020

rjarry commented May 18, 2020 •

edited

rjarry commented May 19, 2020 •

edited

michalvasko commented May 20, 2020 •

edited

rjarry commented Jun 1, 2020 •

edited

rjarry commented Jun 1, 2020 •

edited

rjarry commented Jun 2, 2020 •

edited