Go bindings #325

rafaelsierra · 2022-07-05T09:00:44Z

This PR builds on top of #292

rafaelsierra · 2022-07-05T19:51:47Z

xapian-bindings/go/xapian.go

+func NewDatabase(args ...interface{}) (db raw.Database, err error) {
+	defer func() {
+		if r := recover(); r != nil {
+			if errMsg, ok := r.(string); ok {
+				for msgPrefix, errType := range errorMap {
+					if strings.HasPrefix(errMsg, msgPrefix) {
+						err = errType
+						return
+					}
+				}
+			}
+			panic(r)
+		}
+	}()
+
+	db = raw.NewDatabase(args...)
+	return
+}


@ojwb this is the basic idea behind how to handle errors/exceptions on the go binding

The implementation here is very poor because the error message contains extra information that is not relevant when asserting which kind of error it is (we could use a direct hash access if the error message was just the class/error name)

I believe this could be fixed if we can change how swig generates the error message to include the error code or any other direct identifier we could intercept on Go.

This is the entire error handling function:

static void _swig_gopanic(const char *p) { struct { const char *p; } SWIGSTRUCTPACKED a; a.p = p; crosscall2(_cgo_panic, &a, (int) sizeof a); } #define SWIG_exception(code, msg) _swig_gopanic(msg)

The declaration of crosscall2 is:

void crosscall2(void (*fn)(void *), void *, int);

So if we can change it to include the code from SWIG_exception I believe the error handling would be simpler

Also, if there is a way to change any of that, we could also stop calling _cgo_panic and instead call a Go-provided function that wouldn't cause any panic (recoverable or otherwise)

BTW, I notice SWIG itself has moved away from using crosscall2 for this in swig/swig@6ca5d5d:

swig -go: don't use crosscall2 for panicking

Instead rely only on documented and exported interfaces.

SWIG/Go doesn't seem to make use of SWIG_RuntimeError, etc but if you want to turn the C++ exception into a numeric code you can probably rethrow and catch it in C++ to dispatch by exception type. Look at SetPythonException() in xapian-bindings/python/except.i (generated) for example.

rafaelsierra · 2022-07-05T19:55:24Z

xapian-bindings/go/xapian.go

@@ -0,0 +1,53 @@
+package xapian


My proposal to have an actual error handling (instead of having to let users rely on recover by themselves) is to provide a thin wrapper to the raw SDK and making sure that whenever we get a panic from swig we transform that into an error

With these changes, we no longer get a SIGABRT from Go whenever a panic is called from Swig, this way we can recover() from the panic and keep the process going.

I don't know if this addresses the issue with destructors not being called after the panic

It doesn't seem ideal that this seems to require writing a wrapper for every wrapped function. We use SWIG because it automates away all that drudgery. We've had hand-coded Perl XS and Java JNI bindings in the past and they always ended up lagging behind the C++ API because of the extra work needed, whereas for SWIG-generated bindings in most cases a new method gets wrapped automatically, while new classes just need setting up according to what sort of classes they are (e.g. user-subclassable classes get special handling).

I'll take a look and see what we can do.

I think ideally SWIG itself would support a more Go-like way to wrap exceptions, but there doesn't seem to be much progress on that front.

ojwb · 2022-07-06T02:27:41Z

.appveyor.yml

        } else {
          $env:PATH="C:\mingw-w64\x86_64-7.2.0-posix-seh-rt_v5-rev1\mingw64\bin;C:\msys64\usr\bin;$env:PATH"
-          $env:CONFIGOPTS+=" --host=x86_64-w64-mingw32"
+          $env:CONFIGOPTS+=" --host=x86_64-w64-mingw32 --with-go"
+          $env:BINDINGS+=" xapian-bindings"
        }


The changes to this file don't seem to actually do anything useful - the BINDINGS environment variable is set but never used. The added --with-go option has no effect unless xapian-bindings actually gets configured (and if it does get configured, it should automatically enable any languages the tools/libraries/etc are found for - all --with-go would accomplish is to (a) give an error if the things needed to enable Go bindings aren't found and (b) disable that auto-detection for other languages. Maybe (a) is arguably useful (it would catch the case where the CI image is updated and something needed for a particular language is no longer found - currently we'd quietly stop testing it).

In the GHA CI we just install the various dependencies and rely on the auto-detection, so I think it'd be better to make a deliberate choice for whether we want to pass --with-X for all the languages we expect to be building and testing bindings for, and if so todo that consistently across the CI systems we're using.

In #292 (review) I noted that the use of $BINDINGS was removed, apparently while resolving a merge conflict.

Dockerfile

ojwb · 2022-07-06T02:35:23Z

xapian-bindings/configure.ac

+    if test -z "${GOROOT}" ; then
+      AC_MSG_ERROR([goroot not set correctly])
+    fi
+  fi


We're missing AC_MSG_RESULT for the failure paths above, which will give mangled output. I raised this on the original PR but it was never addressed.

It also shouldn't error out unless $with_go is set to yes - if it's empty we enable the language is everything we need is detected, and disable it otherwise - that way a plain ./configure enables all the bindings it can.

ojwb · 2022-07-06T02:48:33Z

xapian-bindings/configure.ac

+  if test -n "${USE_CLANG_LIB}" ; then
+    AC_MSG_RESULT([${USE_CLANG_LIB}])
+    AC_SUBST(USE_CLANG_GO_LIB,'-lc++')
+    AC_MSG_RESULT([${USE_CLANG_GO_LIB}])


There's no AC_MSG_CHECKING corresponding to some of these AC_MSG_RESULT calls which will result in garbled output.

ojwb · 2022-07-06T02:48:36Z

xapian-bindings/configure.ac

+  if test -z "$GO" ; then
+    AC_MSG_ERROR("go not installed or go path has not been set")
+  fi
+fi


This seems to duplicate much of what's added above.

You shouldn't be running $XAPIAN_CONFIG directly - the XO_LIB_XAPIAN macro has already done that. Just use the already substituted XAPIAN_CXXFLAGS instead of GO_CXX, and similarly for other values.

Don't try to handle the "xapian-core not found" case - that's not the job of each language binding to worry about, and anyway XO_LIB_XAPIAN will have aborted with an error if it didn't find it.

ojwb · 2022-07-06T02:54:47Z

xapian-bindings/go/docs/index.rst

+./bootstrap && ./configure && ./make && make install
+
+if you have already installed xapian-core :
+        just do make install:


This is very specific to building from git, and not what should be in the Go-specific documentation. Look at what other bindings have here - e.g. https://github.com/xapian/xapian/blob/master/xapian-bindings/php7/docs/index.rst

ojwb · 2022-07-06T03:00:31Z

xapian-bindings/go/xapian.go

@@ -0,0 +1,53 @@
+package xapian


It doesn't seem ideal that this seems to require writing a wrapper for every wrapped function. We use SWIG because it automates away all that drudgery. We've had hand-coded Perl XS and Java JNI bindings in the past and they always ended up lagging behind the C++ API because of the extra work needed, whereas for SWIG-generated bindings in most cases a new method gets wrapped automatically, while new classes just need setting up according to what sort of classes they are (e.g. user-subclassable classes get special handling).

I'll take a look and see what we can do.

I think ideally SWIG itself would support a more Go-like way to wrap exceptions, but there doesn't seem to be much progress on that front.

ojwb · 2022-07-06T03:08:32Z

xapian-bindings/go/xapian.go

+func NewDatabase(args ...interface{}) (db raw.Database, err error) {
+	defer func() {
+		if r := recover(); r != nil {
+			if errMsg, ok := r.(string); ok {
+				for msgPrefix, errType := range errorMap {
+					if strings.HasPrefix(errMsg, msgPrefix) {
+						err = errType
+						return
+					}
+				}
+			}
+			panic(r)
+		}
+	}()
+
+	db = raw.NewDatabase(args...)
+	return
+}


BTW, I notice SWIG itself has moved away from using crosscall2 for this in swig/swig@6ca5d5d:

swig -go: don't use crosscall2 for panicking

Instead rely only on documented and exported interfaces.

SWIG/Go doesn't seem to make use of SWIG_RuntimeError, etc but if you want to turn the C++ exception into a numeric code you can probably rethrow and catch it in C++ to dispatch by exception type. Look at SetPythonException() in xapian-bindings/python/except.i (generated) for example.

ojwb · 2022-07-06T03:08:57Z

xapian-bindings/xapian-headers.i

@@ -245,7 +245,7 @@ STANDARD_IGNORES(Xapian, Query)

 %warnfilter(SWIGWARN_TYPE_UNDEFINED_CLASS) Xapian::Query::Internal;
 #if defined SWIGCSHARP || defined SWIGJAVA || defined SWIGPERL || \
-    defined SWIGPYTHON || defined SWIGRUBY
+    defined SWIGPYTHON || defined SWIGRUBY || defined SWIGGO
 // C#, Java, Perl, Python and Ruby wrap these "by hand" to give a nicer API


Comment no longer matches the code.

ojwb · 2022-07-06T03:12:33Z

xapian-core/INSTALL

+
+	make
+
+	make install


This would be useful to document (if the commands given actually work - I'm dubious how well tested it is, given the --nocofirm in the second pacman command seems like a typo) but isn't related to Go support so would be better as a separate patch.

ojwb · 2022-07-06T04:13:11Z

xapian-bindings/configure.ac

+  AC_MSG_RESULT([${USE_CLANG_LIB}])
+  if test -n "${USE_CLANG_LIB}" ; then
+    AC_MSG_RESULT([${USE_CLANG_LIB}])
+    AC_SUBST(USE_CLANG_GO_LIB,'-lc++')


Copying over a comment from the old PR which was never responded to:

This also looks very dubious to me, and could misdetect if the stdlib is present elsewhere - e.g. GXX=/home/stdlib/bin/gcc, or misdetect the other way if CXXFLAGS=-stdlib=libc++ is used instead of CXX='clang -stdlib=libc++'.

What is this actually trying to achieve?

I realise you may well not know, but if so it seems better to drop this and see what (if anything) breaks so we actually understand the need for it, and can perhaps address it better.

This commit also makes sure that the `xapian.go` generated on the source tree contains the necessary CGO flags to compile

rafaelsierra · 2022-07-15T21:45:25Z

xapian-bindings/go/go.i

+func NewDatabase(a ...interface{}) (db Database, err error) {
+	defer func() {
+		if r := recover(); r != nil {
+			if errMsg, ok := r.(string); ok {
+				err = errors.New(errMsg)
+				return
+			}
+			panic(r)
+		}
+	}()
+
+	db = NewWrappedDatabase(a...)
+	return
+}


@ojwb following your last comment I got rid of the static xapian.go file and am now relying only on the go.i file

To still be able to return either a database instance or an error (the idiomatic go way of doing errors) there is no way around recovering from panics, which in this case is a quite verbose way of having a catch

Also, in recent versions, it looks like Go now exposes the _cgo_panic function expecting the parameter to be a struct with a cstr inside, so there is no way to return a more complex struct (with code and message) at the moment

We could receive the message and split by : and use the first index as code and second as message, but I am not sure at the moment if this would be the best approach

I forgot to mention that I am, of course, using Xapian::Database::Database just as example, and that other functions that can also throw errors would follow the same pattern

ttys3 · 2023-06-02T13:23:24Z

@rafaelsierra @ojwb what's the status of this PR ?

I'd like to take over and start another one (based on this PR, and rebased to new master), if, may I allowed to do this ?

rafaelsierra · 2023-06-02T13:43:19Z

Hey @ttys3 , please feel free to start a new PR.

Unfortunately I didn't had much time to go back to this =/

srinivasyadav18 and others added 30 commits July 4, 2022 21:42

go dir

648a4a6

config file for go

cdcff94

examples added

1f472fd

test file

99d397b

"added go"

d195b62

added go

1af46ff

added go

829ce39

test added

b8e8b77

example for indexing

d45ea54

Update Makefile.am

8bbeebb

Update Makefile.am

7256abd

Update Makefile.am

0dd9579

Update Makefile.am

93be6b6

docs and gitignore

06508c5

ex and mk

5563a3f

changed git ignore

6c86540

go bindings

80bf72c

Dockerfile for xapian

10b6b4b

Update Makefile.am

7c2c411

Update index.rst

4ae516d

Update Makefile.am

04f51fa

for tests

69f930c

Update Makefile.am

1a15121

tabs

37a3696

tabs and spaces

d2f2944

Update Makefile.am

b97d114

Update Makefile.am

042ead5

update for os x

b68f801

Update Makefile.am

5697f7f

white spaces

c0a419f

Srinivas Yadav and others added 15 commits July 4, 2022 22:12

appveyor config for go-bindings

6ffb3aa

mingw x86 build

852be72

mingw x86 build

ee33358

mingw x86 build

d2ada73

mingw x86 build

af01225

checking x86

1d8f429

fix appveyor build

20f4efc

go bindings mingw64 x64

4d5977e

clean up

749ddf9

fix up spaces

5793f6a

fix up spaces

790cb20

fix up spaces

d969917

fix up spaces

2c1eb04

Updated build so that make and make install works

6369bc0

Formatted code

9d9687d

rafaelsierra marked this pull request as draft July 5, 2022 09:01

rafaelsierra added 3 commits July 5, 2022 16:00

Beginning work to handle errors

9fae319

Dirty implementation for handling errors

5e271ad

added back allow_failures

b711e8b

rafaelsierra commented Jul 5, 2022

View reviewed changes

ojwb reviewed Jul 6, 2022

View reviewed changes

This was referenced Jul 6, 2022

go bindings #292

Closed

Appveyor config for Xapian Go bindings #295

Closed

rafaelsierra added 3 commits July 15, 2022 17:54

Testing with go errors and exceptions

764c31e

This commit also makes sure that the `xapian.go` generated on the source tree contains the necessary CGO flags to compile

Updated test to check for error type

b35c4dc

Removed Dockerfile

4b0ac91

rafaelsierra commented Jul 15, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go bindings #325

Go bindings #325

rafaelsierra commented Jul 5, 2022

rafaelsierra Jul 5, 2022

ojwb Jul 6, 2022

rafaelsierra Jul 5, 2022

ojwb Jul 6, 2022

ojwb Jul 6, 2022

ojwb Jul 6, 2022

ojwb Jul 6, 2022

ojwb Jul 6, 2022

ojwb Jul 6, 2022

ojwb Jul 6, 2022

ojwb Jul 6, 2022

ojwb Jul 6, 2022

ojwb Jul 6, 2022

ojwb Jul 6, 2022

ojwb Jul 6, 2022

rafaelsierra Jul 15, 2022

rafaelsierra Jul 15, 2022

ttys3 commented Jun 2, 2023 •

edited

rafaelsierra commented Jun 2, 2023

Go bindings #325

Are you sure you want to change the base?

Go bindings #325

Conversation

rafaelsierra commented Jul 5, 2022

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

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

ttys3 commented Jun 2, 2023 • edited

rafaelsierra commented Jun 2, 2023

ttys3 commented Jun 2, 2023 •

edited