Add static-libtiledb feature #186

davisp · 2024-10-22T22:28:23Z

This allows for building libtiledb statically.

davisp · 2024-11-07T19:06:41Z

@rroelke This is reviewable finally. Apologies for forgetting about it.

rroelke

My comments are mostly nits, the exception being

The hard /opt/tiledb/lib path seems wrong

But on top of that, \this seems like something which really ought to have dedicated CI jobs, doesn't it?
A build on Linux and MacOS at least.

rroelke · 2024-11-08T13:54:43Z

tiledb/sys/build/current_os/linux.rs

+    let mut tdb = std::path::PathBuf::from(build_dir);
+    tdb.extend(["tiledb", "libtiledb.a"]);
+    if !tdb.is_file() {
+        panic!("Missing libtiled: {}", tdb.display());


Nit - should be MIssing libtiledb, no?

I actually just went with `Missing static library: {}" cause the tdb.display() includes the full path anyway.

rroelke · 2024-11-08T15:14:25Z

tiledb/sys/build/command.rs

+                out.unwrap_or("".to_string()),
+                err.unwrap_or("".to_string())
+            );
+            return Err(Error::Git(msg));


FWIW the function here runs any subprocess command, there's nothing git-specific here. Seems like this should return up a generic error and then the caller in repo.rs should wrap Error::Git around that.

Yeah, this was some refactoring gone dumb. I've since removed all of this and the Error::Git variant completely.

rroelke · 2024-11-08T15:16:09Z

tiledb/sys/build/command.rs

+
+    // Wait for git to finish executing
+    loop {
+        let result = git.poll();


Why not wait?

Probably because I didn't take the time to read the docs very thoroughly. I happened to stumble over a good example of using std::process::Command this morning so I've since switched everything to that and its turned out cleaner.

rroelke · 2024-11-08T15:26:43Z

tiledb/sys/build/main.rs

+        libdir
+    );
+
+    println!("cargo:rustc-env=DYLD_LIBRARY_PATH=/opt/tiledb/lib");


The hard /opt/tiledb/lib path seems wrong

It was, you caught me in the middle of debugging issues I didn't catch until attempting to use this in a separate repository for a complete static binary.

davisp · 2024-11-08T20:05:42Z

@rroelke Originally I was just going to rely on some tests in another repository for the CI checks of static builds, but doing that exposes the fact that its a really terrible way to develop this feature. I've added a nightly workflow on my fork and am currently testing that we get static builds correctly. I'll give you a holler when that's done and reflected in this PR. I just wanted to avoid you having the same 50+ emails about nightlies failing as I do the CI dev work.

davisp · 2024-11-08T20:28:45Z

tiledb/api/src/string.rs

@@ -34,15 +34,11 @@ impl TDBString {
    }

    pub fn to_string(&self) -> TileDBResult<String> {
-        let mut c_str: *const i8 = out_ptr!();
+        let mut c_str = out_ptr!();


Since these are non-obvious, I'll note that the API uses c_char in its signature. Unfortunately, c_char's signedness isn't defined and I happened to be the first to find a platform where its u8. Rather than cast, I've just removed the type and rely on inference now.

davisp · 2024-11-08T20:47:50Z

@rroelke Apologies for getting side tracked and not telling you to hold off on your review while I fixed things. But things are now actually really fixed.

This includes a new "Static Nightlies" CI workflow that runs on Ubuntu and MacOS and actually asserts that there's no dynamic linkage after the tests complete. I've developed that CI workflow on my personal fork and you can see that its passing here:

https://github.com/davisp/tiledb-rs/actions/runs/11749237318

Also, to show that the static assert works on Linux, I ran this temporary commit through the standard PR build with the check enabled and it fails as expected:

https://github.com/davisp/tiledb-rs/actions/runs/11749234997/job/32735029329

Unfortunately I can't really link to those on this repo until after this PR is merged, but hopefully the runs on my fork will be convincing enough.

This updates the tiledb-sys crate to be able to build a static version of libtiledb. This allows Rust projects to create statically linked binaries for distribution.

rroelke · 2024-11-12T20:20:36Z

tiledb/sys/build/repo.rs

+    let out_dir = utils::out_dir().display().to_string();
+    let output = Command::new("git")
+        .arg("clone")
+        .arg("https://github.com/TileDB-Inc/TileDB")


Maybe this should be environmental so a network connection isn't required. So that you can point at a local filesystem path. That's also potentially useful for testing against non-master.

Similarly, should there be an argument to clone the latest release? or nightly? or something?

Yep, I've not added this as I'm not yet certain how we'll end up specifying versions of tiledb to link against when we get to formalized releases of tiledb-rs. I've opened a story here: https://app.shortcut.com/tiledb-inc/story/59243

rroelke

Just the one note about where to clone core from, otherwise let's merge!

davisp force-pushed the pd/sc-51274/static-build branch from f7aa6d8 to 7750e68 Compare October 23, 2024 21:35

davisp force-pushed the pd/sc-51274/static-build branch 4 times, most recently from 44f7367 to a7417c6 Compare November 7, 2024 19:05

davisp requested a review from rroelke November 7, 2024 19:06

rroelke reviewed Nov 8, 2024

View reviewed changes

davisp force-pushed the pd/sc-51274/static-build branch from b408abc to 3db0f84 Compare November 8, 2024 17:14

davisp force-pushed the pd/sc-51274/static-build branch from 3db0f84 to e865bb8 Compare November 8, 2024 20:23

davisp commented Nov 8, 2024

View reviewed changes

davisp force-pushed the pd/sc-51274/static-build branch from e865bb8 to 1566417 Compare November 8, 2024 20:41

davisp requested a review from rroelke November 8, 2024 20:44

davisp force-pushed the pd/sc-51274/static-build branch from 1566417 to dd7fd52 Compare November 8, 2024 20:48

Add ability to use static builds of libtiledb

f446234

This updates the tiledb-sys crate to be able to build a static version of libtiledb. This allows Rust projects to create statically linked binaries for distribution.

davisp force-pushed the pd/sc-51274/static-build branch from dd7fd52 to f446234 Compare November 8, 2024 20:50

rroelke reviewed Nov 12, 2024

View reviewed changes

rroelke approved these changes Nov 12, 2024

View reviewed changes

davisp merged commit b8ccc88 into main Nov 12, 2024
3 checks passed

davisp deleted the pd/sc-51274/static-build branch November 12, 2024 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add static-libtiledb feature #186

Add static-libtiledb feature #186

davisp commented Oct 22, 2024

davisp commented Nov 7, 2024

rroelke left a comment

rroelke Nov 8, 2024

davisp Nov 8, 2024

rroelke Nov 8, 2024

davisp Nov 8, 2024

rroelke Nov 8, 2024

davisp Nov 8, 2024

rroelke Nov 8, 2024

davisp Nov 8, 2024

davisp commented Nov 8, 2024

davisp Nov 8, 2024

davisp commented Nov 8, 2024

rroelke Nov 12, 2024

davisp Nov 12, 2024

rroelke left a comment

Add static-libtiledb feature #186

Add static-libtiledb feature #186

Conversation

davisp commented Oct 22, 2024

davisp commented Nov 7, 2024

rroelke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davisp commented Nov 8, 2024

Choose a reason for hiding this comment

davisp commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rroelke left a comment

Choose a reason for hiding this comment