canonical / dqlite

Embeddable, replicated and fault-tolerant SQL engine.
https://dqlite.io
Other
3.9k stars 219 forks source link

dqlite.h: Introduce DQLITE_API #511

Closed cole-miller closed 1 year ago

cole-miller commented 1 year ago

By analogy with RAFT_API. As a complement, add -fvisibility=hidden to the CFLAGS for libdqlite.a.

Signed-off-by: Cole Miller cole.miller@canonical.com

cole-miller commented 1 year ago

Ah, the tests need to be able to call things from client/protocol.h, let me fix that.

freeekanayaka commented 1 year ago

Ah, the tests need to be able to call things from client/protocol.h, let me fix that.

I didn't look at the failure nor the test code, but the idea would that any test that exercises internal parts of dqlite would go under test/unit (which statically links the code and has access to all symbols, i.e. white box approach), and tests that exercise the user API would under test/integration (which dynamically links libdqlite.so and runs the exactly as a user would).

freeekanayaka commented 1 year ago

Ah, the tests need to be able to call things from client/protocol.h, let me fix that.

I didn't look at the failure nor the test code, but the idea would that any test that exercises internal parts of dqlite would go under test/unit (which statically links the code and has access to all symbols, i.e. white box approach), and tests that exercise the user API would under test/integration (which dynamically links libdqlite.so and runs the exactly as a user would).

We might need to temporarily put in place some hack to allow integration tests to call functions defined in client/protocol.h symbols, but with the goal of cleaning that up at some point.

cole-miller commented 1 year ago

@freeekanayaka I've applied DQLITE_VISIBLE_TO_TESTS to functions and globals that are not public API but currently used by integration tests. It's just defined as DQLITE_API right now, because I couldn't think of a convenient & non-invasive way to express "only visible when building libdqlite.a for use in the integration tests". If you think of a way to do this, let me know! Meanwhile, this PR is still an improvement on the status quo where all dqlite symbols have default visibility.

codecov[bot] commented 1 year ago

Codecov Report

Merging #511 (c1126f7) into master (eb2bb54) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #511   +/-   ##
=======================================
  Coverage   61.00%   61.00%           
=======================================
  Files          34       34           
  Lines        6350     6350           
  Branches     1888     1888           
=======================================
  Hits         3874     3874           
  Misses       1293     1293           
  Partials     1183     1183           
Impacted Files Coverage Δ
src/vfs.c 77.22% <ø> (ø)
freeekanayaka commented 1 year ago

Looks good to me, and it's indeed better than before.

Please would you file an issue for cleaning this up properly in the future? And reference this PR in it. Thanks