Skip to content
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

Support BUILD gen for rust-libloading #38

Open
acmcarther opened this issue Mar 28, 2018 · 5 comments
Open

Support BUILD gen for rust-libloading #38

acmcarther opened this issue Mar 28, 2018 · 5 comments

Comments

@acmcarther
Copy link
Member

libloading recently made a change to bundle C file to have a weak mutex:
nagisa/rust_libloading#32

This is a corner case of raze -- its not really possible to either

  1. Run the build script and scoop up the build object file

OR

  1. Override via raze.crate settings what it would have done, without just overriding the whole crate.

Furthermore, the build script itself depends on some flags that I wasn't aware of:

    let target_os = env::var("CARGO_CFG_TARGET_OS");
    let is_unix = env::var_os("CARGO_CFG_UNIX").is_some();

For now, I'm overriding the whole crate. This exposes an ergonomic painpoint: It should be possible to workspace-wide replace one target with another.


As a side note, I would benefit from this particular attr being stabilized, but it doesn't seem like that will happen any time soon:
rust-lang/rust#29603

That is to say, the small-ish missing stable feature of specifying weak linkage led to rust-libloading bringing in C code to replace it, which is hard to automatically detect and build on my end. What they had before (lazy_static) worked in 99.9% of circumstances, until some other random feature required them to up their major version, dramatically increasing the chances of the crate being linked twice (which would potentially lead to bad behavior when using lazy_static).

Yikes.

@nagisa
Copy link

nagisa commented Mar 28, 2018

Hi!

Would it help you if I ended up implementing a configuration switch in the library which would allow you to "pick" the old lazy_static approach?

It would still need you to figure out how to emit the necessary CARGO_CFG environment variables as well as giving rustc the relevant cfg flags, but it would definitely help in that libloading would no longer generate a C helper.

For reference, nagisa/rust_libloading#32 is the bug that the weakly linked mutex is fixing.

@acmcarther
Copy link
Member Author

acmcarther commented Mar 28, 2018

Hello @nagisa! I appreciate your input here!

It would not likely be the best use of your time to add back support for lazy_static. Although I would be aware of the flag (and use it!), there's not currently a way for that knowledge to be "disseminated" to other folks who use cargo-raze, and the Mutex fix as currently implemented is a fix to a real problem that users of cargo-raze aren't immune to. Furthermore, embedded CC deps is a problem that this tool needs to learn how to handle anyway...

I'm actually personally interested in rust-lang/rust#29603 and #[linkage(weak)] as it would make another library I wrote practical to use in Cargo contexts: one that also requires there to be only one copy of some static state, as the crate plays a sort of similar type of role as log. Do you have any insider perspective that might indicate whether or not pushing stabilization of #[linkage(weak)] in isolation would be productive?


action items for me:

  • Follow up on linkage(weak)
  • Prototype support of a mechanism for sharing suggested crate raze settings.
  • Consider support for extending BUILD files (perhaps in a target-aware manner) from Cargo.toml's raze settings
  • Add workspace-wide override section to raze settings in Cargo.toml.
  • Add CARGO_CFG flags to generated build script runners

notes: Override procedure https://gist.github.com/acmcarther/8473901b5b45ae338776b9c66731c116

@aaliddell
Copy link

What's the current recommended way of supporting libloading with raze in a cross-platform supporting way, since those env vars are not set? Is it still to follow the procedure you've got in that gist?

I ask since that procedure doesn't presently appear to support anything other than Linux, but could conceivably with some helpfully places select()s. I thought I'd check before trying to figure those fixes out 👍

Alternatively, could this be achieved in a similar way with gen_buildrs = false, additional_deps and additional_build_file on libloading itself? This'd avoid having to copy the libloading source into a local overrides directory, if I understand correctly.

@aaliddell
Copy link

As of bazelbuild/rules_rust@e64700d this now appears to just work with gen_buildrs = true 🎉

@nagisa
Copy link

nagisa commented Apr 9, 2021

This is probably no longer a relevant issue as libloading doesn't have a build.rs anymore and can be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants