GNOME Bugzilla – Bug 634673
Change of files in the c-source of GnuCash may lead to broken build of python-bindings
Last modified: 2018-06-29 22:47:07 UTC
SVN r19798 changed libqof/qof/qofsession.h. This led to breaking the compile of the python-bindings. Details are described in Bug https://bugzilla.gnome.org/show_bug.cgi?id=634165. That bug covers another topic so I opened this bug to further discuss the matter.
I wondered if this problem occurs for everyone. I installed a SVN Revision 19769 and updated to R19795 which is the change in the python-bindings described in Bug 634165. Both compiled without problem. Updating to R19800 broke compilation. Removing gnucash_core.c fixes the problem. But as the discussion on Bug 634165 showed it may only be a workaround, also to include this in the Makefile.
Created attachment 174319 [details] [review] Patch to add dependencies for gnucash_core.c I thought my quick and easy patch wouldn't make it. To cover all the dependencies for gnucash_core.c I've added almost all the dependencies explicitly to Makefile.am. Is there a better way to do this? I'm not a Makefile expert and seems a little inelegant to me.
Comment on attachment 174319 [details] [review] Patch to add dependencies for gnucash_core.c There's no easier way here, so you are quite correct. Thanks for spotting the MAINTAINERCLEANFILES typo. However, gnucash_core.c doesn't depend on the *.c files - just on the *.h files. The patch can go into SVN but without the _gnucash_core_c_dep.
Committed as r19802, without the c file dependencies, the backends, or the gnc C++ bindings for CuteCash. The C++ bindings are redundant, and aren't included in gnucash_core.i. The backends are the subject of bug 634712, for which I should have a fix shortly. In the meantime, I'm leaving this open because the current tests don't really exercise the issue. Christoff or Mike, please test r19802 and report whether this bug is fixed.
This seems to fix the dependencies problem for me at least. Thanks for correcting my errors in the patch Christian. I should have mentioned the MAINTAINERCLEANFILES typo when I submitted.
I tried to install a R19795, compile and install -> works. Update to actual R19805, compile and install -> works. So change and future changes of the c-sources seems to be dealt with.
Created attachment 174430 [details] [review] This patch adds a fake argument to qof_session_begin This patch adds a fake argument to qof_session_begin. This is a try to reflect a change in the number of arguments of a function like it happended in r19798.
(In reply to comment #7) > Created an attachment (id=174430) [details] > This patch adds a fake argument to qof_session_begin > > This patch adds a fake argument to qof_session_begin. This is a try to reflect > a change in the number of arguments of a function like it happended in r19798. When I apply this patch on the situation I described above (R19795 install, compile, update to 19805) and make clean, make install It doesn´t work. So for the moment I redraw the conclusion that everything is fine.
(In reply to comment #8) > (In reply to comment #7) > > Created an attachment (id=174430) [details] [details] > > This patch adds a fake argument to qof_session_begin > > > > This patch adds a fake argument to qof_session_begin. This is a try to reflect > > a change in the number of arguments of a function like it happended in r19798. > > When I apply this patch on the situation I described above (R19795 install, > compile, update to 19805) and make clean, make install It doesn´t work. To be a bit more specific : Gnucash works nut the bindings don´t : TypeError: qof_session_begin() takes exactly 6 arguments (5 given) > > So for the moment I redraw the conclusion that everything is fine.
Comment on attachment 174319 [details] [review] Patch to add dependencies for gnucash_core.c r19802 (by jralls, with my proposed modifications). Thanks a lot!
(In reply to comment #9) > > When I apply this patch on the situation I described above (R19795 install, > > compile, update to 19805) and make clean, make install It doesn´t work. > > To be a bit more specific : Gnucash works nut the bindings don´t : > TypeError: qof_session_begin() takes exactly 6 arguments (5 given) But this discussion was about the "Build" of python, not their usage (which is a different topic). Without the Makefile changes, your fake argument would have broken the build at src/optional/python, but with them the build completes. So IMHO the issue is fixed. Thanks everyone!
(In reply to comment #7) > Created an attachment (id=174430) [details] > This patch adds a fake argument to qof_session_begin > > This patch adds a fake argument to qof_session_begin. This is a try to reflect > a change in the number of arguments of a function like it happended in r19798. That patch isn't against current svn. I'm not sure what it's against, but it includes a bunch of changes from r19798. At this point I think you should `svn revert -R .; svn up ` and try again. Also, r19802 includes Makefile changes, so you need to run autogen.sh and configure.
I am confused. I will work that out but not now.
Created attachment 174456 [details] [review] A patch against R19808 to add a fake argument to qof_session_begin
(In reply to comment #14) > Created an attachment (id=174456) [details] [review] > The same patch this time against R19808 (I hope) One source of my confusion. I am fighting to understand how changing the c-source works.
OK, now I'm confused. What is the point of the "fake" argument to qof_session_begin?
I tried to create a test case to see if the changes that were made to the python-bindings to reflect changes in the c-source actually work. But for me they donßt and that makes me feel dumb because I don´t know why.
Comment on attachment 174456 [details] [review] A patch against R19808 to add a fake argument to qof_session_begin It is intended to be a testcase. I hope it works.
OIC,it's not something you want us to commit, you want help with getting it to show up in gnucash_core.c, is that it?
Actually I don´t want it to show up anywhere at all except for Bugzilla. I just wanted to have a patch with that I could create a testcase to see if the python-bindings compile without problem. The testcase is to add an argument to a function, qof_session_begin was just taken because a change of that function led to this discussions. Maybe you could put it in the distribution but then it should be changed so that if argument fake is TRUE then gnucash should respond with "Are you serious ?" ;-))
(In reply to comment #20) > Actually I don´t want it to show up anywhere at all except for Bugzilla. I just > wanted to have a patch with that I could create a testcase to see if the > python-bindings compile without problem. The testcase is to add an argument to > a function, qof_session_begin was just taken because a change of that function > led to this discussions. > > Maybe you could put it in the distribution but then it should be changed so > that if argument fake is TRUE then gnucash should respond with "Are you serious > ?" ;-)) Sorry for producing so much output without actual output. ;-) When you apply my fakepatch, is make clean and make enough to make your bindings work ?
(In reply to comment #19) > OIC,it's not something you want us to commit, you want help with getting it to > show up in gnucash_core.c, is that it? Ok, after some minutes I understand what you ask me. Yes, I want to help with that ;-)
So all you need to do to see how SWIG works is to add your extra parameter to the declaration of qof_session_begin() in qofsession.h, then cd over to src/optional/python-bindings and run make. No make clean is required since r19802, because the headers that gnucash_core.c depends upon have been enumerated in the Makefile. What will happen is that the binding of qof_session_begin will acquire the new paramater, and you'll be able to call it from code (as gnucash.gnucash_core.qof_session_begin()). That doesn't do anything about fixing up Session.__init__() in gnucash_core.py: That part is a manual task, and you have to do it yourself. In fact, if you don't, trying to run python code will fail because Session.__init__() is calling qof_session_begin() with only 5 parameters. (NB that qof_session is aliased to a private class GnuCashCoreClass inside of class Session, so the code that you see is "self.begin()". I don't think such trickery is helpful, because it makes it difficult for someone (i.e., me) to change a C function signature and find all of the places where I need to change calls to that function.)
From the discussion I gather that indeed the "build of python-bindings" is now fixed. As for jralls remark, > That doesn't do anything about fixing up Session.__init__() in gnucash_core.py Well, yes. Python is an interpreted language, so by definition we cannot catch such kind of error in the compilation stage. Instead, the python bindings must bring its own set of unittests (potentially being run by "make check") which will then catch this sort of error. But that's the responsibility of the python binding maintainers, so jralls as a non-python developer can feel free to change the gnucash API, even though this might risk a temporary breakage of some bindings, but that's the fate of bindings. For today, the issue is resolved. For the future, just as today no automatic resolution is possible, so we will continue to talk to each other to clear up any issue that might arise in the future. Thanks everybody!
GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=634673. Please update any external references or bookmarks.