Re: [RFC PATCH 3/6] contrib/cgit-rs: introduce Rust wrapper for libgit.a

On 2024.08.07 21:21, brian m. carlson wrote:

On 2024-08-07 at 18:21:28, Josh Steadmon wrote:

quoted

Introduce cgit-rs, a Rust wrapper crate that allows Rust code to call
functions in libgit.a. This initial patch defines build rules and an
interface that exposes user agent string getter functions as a proof of
concept. A proof-of-concept library consumer is provided in
contrib/cgit-rs/src/main.rs. This executable can be run with `cargo run`

Symbols in cgit can collide with symbols from other libraries such as
libgit2. We avoid this by first exposing library symbols in
public_symbol_export.[ch]. These symbols are prepended with "libgit_" to
avoid collisions and set to visible using a visibility pragma. In
build.rs, Rust builds contrib/cgit-rs/libcgit.a, which also contains
libgit.a and other dependent libraries, with -fvisibility=hidden to hide
all symbols within those libraries that haven't been exposed with a
visibility pragma.

I think this is a good idea.  It's optional and it allows us to add
functionality as we go along.  Platforms that don't have Rust can just
omit building it.

quoted

+[dependencies]
+libc = "0.2.155"

I don't love that we're using libc here.  It would be better to use
rustix because that provides safe APIs that are compatible with POSIX,
but I think for now we need this because rustix doesn't offer memory
management like free(3).  I'd really prefer that we didn't have to do
memory management in Rust, but maybe that can come in with a future
series.

Yeah, needing to free() is the only thing we striclty need from libc
right now. Please correct me if I'm wrong, but IIUC then any memory that
is allocated on the C side and then passed to Rust needs one of:
1) freed by libc::free() on the Rust side,
2) passed back to the C side to be freed there, or
3) leaked

Am I correct in assuming that your opinion is that writing additional
*_free() functions on the C side is worth it to avoid libc? If so, then
I'm fine with including that in V2.

libc also comes with the downside that it calls the actual libc
functions, so you have to write things like stat64 on Linux if you want
a 64-bit stat, but that doesn't exist on some of the BSDs, so you have
to write something else and compile it conditionally, and all of that
makes the portability of it worse than with C.

In any event, I have the intention to send a patch to replace libc with
rustix in the future if this series lands.

Even if we can't fully eliminate libc in V2, I'll keep this in mind and
avoid adding additional usage of libc as much as possible.

quoted

diff --git a/contrib/cgit-rs/public_symbol_export.c b/contrib/cgit-rs/public_symbol_export.c
new file mode 100644
index 0000000000..3d1cd6cc4f
--- /dev/null
+++ b/contrib/cgit-rs/public_symbol_export.c

@@ -0,0 +1,20 @@
+// Shim to publicly export Git symbols. These must be renamed so that the
+// original symbols can be hidden. Renaming these with a "libgit_" prefix also
+// avoid conflicts with other libraries such as libgit2.
+
+#include "contrib/cgit-rs/public_symbol_export.h"
+#include "version.h"
+
+#pragma GCC visibility push(default)

I assume this also works in clang?

Yep.

-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA