Skip to content

rust - re-export libceed-sys features through libceed to allow building non-static without using libceed-sys directly #1813

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jedbrown
Copy link
Member

Fix #1694

I will note that there is nothing wrong with depending on libceed-sys directly to set the feature at that level, so I don't think this is a necessary change, just a convenience.

Thanks-to: @eliasboegel

…ng non-static without using libceed-sys directly
@eliasboegel
Copy link

Thanks for bringing this up again and creating the PR!

Out of curiosity: Are you sure that this isn't a necessary change in order to link libceed dynamically?
Because of how features are intended to be additive only and are unified under that assumption,

[dependencies]
libceed-sys = { version="0.12", default-features=false}

will result in libceed-sys being built as a shared library, but

[dependencies]
libceed-sys = { version="0.12", default-features=false}
libceed = "0.12

will result in libceed-sys being built as a static library even though the static feature is explicitly disabled with default-features=false. This is because libceed brings in the default static feature, overridingdefault-features=false in the libceed-sys specification.

The possible fixes are either:

  1. Re-export of the libceed-sys features like in this PR to allow:
[dependencies]
libceed = { version="0.12", default-features=false}
  1. Opting for the "inverse feature" in libceed-sys, i.e. having an optional (additive), non-default shared feature instead of a default static feature, such that libceed doesn't enable any feature. This should allow doing:
[dependencies]
libceed-sys = { version="0.12", shared=true}
libceed = "0.12"

Happy to hear your thoughts, and if you prefer 2), then I'm happy to implement that instead of the feature re-export.

@jedbrown
Copy link
Member Author

I should have written more carefully. I was thinking of having libceed/Cargo.toml use

[dependencies]
libceed-sys = { version = "0.12", path = "../libceed-sys", default-features = false}

but skipping the re-exporting of features. I think that supports all use cases, but requires transitive naming of libceed-sys to configure it. However, one is supposed to avoid having mutually-exclusive features whenever possible. What if we removed the static feature from libceed-sys and just leave the non-default system feature? (We can re-export it as a convenience.)

@eliasboegel
Copy link

Ah, thanks for clarifying. I'm personally not a fan of depending on transitive naming and instead in favor of removing the static feature from libceed-sys. I still think it would be important to have a shared build available due to #1694, but that could be a non-default feature called shared or dynamic or similar (re-exported or not).

@jedbrown
Copy link
Member Author

jedbrown commented Apr 27, 2025

Do you mean something other than the existing system feature? To clarify, is there reason to support building a private shared libceed.so/libceed.dylib (in most cases implying that the user must set LD_LIBRARY_PATH or otherwise arrange for the library to be found), versus private (meaning non-system) builds always being static (libceed.a)?

This way no features are activated by default. It has the downside that
if only the shared feature is enabled, it will attempt to statically
link to the system library (which is most likely a shared library).
@eliasboegel
Copy link

Yes, in my mind they serve different purposes:
static (or a proposed shared): Building libCEED through Cargo
system: Use externally an built libCEED.

I have to admit that my motivation for a Cargo-built shared libCEED is quite niche. I would like to retain the ability to do this purely because of the weak symbol problem on MacOS when linking statically (#1694), which I have not been able to solve, so I link libCEED dynamically as a bandaid solution.

I can absolutely see how thats not a common case though and can see an argument for removing private shared builds to streamline the features. In that case I'll need to separately build/install libCEED while on my MacOS machine instead of having Cargo build it. That would be a little inconvenient but acceptable since that'll be necessary for any device support anyways.

@jedbrown
Copy link
Member Author

Okay, I pushed a commit to swap the feature names, but it'll use pkg-config --static when trying to find the system library, unless one sets both system and shared. (If we keep it, I'll document that behavior, but I'm not sure I like it.) What do you think?

@eliasboegel
Copy link

This works for me and I think this is a relatively good solution. What's your concern with it?

@jedbrown
Copy link
Member Author

Just that activating system only will tend to overlink or possibly fail, since system libs are usually shared libs. So it's a bit more user complexity to remember to activate both system and shared.

@eliasboegel
Copy link

I see. If you feel that this is a significant factor then I find removing shared/static altogether acceptable. It will remove the ability to use a Cargo-built libCEED on MacOS while #1694 persists, but the workaround is dynamically linking an externally built libCEED. For me personally that wouldn't be a problem. Can you see any use-case for statically linking an externally built libCEED? If not, then I think we could move to only system.

@jedbrown
Copy link
Member Author

jedbrown commented May 1, 2025

Revisiting this while trying to understand the root issue of weak symbols not working with static linking on MacOS. I don't have a solution there, but I found that ar crD doesn't work because Apple ar does not support D (deterministic), but one can use the environment variable ZERO_AR_DATE=1.
rust-lang/rust#47086 (comment)

@eliasboegel
Copy link

I also faced the ar D issue, and worked around it by using a third party bintools distribution rather than that shipped with Apple Clang. Made no difference on the weak symbol issue though.

@jeremylt jeremylt added this to the v0.13 milestone May 6, 2025
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.

Rust bindings fail to build built-in backends on Darwin aarch64
3 participants