David Reiss
added a comment - 14/Sep/09 17:18 The code doesn't appear to have been modified since Ross's last patch inGit: http://thrift-rpc.org/?p=thrift.git;a=shortlog;h=refs/heads/pri/rmcfarland/c_bindings_linear

Michael Lum
added a comment - 15/Aug/10 22:06 here's the summary
the thrift binary protocol is implemented
the buffered, framed, memory buffer, socket, and server socket transports are implemented
the codegen adds a --gen c option that supports all Thrift types as various glib types
there are hooks into autotools to support gcov and valgrind to show coverage and memory/leak checks
about 25 tests
test that calls all services in ThriftTest.thrift from a C client to a C++ server
have a working cassandra 0.6.2 client
compiles on Snow Leopard, CentOS 5 and MinGW in WinXP
what's left:
still working on getting code coverage up to 100% with no memory leaks reported by valgrind.
some additional thrift server code to support C services
both are in progress.

Bryan Duxbury
added a comment - 16/Aug/10 00:01 Is it worth getting this into the codebase either in a branch or trunk as an "experimental" language? There's a good chance it will be useful to some folks before its "stable".

I'd be okay moving it into trunk, but two things I think we should definitely do first are (1) delete random files like "gen-c" at top level and (2) rename it to c_glib or something like that to indicate that the implementation is very glib-dependent.

David Reiss
added a comment - 16/Aug/10 00:05 This is already in a branch at http://svn.apache.org/repos/asf/incubator/thrift/branches/c-bindings/
I'd be okay moving it into trunk, but two things I think we should definitely do first are (1) delete random files like "gen-c" at top level and (2) rename it to c_glib or something like that to indicate that the implementation is very glib-dependent.

Michael Lum
added a comment - 16/Aug/10 00:48 the implementation doesn't have any gen-c at top level, its a pretty big refactoring of what was there.
i'll submit a patch soon after making some changes:
applies cleanly to trunk
rename to --gen c_glib
makes c_glib optional

David Reiss
added a comment - 24/Aug/10 22:25 It looks like the generator is not in the patch. You can just submit the output of git diff if you want. Would you mind renaming the generator and lib directory to c_glib?

what specifically do you think should be "c_glib"? I attempted to change it in a few places, and some of the built-in macros and build artifacts didn't like having underscores in the generator name. IIRC, THRIFT_REGISTER_GENERATOR was one of them. I didn't dig deeper into the macros themselves. Unless it can be called cglib (which seems even more confusing to me).

Michael Lum
added a comment - 24/Aug/10 22:49 what specifically do you think should be "c_glib"? I attempted to change it in a few places, and some of the built-in macros and build artifacts didn't like having underscores in the generator name. IIRC, THRIFT_REGISTER_GENERATOR was one of them. I didn't dig deeper into the macros themselves. Unless it can be called cglib (which seems even more confusing to me).

> what specifically do you think should be "c_glib"?
The generator file and generator class, the library directory, and the first argument to THRIFT_REGISTER_GENERATOR. Basically, anything that would prevent us from having an additional C implementation using a different utility library (like apr).

I don't see anything in THRIFT_REGISTER_GENERATOR that would cause a problem, because "_" is a valid identifier character. Do you remember the error you got?

David Reiss
added a comment - 24/Aug/10 22:59 > what specifically do you think should be "c_glib"?
The generator file and generator class, the library directory, and the first argument to THRIFT_REGISTER_GENERATOR. Basically, anything that would prevent us from having an additional C implementation using a different utility library (like apr).
I don't see anything in THRIFT_REGISTER_GENERATOR that would cause a problem, because "_" is a valid identifier character. Do you remember the error you got?

Michael Lum
added a comment - 24/Aug/10 23:46 Let me take a stab at changing the names to reproduce the errors I got. I can't remember exactly what it was. Either that or we take a vote for namespace squatter's rights

Any feedback on the updated patch? I'm guessing it won't cleanly apply to trunk anymore since it's been two weeks, but if the general idea looks correct, I can update it to apply cleanly to the current trunk.

Michael Lum
added a comment - 10/Sep/10 18:34 Any feedback on the updated patch? I'm guessing it won't cleanly apply to trunk anymore since it's been two weeks, but if the general idea looks correct, I can update it to apply cleanly to the current trunk.

Based on my limited knowledge of glib, this looks good overall. I have a few comments:

configure.ac:
I'm pretty sure autoscan is incorrect to request PROG_AWK and PROG_RANLIB.
Can you remove those?

Can you update the c_glib naming on AX_THRIFT_LIB and AX_THRIFT_GEN?
Also, can you add the c_glib binding to the summary printed out at the bottom?
I changed the style of the library checks a bit to make this work.

What is the source of the code used to enable coverage?

test/c:
Can you rename this to test/c_glib?

Makefile.in:
This should not be in the patch.

thrift_binary_protocol.c:
I think this line might cause problems with strict aliasing. See bitwise_cast in the C++ codebase for more information.
guint64 bits = GUINT64_FROM_BE (*(unsigned long long *)&value);

Makefile.am:
no need for ACLOCAL_AMFLAGS anymore
Replace thriftc with thrift_c_glib

David Reiss
added a comment - 13/Sep/10 22:07 Based on my limited knowledge of glib, this looks good overall. I have a few comments:
configure.ac:
I'm pretty sure autoscan is incorrect to request PROG_AWK and PROG_RANLIB.
Can you remove those?
Can you update the c_glib naming on AX_THRIFT_LIB and AX_THRIFT_GEN?
Also, can you add the c_glib binding to the summary printed out at the bottom?
I changed the style of the library checks a bit to make this work.
What is the source of the code used to enable coverage?
test/c:
Can you rename this to test/c_glib?
Makefile.in:
This should not be in the patch.
thrift_binary_protocol.c:
I think this line might cause problems with strict aliasing. See bitwise_cast in the C++ codebase for more information.
guint64 bits = GUINT64_FROM_BE (*(unsigned long long *)&value);
Makefile.am:
no need for ACLOCAL_AMFLAGS anymore
Replace thriftc with thrift_c_glib
thriftc.pc:
Replace thriftc with thrift_c_glib

Anatol Pomozov
added a comment - 27/Sep/10 07:37 I think about adding Vala language support to Thrift https://issues.apache.org/jira/browse/THRIFT-764 and I need c-binding (with glib) for it. What is ETA for seeing it in the trunk?
Once it will be added to trunk I start working on Vala support and BTW it will be a good test case for c-bindings (+glib).
Anyways thanks Michael and everyone else for doing this great job.

Michael Lum
added a comment - 27/Sep/10 08:24 The changes aren't too much work to implement. I just haven't been able to get to them due to some other projects going on. I should at least be able to get it done in the next couple of weeks.

Michael Lum
added a comment - 27/Sep/10 23:01 This patch applies cleanly to r1001921.
changes:
configure.ac removes PROG_AWK and PROG_RANLIB, although now bootstrap complains about it being needed by Erlang
renamed AX_THRIFT_LIB and AX_THRIFT_GEN naming to c_glib
the source of the code coverage are hooks into gcov, as seen in configure.ac and executed by the test-wrapper script in test/c_glib
Makefile.in removed
changed thrift_binary_protocol.c to use type punning to solve the strict aliasing problem
removed ACLOCAL_AMFLAGS
renamed thriftc to thrift_c_glib

However, it is out of date. I've been locally merging it with the trunk to produce patches that apply cleanly to trunk. I was hoping that this would get included into the thrift trunk soon so that I could rebase on it and keep working on features.

Since 10 days have passed since I submitted the last patch, I'm guessing it won't cleanly apply to the trunk anymore and a new patch will have to be created that works.

Michael Lum
added a comment - 07/Oct/10 19:20 I was working on it here:
http://github.com/mlum/thrift
However, it is out of date. I've been locally merging it with the trunk to produce patches that apply cleanly to trunk. I was hoping that this would get included into the thrift trunk soon so that I could rebase on it and keep working on features.
Since 10 days have passed since I submitted the last patch, I'm guessing it won't cleanly apply to the trunk anymore and a new patch will have to be created that works.

Anatol Pomozov
added a comment - 08/Oct/10 00:54 Hi, Michael.
If it is not very hard for you, could you please push your recent changes (+merge from trunk) to http://github.com/mlum/thrift
Thanks for what you are doing.

Bryan Duxbury
added a comment - 14/Oct/10 00:41 What's the consensus on this? I'd love to commit it so you guys can stop rebasing it all the time, but I lack the personal experience to give it a review.

David Reiss
added a comment - 14/Oct/10 18:33 All of my concerns have been addressed. I'm okay with the patch at this point. I think last patch from Anatol Pomozov might have broken test-wrapper.sh by moving it out of AC_CONFIG_FILES.

Anatol Pomozov
added a comment - 14/Oct/10 19:37 Hi, David.
You are right. I added it back to the list. See fix here http://github.com/anatol/thrift/commit/4c5aa31f04a0ab6addbb9a8dea6d8bfe3c171288
Also I think we need to add *.h to the list of INCLUDE files http://github.com/anatol/thrift/commit/bccb67878b305f03fc175e13eb29201f249d72ce

Anatol Pomozov
added a comment - 14/Oct/10 22:06 Test files are moved to lib/c_glib/test. Check this out http://github.com/anatol/thrift/commit/aa205295358bc141010cb9773b5535c0b2f3fdd4
./configure --with-c-glib --without-ruby --without-python --without-erlang
make
make check
works fine for me.
Regarding CPP file - this is a test that checks possibility of using C client with C++ server. The best thing as for me is to add "if WITH_CPP" to Makefile for this particular test.
I also found another issue: the output dir for the generator is "gen-c", it should be "gen-c_glib" instead. Does it sound right?
PS I'll be unavailable next 5 days, so I cant respond your requests. If Michael wants to continue it i'll be glad. Otherwise i'll fix it next weekend.

Michael Lum
added a comment - 25/Oct/10 23:51 Yeah, I wasn't able to reproduce it on an Ubuntu VM. When I get some time, I'm thinking of building a Debian Lenny VM, but the next two weeks are going to be a little busy for me.

Anatol Pomozov
added a comment - 26/Oct/10 21:29 I believe that the SEGFAULT happens because of old packages (libglib2). On Ubuntu I have 2.24 on mac - 2.26.
BTW I am going to add Vala support to Thrift later and minimum version for vala is glib 2.24

Anatol Pomozov
added a comment - 26/Oct/10 23:51 I installed Debian Lenny to a virtual machine and tried to compile Thrift with c_glib support.
As I mentioned above Thrift requires Autoconf 2.61. I updated system from lenny-backports and "make && make check" works fine for me without any SEGFAULTS.
What I did
apt-get install git-core libglib2.0-0-dev autoconf libtool libboost-dev bison flex make g++ pkg-config
./bootstrap.sh
./configure --with-c_glib --without-cpp --without-python
make
make check
I did the same for Debian Testing with the same result - tests are passed.
So it makes me think that the problem is in incompatibility with older glib version.
Roger, can you please install libglib2.0-0 from lenny-backports and let us know if you still have the issue.

Roger Meier
added a comment - 27/Oct/10 07:05 Great job! All 22 tests passed
I did sudo apt-get -t lenny-backports install libglib2.0-dev on my Debian Lenny and it rocks!
Could someone create the final patch against trunk and I will commit this.

Anatol Pomozov
added a comment - 27/Oct/10 20:10 Not sure why, but git official git mirror git://git.apache.org/thrift.git still contains c-bindings c-compiler branches. Sounds like a bug in svn->git conversion tool.
Anyway it is not a blocker. I am going to close the issue.

Aurélien Revol
added a comment - 16/Sep/11 11:51 Well I suppose this answers my questions: THRIFT-1016: using GSocket in c_glib library . (Another element of answer lies in the existence of THRIFT-1145 , whose description is misleading however.)
Has that ever been tried? What are the possible obstacles to using GSocket?

I tested c_glib client with a csharp server just for fun.
c_glib client calling a function on csharp-server and returns a big string.
On any time i get a segfault or block by reading the returning string from server.