Shastick commented 3 years ago

Hello!

I have kind of a silly question that fits in a wider context, so I'll start with that. Apologies if such an issue is not the right place to ask.

As part of building some go services with bazel and the help of rules_go, I needed to support connecting to Oracle via this library.

To get this to work with Bazel, I had to do a very minor tweak to a go file (along some other things for which this library is not responsible for[^1]). In drv.go I replaced, on line 68, #include "dpi.c" with #include "dpiImpl.h".

Now, I'd be curious to know if:

this is even a good idea? My knowledge of C and CGO is too limited to assert if this may blow up in a bad way: at the moment this lets me run my simple use case, but I'm wondering if I'm setting myself up for a surprise in the futur. On the other hand, it is the only file that includes dpi.c from a .go file, so it might be Ok?
you'd be interested in updating the library in a way that makes it play nicely with bazel and rules_go? I'm assuming I'm not the only one who wants to combine Go, Oracle and Bazel. I'd be happy to help for this, and am asking to understand what would or would not be acceptable to you (in terms of moving sources, or adding a bazel build configuration, or a combination thereof)

Basically, I'm happy with my current setup, and would be interested in making godror easily useable with Bazel – assuming the drv.go update is not a problem.

If a PR targeted at Bazel is too specific for this lib, I'm also happy to document the workaround.

Kindly, Julien

[^1]: for the curious who might seek to use godror from bazel, this is due to the fact that gazelle doesn't reimplement the totality of the magic that comes with CGO. I'm no expert in this subject and make this assumption from this issue.

tgulacsi commented 3 years ago

For me, changing that #include erors with lots of "undefined reference" errors. So please show the diff that plays nicely both with bazel and "go install" !

For the second question: I'm open for a non-intrusive change that does not harm the normal (non-Bazel) use case.

Shastick commented 3 years ago

Thanks for the quick feedback.

For me, changing that #include errors with lots of "undefined reference" errors.

Ok, the plot thickens. (I did not try to go install again after my tweaks – sorry :()

Without the replacement, on OS X, I run in a ton of duplicate symbols errors, eg:

duplicate symbol '_dpiVar_getReturnedData' in:
    /var/folders/8y/pztgykk94_q4j7fnmjhnbnw0001t4q/T/rules_go_work-470405566/_x3.o
    /var/folders/8y/pztgykk94_q4j7fnmjhnbnw0001t4q/T/rules_go_work-470405566/_x43.o
ld: 1164 duplicate symbols for architecture x86_64

Looks like I'd need to (re)learn a few things about C and CGO in order to make a clean PR, not sure if/when I'll find the time. Meanwhile, here is a gist as well as a Stackoverflow post to discuss non-godror specific things.

kurt-google commented 3 years ago

We are using godror with bazel as well and work around this issue.

When using bazel we build odpi as a cc_library separately and linked it to the go_library for godror as a cdeps, and replacing dpi.c with the header. In this situation the odpi library is compiled separately. In the patch posted these C files are passed as additional srcs which will be compiled separately as C object files and then linked into the final binary. go install takes the #cgo front matter and just compiles it directly as an object that is linked together with the go object file.

I think the patch Shastick is using may work with dpi.c if the .c/.h files are instead added to the data dependency (instead of being compiled separately and linked when placed in srcs). Since it looks like you are getting both the #cgo front matter compiled and the globbed *.c srcs compiled causing duplicate symbols. I'm not aware of a good solution to make the bazel patching simple except changing godror to rely on a precompiled odpi, which seems worse for go install users.

tgulacsi commented 3 years ago

@kurt-google can you share your patch? Maybe we can include it behind a build tag.

kurt-google commented 3 years ago

https://github.com/GoogleCloudPlatform/elcarro-oracle-operator/blob/main/hack/0001-Patch-to-add-bazel-support.patch Is the patch to godror adding bazel targets (on to 0.25.3). However without something like automation to check that golang imports are updated and newly added files are added such a build file will quickly fall out of sync as godror changes (or odpi changes).

tgulacsi commented 3 years ago

I see. I've thought that for helping Bazel users, we could put the needed changes behind a build tag, but that's a tiny part of the whole needed machinery, so doesn't worth the effort, I think.

paramadeep commented 2 years ago

@kurt-google I forked godror and added bazel to it using gazzel. Also include cc_library dependency that was mentioned in your patch. When i build the godror independently it works fine. But when I reference the forked godror in my actual project go.mod replace github.com/godror/godror => github.com/paramadeep/godror v0.30.1 now I get the same dependency problem. `external/com_github_godror_godror/conn.go:10:10: fatal error: 'dpiImpl.h' file not found

include "dpiImpl.h"

     ^~~~~~~~~~~

1 error generated. Error in child process '/usr/bin/xcrun'. 1 compilepkg: error running subcommand external/go_sdk/pkg/tool/darwin_amd64/cgo: exit status 2` Can you please help how you managed to use the patched version of godror in you project.

For time-being I have added all godror code directly in my code base to get things running.

godror / godror

Very simple question around an import in drv.go and support for Bazel. #157

include "dpiImpl.h"