search

xlsynth / xlsynth-crate

Rust crate that publishes XLSynth capabilities (wrapping libxlsynth.so/.dylib)
https://crates.io/crates/xlsynth
Apache License 2.0
0 stars 1 forks source link

consider static linking #1

Open proppy opened 3 weeks ago

proppy commented 3 weeks ago

I was wondering if you've considered statically linking to XLS c_api?

Ideally that could allow us to detect breakage to incompatible API/signature change at build time rather having to execute a battery of tests to ensure that all dynamically loading path are covered.

What do you think?

Reference:

proppy commented 2 weeks ago

wrapping the c_api.rs functions in extern "C" block as described in https://docs.rust-embedded.org/book/interoperability/c-with-rust.html and reusing the the code to unmarshal/marshal the arguments/return values should be enough to allow linking to //xls/public:c_api statically.

cdleary commented 2 weeks ago

I think the benefit of the shared library conceptually is that you could make a binary release of the xlsynth-crate independent of the shared object's binary release, but if we consider environments where source is always available it shouldn't make a big difference.

I imagine we could make a build option to either use a dynamic loading c_api.rs or a statically linking one.

Note that static vs dynamical linking does not help with signatures -- to ensure consistency they'd need to be converted from some source of truth, e.g. a C header specification, and we're trying to avoid relying on anything from C++ build time in this crate other than output artifacts.

If having a static linking mode would be helpful for the crate usability we can consider adding that (e.g. behind a feature flag), but I don't think it has any particular advantages for use cases I'm aware of, given we're going to keep the C++ build step (that produces the library) separated.

proppy commented 2 weeks ago

I think the benefit of the shared library conceptually is that you could make a binary release of the xlsynth-crate independent of the shared object's binary release, but if we consider environments where source is always available it shouldn't make a big difference.

We could also release a static version of libxls.a with accompagning C headers if we wanted to preserve this property.

I imagine we could make a build option to either use a dynamic loading c_api.rs or a statically linking one.

From a create usuability stand point, would it make a difference if it was referring to a dynamic loading c_api.rs or a statically linking one?

One difference I could see is that symbol resolution could happen at build time (so on cargo install) rather than at execution time, but if crate are pinned to specific version of the output artefact (as you pointed later) that shouldn't really be an issue for the end-user (as long as maintainer ensure that all dynamically loading path are properly covered by tests).

Note that static vs dynamical linking does not help with signatures -- to ensure consistency they'd need to be converted from some source of truth, e.g. a C header specification, and we're trying to avoid relying on anything from C++ build time in this crate other than output artifacts.

The rust ecosystem seems to have tool like https://github.com/rust-lang/rust-bindgen which support generating binding for C API, but my initial thought was the binding signature could be maintained manually in https://github.com/xlsynth/xlsynth-crate/blob/main/src/c_api.rs simarly to what it does for the dynamic symbol lookup today.

If having a static linking mode would be helpful for the crate usability we can consider adding that (e.g. behind a feature flag), but I don't think it has any particular advantages for use cases I'm aware of, given we're going to keep the C++ build step (that produces the library) separated.

The static approach would definitly be easier to adapt to Google internal build environment.

cdleary commented 2 weeks ago

Sounds good, as I mentioned I don't think we benefit much from dynamic linkage given it's all source-available on crates.io, so pulling from a versioned .a release in build.rs and adding it to the linker flags from there SGTM as a change if it works!

proppy commented 2 weeks ago

I mentioned I don't think we benefit much from dynamic linkage given it's all source-available on crates.io

Do you envision maintaining both, or just keeping the static linkage variant? I have a proof of concept of the later here https://github.com/xlsynth/xlsynth-crate/commit/b951a6ecfb62edfd1d497b7e6e1c9cfdc6c6a362 that I'm happy to submit as a PR and iterate on.

In particular I'm not sure how it affects the MacOSX build path.

cdleary commented 2 weeks ago

Let's have both behind a cargo feature for now until we can firm up the OS X side, but I imagine it'd work there (or would be similarly easy to make work there) as well once we have the Linux side going.

proppy commented 1 week ago

as commented in https://github.com/xlsynth/xlsynth/pull/1; it turns out bazel is currently limited in it's ability to construct standalone .a artifact https://github.com/bazelbuild/bazel/issues/1920 (that's being addressed in https://github.com/bazelbuild/bazel/pull/16954)

proppy commented 1 week ago

Managed to get dynamic loading working in google's internal environment (with https://github.com/xlsynth/xlsynth-crate/pull/12), though even if it would be nice to support static linking it's not strictly required anymore.

cdleary commented 2 days ago

If we've got something working let's consider it just nice-to-have? We can always revisit as we find it's more needed.