internal/bindings: Don't use raft directly #247
Conversation
Signed-off-by: Cole Miller <cole.miller@canonical.com>
| @@ -1,6 +1,6 @@ | |||
| package bindings | |||
|
|
|||
| /* | |||
| #cgo linux LDFLAGS: -lraft -ldqlite -Wl,-z,now | |||
| #cgo linux LDFLAGS: -ldqlite -Wl,-z,now | |||
There was a problem hiding this comment.
Unrelated comment: why are we using the -Wl,-z,now flags?
They seem to have been introduced here #143, but I'm not sure to understand what compatibility situation they address.
Maybe @MathieuBordere can explain this better?
There was a problem hiding this comment.
It will lead to failing to load a dqlite library that contains unknown symbols.
For example, we extend dqlite's API with some new functionality, but otherwise don't break API/ABI compatibility.
Some user uses a new version of go-dqlite that uses the new dqlite functionality, but this version of go-dqlite is used on a system that runs a version of dqlite that doesn't support this API. Without -Wl, -z, now, this will lead to a runtime failure when a codepath hits an unknown symbol, while with the flags, go-dqlite will fail to load from the start.
There was a problem hiding this comment.
Was this situation hit in real world?
I'd expect the common case is that at runtime go-dqlite will find the same dqlite version it found at build time, or alternatively a higher version. Not a lower version though.
I'm asking mainly since it's a bit unfortunate to require users to set the extra CGO_LDFLAGS_ALLOW environment variable, and to not have go get work without that.
If we could avoid that without any meaningful real-world harm, it'd be good.
There was a problem hiding this comment.
Yeah, it's something I hit when testing at the time, and figured it could happen in real life too.
We could call dqlite_version_number when go-dqlite initializes to check for the minimal dqlite version instead of adding the linker flags, it would also serve a documentation purpose, because currently the minimal dqlite version is nowhere documented except in the (some) release notes.
There was a problem hiding this comment.
I'll open a PR to get rid of -Wl, -z, now.
There was a problem hiding this comment.
Yeah, it's something I hit when testing at the time, and figured it could happen in real life too.
We could call
dqlite_version_numberwhengo-dqliteinitializes to check for the minimal dqlite version instead of adding the linker flags, it would also serve a documentation purpose, because currently the minimal dqlite version is nowhere documented except in the (some) release notes.
That sounds like a good idea, thanks.
Signed-off-by: Cole Miller cole.miller@canonical.com