consider static linking

[proppy]

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:

• https://doc.rust-lang.org/cargo/reference/build-scripts.html#rustc-link-lib
• https://doc.rust-lang.org/rustc/command-line-arguments.html#-l-link-the-generated-crate-to-a-native-library

[proppy]

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]

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]

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]

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]

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 b951a6e 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]

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]

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

[proppy]

Managed to get dynamic loading working in google's internal environment (with #12), though even if it would be nice to support static linking it's not strictly required anymore.

[cdleary]

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.