After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 634673 - Change of files in the c-source of GnuCash may lead to broken build of python-bindings
Change of files in the c-source of GnuCash may lead to broken build of python...
Status: RESOLVED FIXED
Product: GnuCash
Classification: Other
Component: Python Bindings
git-master
Other Linux
: Normal normal
: ---
Assigned To: Mark Jenkins
Mark Jenkins
Depends on:
Blocks:
 
 
Reported: 2010-11-12 10:37 UTC by Christoph Holtermann
Modified: 2018-06-29 22:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to add dependencies for gnucash_core.c (2.31 KB, patch)
2010-11-12 12:02 UTC, Mike Evans
committed Details | Review
This patch adds a fake argument to qof_session_begin (12.12 KB, patch)
2010-11-14 12:52 UTC, Christoph Holtermann
none Details | Review
A patch against R19808 to add a fake argument to qof_session_begin (12.12 KB, patch)
2010-11-14 19:26 UTC, Christoph Holtermann
none Details | Review

Description Christoph Holtermann 2010-11-12 10:37:29 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.
Comment 1 Christoph Holtermann 2010-11-12 11:52:13 UTC
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.
Comment 2 Mike Evans 2010-11-12 12:02:52 UTC
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 3 Christian Stimming 2010-11-12 15:33:32 UTC
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.
Comment 4 John Ralls 2010-11-12 20:07:11 UTC
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.
Comment 5 Mike Evans 2010-11-14 10:05:58 UTC
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.
Comment 6 Christoph Holtermann 2010-11-14 11:19:16 UTC
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.
Comment 7 Christoph Holtermann 2010-11-14 12:52:05 UTC
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.
Comment 8 Christoph Holtermann 2010-11-14 12:54:35 UTC
(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.
Comment 9 Christoph Holtermann 2010-11-14 12:56:18 UTC
(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 10 Christian Stimming 2010-11-14 13:21:23 UTC
Comment on attachment 174319 [details] [review]
Patch to add dependencies for gnucash_core.c

r19802 (by jralls, with my proposed modifications). Thanks a lot!
Comment 11 Christian Stimming 2010-11-14 13:24:03 UTC
(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!
Comment 12 John Ralls 2010-11-14 17:01:18 UTC
(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.
Comment 13 Christoph Holtermann 2010-11-14 19:01:26 UTC
I am confused. I will work that out but not now.
Comment 14 Christoph Holtermann 2010-11-14 19:26:31 UTC
Created attachment 174456 [details] [review]
A patch against R19808 to add a fake argument to qof_session_begin
Comment 15 Christoph Holtermann 2010-11-14 19:29:01 UTC
(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.
Comment 16 John Ralls 2010-11-14 19:35:00 UTC
OK, now I'm confused. What is the point of the "fake" argument to qof_session_begin?
Comment 17 Christoph Holtermann 2010-11-14 19:45:31 UTC
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 18 Christoph Holtermann 2010-11-14 19:47:56 UTC
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.
Comment 19 John Ralls 2010-11-14 20:00:04 UTC
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?
Comment 20 Christoph Holtermann 2010-11-14 20:06:36 UTC
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 ?" ;-))
Comment 21 Christoph Holtermann 2010-11-14 20:09:15 UTC
(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 ?
Comment 22 Christoph Holtermann 2010-11-14 20:19:07 UTC
(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 ;-)
Comment 23 John Ralls 2010-11-15 01:16:01 UTC
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.)
Comment 24 Christian Stimming 2010-11-15 09:38:26 UTC
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!
Comment 25 John Ralls 2018-06-29 22:47:07 UTC
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.