search

Drup / ocaml-lmdb

Ocaml bindings for lmdb.
https://drup.github.io/ocaml-lmdb/dev/
MIT License
48 stars 3 forks source link

Add some conditional compilation to handle different lmdb versions #5

Closed Drup closed 4 years ago

Drup commented 5 years ago

@madroach I think we really need something like that for reliability, especially in CI.

Drup commented 5 years ago

Alternative proposal: pick a version of lmdb, and bundle it.

madroach commented 5 years ago

There is a bug fixed in lmdb 0.9.21. That's why I did 48b676c7deea699bac9985018badc36bc79a8295. I'm wondering if this somehow stopped working. I'll try to fix it with #20. I don't yet see the need for conditional compilation or bundling a specific version yet. Its not our job to fix bugs in the lmdb backend by working around them in the binding. The only problem we are facing at the moment are failing tests in CI. But they are supposed to fail, because they trigger a bug. I would rather "recommend-depend" on lmdb >=0.9.21.

Drup commented 5 years ago

Its not our job to fix bugs in the lmdb backend by working around them in the binding.

Well, that would be a strong point towards bundling a version of lmdb in the library. This would allow us to choose the version, and upgrade it at your own pace.

madroach commented 5 years ago

The point I was trying to make is that we should not work around bugs in the lmdb backend by any means. This includes bundling lmdb, too. The other point was that there is currently no need to take action in that direction, because the binding will compile even on older versions of the back-end lmdb. Just the tests will fail, and rightfully do so. The only problem we face at the moment is CI using a buggy lmdb. I would rather fix this in CI.

madroach commented 5 years ago

Nice. Travis works again in #20. :satisfied:

Drup commented 5 years ago

Well, the reason I propose conditional compilation and/or bundling is not to walk around bugs, it's to be able to bind functions in newer versions of lmdb.

madroach commented 5 years ago

Ah, ok. That's sensible. At the moment I'm hoping for the backend api to stay stable :sweat:

madroach commented 5 years ago

Since the lmdb backend as well as the stubs are written in C, we can use the C preprocessor and MDB_VERSION_* macros to do conditional compilation - at least on the C stubs side of things. On non-availability of some feature in the backend we would then be able to raise an exception like Invalid_argument "not supported by this version of lmdb, need at least 0.9.21". This won't allow changes in the OCaml interface obviously, and therefore only allow to fail at runtime.

madroach commented 4 years ago

Can we close this issue for now?

Drup commented 4 years ago

The problem is still the same, I don't see why we should close. It's almost impossible to distribute the library without either bundling, or handling multiple version of the C library.

madroach commented 4 years ago

Can you elaborate? What problem are we facing now? Is this a blocker for release?

Drup commented 4 years ago

Yes, it is a blocker: the ocaml bindings have to be compatible with the installed version of lmdb, which is installed by the system, and thus we don't have control over its version.

Either we bundle lmdb, and choose a specific version, or we make sure we are compatible with many versions.

madroach commented 4 years ago

I believe we are compatible with many versions. Debian oldstable uses 0.9.14. You think this would be a reasonable version to start support? I can do some tests this evening.

madroach commented 4 years ago

I tested lmdb 0.9.14…0.9.24. This is what I found:

The oldest lmdb versions I could find on Unix distributions are

Bundling lmdb is problematic, because at least on OpenBSD it needs to be patched to always use WRITE_MAP due to the missing unified buffer cache on OpenBSD. OpenBSD ships with a patched version we can rely on. I don't know about other OSes.

Drup commented 4 years ago

Ok, this is fairly convincing. I feel like we should try to test that in CI, but it's always complicated.

madroach commented 4 years ago

Since I'm pretty annoyed of errors in travis that I cannot reproduce locally I'm very reluctant to adding complexity to the travis scripts.x If you want to ensure we stay compatible with let's say 0.9.17, we could just remove the update to 0.9.21 from .travis-ci.sh and tweak the test cases to allow failure on lmdb <0.9.21. Could you please clarify what you actually want to test.

Drup commented 4 years ago

Could you please clarify what you actually want to test.

The different systems, with their different lmdb versions. We do not have to do this now, just something that would be good.

madroach commented 4 years ago

Ok. I see. Testing builds against different versions of lmdb is not difficult to script. But this has nothing to do with conditional compilation. So may we close this issue now?

Drup commented 4 years ago

You really want to close all the issues. :D

Yes, we can