Python #2135

markus2330 · 2018-07-28T21:30:35Z

Basics

Do not describe the purpose here but:

Short descriptions should be in the release notes (added as entry in
doc/news/_preparation_next_release.md which contains *(my name)*)
Please always add something to the the release notes.
Longer descriptions should be in documentation or in design decisions.
Describe details of how you changed the code in commit messages
(first line should have module: short statement syntax)
References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

I added unit tests
I ran all tests locally and everything went fine
affected documentation is fixed
I added code comments, logging, and assertions (see doc/CODING.md)
meta data is updated (e.g. README.md of plugins)

Review

Remove the line below and add the "work in progress" label if you do
not want the PR to be reviewed:

@markus2330 please review my pull request

markus2330 · 2018-07-28T21:32:26Z

@tom-wa this should make the python plugin stable. I have not tested it, though.

markus2330 · 2018-07-29T04:43:07Z

@ingwinlu Do you know how we can make the python unit tests locate their respective "kdb module" (both version 2 and 3). I still do not understand why we do not install Elektra to /usr/local, wouldn't that be much easier? Then such things would work out of the box.

markus2330 · 2018-07-29T07:06:32Z

@e1528532 did I use pluginprocess correctly?

I do not quite understand why ELEKTRA_PLUGINPROCESS_OPEN gets implicitely called but not ELEKTRA_PLUGINPROCESS_CLOSE? And in line 384 of src/libs/pluginprocess/pluginprocess.c, shouldn't that be ELEKTRA_PLUGINPROCESS_CLOSE instead of ELEKTRA_PLUGIN_CLOSE?

markus2330 · 2018-07-30T15:58:13Z

jenkins build all please

markus2330 · 2018-07-30T16:29:44Z

@sanssecours Do you have any idea why "swig3.0 -c++ -python -external-runtime runtime.h" does not produce runtime.h under Mac OS X? Or is there some other reason why https://travis-ci.org/ElektraInitiative/libelektra/jobs/409880622 fails?

@ingwinlu Seems like "Pull docker images" hangs?

ingwinlu · 2018-07-30T17:44:51Z

nothing hangs. 36 builds in queue as we have taken down v2 for benchmarking one of our plugins.

markus2330 · 2018-07-30T17:58:50Z

nothing hangs

?

36 builds in queue

How does the build server come up with this high number, these are much more jobs than PRs which had activity today.

Would be great if this PR can be merged today, I need Debian packages of it and unfortunately there is no easy way to create Debian packages without merging to master.

ingwinlu · 2018-07-30T18:01:48Z

How does the build server come up with this high number, these are much more jobs than PRs which had activity today.

because our builds run in parallel. each parallel stage adds 1 to the number.

I need Debian packages of it

pretty sure master is broken since the yaml merge.

unfortunately there is no easy way to create Debian packages without merging to master.

that was a compromise to not have even more builds that the system can't handle. It can be rewritten to build all the time with the obvious downside of slowing down the overworked system even more.

markus2330 · 2018-07-30T18:06:06Z

that was a compromise to not have even more builds that the system can't handle.

I thought the multiple repos are the problem. (PRs should not overwrite the packages of master.)

It can be rewritten to build all the time with the obvious downside of slowing down the overworked system even more.

This can be fixed with more hardware. When v2 is running we usually do not have any problems anyway.

sanssecours · 2018-07-31T05:29:24Z

Do you have any idea why "swig3.0 -c++ -python -external-runtime runtime.h" does not produce runtime.h under Mac OS X?

Nope, sorry.

Or is there some other reason why https://travis-ci.org/ElektraInitiative/libelektra/jobs/409880622 fails?

Not as far as I can tell.

markus2330 · 2018-07-31T07:39:13Z

Nope, sorry.

What happens when the above command is executed? Maybe we use a wrong version of swig? With "3.0.10" on Linux it works without problems.

The alternative would be to disable the python plugins for Mac OS X...

sanssecours · 2018-07-31T08:04:56Z

What happens when the above command is executed? Maybe we use a wrong version of swig?

The Homebrew formula of SWIG does not include a binary called swig3.0:

find (brew --cellar swig) -regex '.*bin.*'
#> /usr/local/Cellar/swig/3.0.12/bin
#> /usr/local/Cellar/swig/3.0.12/bin/swig
#> /usr/local/Cellar/swig/3.0.12/bin/ccache-swig

.

The alternative would be to disable the python plugins for Mac OS X...

An easier alternative would be to just remove the Python configuration from .travis.yml.

markus2330 · 2018-07-31T08:17:30Z

The Homebrew formula of SWIG does not include a binary called swig3.0

Might also be called differently, SWIG_EXECUTABLE of CMakeCache.txt should tell you which one is used.

An easier alternative would be to just remove the Python configuration from .travis.yml.

Even if we know it does not compile? Its status reads "maintained unittest configurable global memleak" so people would not expect it to not work.

sanssecours · 2018-07-31T08:33:59Z

What happens when the above command is executed?

The command swig -c++ -python -external-runtime runtime.h executed inside src/bindings/swig/python works on my machine. The command produces the following file (without the extension .txt): runtime.h.txt.

Even if we know it does not compile? Its status reads "maintained unittest configurable global memleak" so people would not expect it to not work.

In that case let us also disable the Python binding on macOS. As it stands we do not have a maintainer for the macOS version of the binding anyway.

markus2330 · 2018-07-31T09:10:39Z

Ok, then let us disable the python binding for the GCC build in Mac OSX. Thank you for your investigation!

e1528532

I do not quite understand why ELEKTRA_PLUGINPROCESS_OPEN gets implicitely called but not ELEKTRA_PLUGINPROCESS_CLOSE?

I think the reason was that i already return an boolean (represented as int) in the close function but i also need the result of the "remote call" in that context, so the function would have had to return two values. thinking about it again one could probably just use a tuple (probably a struct in c) to return both values.

And in line 384 of src/libs/pluginprocess/pluginprocess.c, shouldn't that be ELEKTRA_PLUGINPROCESS_CLOSE instead of ELEKTRA_PLUGIN_CLOSE?

yes you are right i also used ELEKTRA_PLUGINPROCESS_CLOSE in the haskell plugin, please feel free to correct the issue

Regarding the fix for the original key name issue, in theory it looks fine to me apart from a few minor things. i haven't tried it out yet though on my own machine, i assume it should work?

e1528532 · 2018-07-31T11:10:08Z

doc/news/_preparation_next_release.md

@@ -164,7 +164,7 @@ Thanks to Daniel Bugl.
 ### Interpreter Plugins

 - The plugins ruby, python and jni can now also be mounted as global plugin.
- Fix crash in python plugin. *(Markus Raab)*
+- Fix crash in python plugin by using process plugin. *(Markus Raab)*


the library is currently called pluginprocess, the process plugin is the plugin that uses this library for generic wrapping of other plugins instead. This could probably be misleading therefore.

Thank you! My initial plan was to use the process plugin but this changed now.

Can you maybe create a new PR fixing the pluginprocess #2142? If it is useful for you can cherry-pick my commits in this PR.

e1528532 · 2018-07-31T11:11:04Z

src/libs/pluginprocess/pluginprocess.c

+static Key * popParent (KeySet * ks)
+{
+	Key * parentKey = ksLookupByName (ks, "/pluginprocess/parent", KDB_O_POP);
+	Key * parentNameKey = ksLookupByName (ks, "/pluginprocess/parent/name", KDB_O_POP);


minor thing but above the variable is called nameKey and here its parentNameKey, i'd unify that

e1528532 · 2018-07-31T11:18:30Z

src/plugins/python/python.cpp

+		if (!elektraPluginProcessIsParent (pp)) elektraPluginProcessStart (handle, pp);
+	}
+
+	moduleData * data = static_cast<moduleData *> (elektraPluginGetData (handle));


minor optimization: store the result of elektraPluginGetData before the if, so you don't need to call the function again here like its done in the haskell plugin

e1528532 · 2018-07-31T11:20:08Z

src/plugins/python/python.cpp

+		moduleData * md = createModuleData (handle);
+		if (!md)
+		{
+			if (ksLookupByName (elektraPluginGetConfig (handle), "/module", 0) != nullptr)


in case the creation of md fails, pp (pluginprocess) is not cleaned up. you should call elektraPluginProcessClose for that.

e1528532 · 2018-07-31T11:22:32Z

src/plugins/python/python.cpp

+	if (elektraPluginProcessIsParent (pp))
+	{
+		int result = elektraPluginProcessSend (pp, ELEKTRA_PLUGINPROCESS_CLOSE, NULL, errorKey);
+		if (elektraPluginProcessClose (pp, errorKey)) elektraPluginSetData (handle, NULL);


is it a good idea to set the plugin's handle to null? this only means the pluginprocess struct (pp) got cleaned up and could be removed from the plugin's other data (in this plugin it seems to be this moduleData struct, so make moduleData.pp null instead and cleanup the rest of the plugin at a central point.

It was copied from the example. Shouldn't elektraPluginProcessClose be called in both parent & child?

well the child process gets killed anyway so it shouldn't make much of a difference

ok the child does call cleanupPluginData at the end of elektraPluginProcessStart so it should be fine.

markus2330 · 2018-07-31T13:06:48Z

please feel free to correct the issue

It would be great if you can do the necessary fixes processplugin.

i haven't tried it out yet though on my own machine, i assume it should work?

It improves one specific problem but I am afraid there are other problems with the current protocol. The library also needs more unit tests.

e1528532 · 2018-08-02T12:19:23Z

regarding elektraPluginProcessClose not implicitly sending the ELEKTRA_PLUGINPROCESS_CLOSE command due to the necessity of a tuple containing (result, cleanedUp) otherwise, should i change this or leave it like it is? It would require changing the header.

to make module/plugin loading more robust

see #2142

…s in its own process now, doesn't matter anymore)

markus2330 · 2018-08-07T17:40:23Z

Finally it is done 👍

If someone is interested why there are two travis jobs, one is because of "Build pushed branches" and the other because of the pull request (branches merged into master).

markus2330 force-pushed the python branch from 8095686 to 532666e Compare July 28, 2018 23:05

markus2330 force-pushed the python branch from 4236934 to e4b967f Compare July 29, 2018 06:58

markus2330 requested review from ingwinlu and e1528532 July 29, 2018 07:03

markus2330 force-pushed the python branch 4 times, most recently from 92962fa to 231a275 Compare July 30, 2018 14:37

markus2330 mentioned this pull request Jul 30, 2018

pluginprocess: transfer of parentKey #2142

Closed

markus2330 force-pushed the python branch from 231a275 to eb3f7e1 Compare July 31, 2018 09:10

markus2330 force-pushed the python branch from 83da295 to 5f21f15 Compare July 31, 2018 09:55

e1528532 requested changes Jul 31, 2018

View reviewed changes

markus2330 mentioned this pull request Aug 1, 2018

Writing a Plugin README.md #2145

Closed

python: fix installation of test_data

87f52bf

markus2330 force-pushed the python branch 5 times, most recently from e863158 to 482d662 Compare August 7, 2018 09:24

Markus Raab and others added 19 commits August 7, 2018 15:53

python: call correct functions

47ccd40

python: small improvements

a98c541

core: remove outdated license disclaimer

84b1d6a

python: add module paths

1db3222

to make module/plugin loading more robust

python: fix cmake paths

5220c4f

cmake: hide some variables

d6063ef

python2: disable gcc under mac os X

aee7db8

pluginprocess: handle parentKey separately

717358f

see #2142

python: partly fix of review

c6361b9

python: use the improved pluginprocess implementation

b4e0188

python: make pluginprocess and python work, remove obsolete locks (it…

6555d38

…s in its own process now, doesn't matter anymore)

python: adjust pluginprocess usage

52d91a5

python: don't init pluginprocess if config invalid

30df675

python: fix segfaults

30fa462

python: format source

775933f

python: update README.md (removed shutdown)

a51b109

python: fix python site-packages detection

ca991b5

jenkinsfile: add env to ease debugging

350b583

python: remove not needed path

4ebb223

markus2330 force-pushed the python branch from 482d662 to 4ebb223 Compare August 7, 2018 13:55

python: fix key args in tests

1924f5d

markus2330 merged commit 7c8c9dc into master Aug 7, 2018

markus2330 deleted the python branch August 7, 2018 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python #2135

Python #2135

markus2330 commented Jul 28, 2018

markus2330 commented Jul 28, 2018

markus2330 commented Jul 29, 2018

markus2330 commented Jul 29, 2018

markus2330 commented Jul 30, 2018

markus2330 commented Jul 30, 2018

ingwinlu commented Jul 30, 2018

markus2330 commented Jul 30, 2018

ingwinlu commented Jul 30, 2018

markus2330 commented Jul 30, 2018

sanssecours commented Jul 31, 2018

markus2330 commented Jul 31, 2018

sanssecours commented Jul 31, 2018

markus2330 commented Jul 31, 2018

sanssecours commented Jul 31, 2018

markus2330 commented Jul 31, 2018

e1528532 left a comment

e1528532 Jul 31, 2018

markus2330 Jul 31, 2018

e1528532 Jul 31, 2018

e1528532 Jul 31, 2018

e1528532 Jul 31, 2018

e1528532 Jul 31, 2018

markus2330 Jul 31, 2018

e1528532 Aug 2, 2018

e1528532 Aug 2, 2018

markus2330 commented Jul 31, 2018

e1528532 commented Aug 2, 2018

markus2330 commented Aug 7, 2018

Python #2135

Python #2135

Conversation

markus2330 commented Jul 28, 2018

Basics

Checklist

Review

markus2330 commented Jul 28, 2018

markus2330 commented Jul 29, 2018

markus2330 commented Jul 29, 2018

markus2330 commented Jul 30, 2018

markus2330 commented Jul 30, 2018

ingwinlu commented Jul 30, 2018

markus2330 commented Jul 30, 2018

ingwinlu commented Jul 30, 2018

markus2330 commented Jul 30, 2018

sanssecours commented Jul 31, 2018

markus2330 commented Jul 31, 2018

sanssecours commented Jul 31, 2018

markus2330 commented Jul 31, 2018

sanssecours commented Jul 31, 2018

markus2330 commented Jul 31, 2018

e1528532 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus2330 commented Jul 31, 2018

e1528532 commented Aug 2, 2018

markus2330 commented Aug 7, 2018