Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release ocaml-rocksdb-0.02 #27360

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

chetmurthy
Copy link
Contributor

This ocaml-rocksdb interface is built using cppffigen, a tool that generates OCaml FFI bindings to C++ libraries that follow the Google C++ Style Guide for memory-ownership.

This might not be the best name for this package. There are already "orocksdb" and "ahrocksdb" packages (both based on ctypes); if you think this should be named something else, I'd be happy to change it.

This ocaml-rocksdb interface is built using cppffigen, a
tool that generates OCaml FFI bindings to C++ libraries
that follow the Google C++ Style Guide for memory-ownership.
@chetmurthy chetmurthy force-pushed the release-ocaml-rocksdb-0.02 branch from 5440587 to 9c78ce6 Compare February 4, 2025 07:53
"--personality=i686-w64-mingw32" {os = "win32" & os-distribution = "cygwin" & host-arch-x86_32:installed}
"--personality=x86_64-w64-mingw32" {os = "win32" & os-distribution = "cygwin" & host-arch-x86_64:installed}
"pkg-config" {os != "win32" | os-distribution != "cygwin"}
"--atleast-version=7.8.3" "rocksdb"] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this 7.8.3 constant correct? what's its relation to the "ge-5-17-2" aim?

!(os-distribution = "ubuntu" & (os-version = "20.04" | os-version = "22.04"))
}
]
synopsis: "Virtual package relying on a system installation of RocksDB (version >= 5.17.2)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the need for this kind of package, and I wish we had a good solution (or at least a standard solution) to offer but right nnow it's an unsolved problem for the opam repo.

I'm not very happy with adding version constraints as part of the name of a package. Although I can see how it's needed and how we can't reasonably generate conf-rocksdb packages for each available version of rocksdb. Or maybe that's reasonable but would require having a separate dedicate opam repository.

Anyway… Anyone has ideas?

Otherwise I guess we can possibly merge for now and possibly revisit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually worse and worse. When I started with trying to get this to work, I foolishly assumed that pkg-config would be available to check rocksdb's package-version. But (haha, silly me) that wasn't the case with these older versions of rocksdb like (sigh) current Debian (11, 12) and backlevel Ubuntu (20.24, 22.04). And they were all backlevel versions of Rocksdb (5.17.2, 6.11.4). Other platforms had 7.8.3, though a few even failed the check.

I could put these changes into conf-rocksdb ?

The bigger problem of not being able to state version-constraints is .... painful. Some packages (like Rocksdb) appear to be changing their API pretty rapidly. But OTOH this is a big change to opam, and I sure think there are better ways to spend developer resource than on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we can accept the ge-… name for now

And maybe someone with time available can write a generator for many conf-packages where the version number is just substituted.

@@ -0,0 +1,59 @@

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we avoid having ocaml as part of the name of packages. See https://github.com/ocaml/opam-repository/tree/master/governance/policies#7-new-package-names-should-avoid-the-ocaml-prefixsuffix

I don't think this porject necessarily needs an exception. (Unless you can make a case for it?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to make it "rocksdb" ? Or any other name you would propose grin.

@shonfeder
Copy link
Contributor

Quite a few failing CI checks here on various platforms. Sounds like we'll need investigation there, in addition to the naming question. rocksdb seems fine, IMO.

Marking this accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants