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

document new dlopen feature and packaging #165

Merged
merged 3 commits into from
Mar 1, 2022
Merged

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Feb 6, 2022

No description provided.

@decathorpe
Copy link

Hey, coming here from the "Fedora Devel" matrix room. (I'm the main Rust package maintainer in Fedora.)


You could simplify those cfg-expressions a little, by using some shortcuts documented here:
https://doc.rust-lang.org/reference/conditional-compilation.html

In particular, cfg(target_family = "windows") is equivalent to just using cfg(windows).

I've also very rarely seen target_vendor used anywhere in the Rust ecosystem, most multiplatform crates handle this differently. A GitHub code search yields only 185 results for usage of target_vendor in Cargo.toml files, and most of those only use it to gate things for WebAssembly environments. A search for target_os on the other hand yields thousands of results. For example, the winit crate does this to check for "apple platforms":

[target.'cfg(any(target_os = "ios", target_os = "macos"))'.dependencies]

I'm not even sure if target_vendor = "apple" even checks for the same thing here, checking for ios and macos explicitly might be good (if that's what you want). The second dependency is also a bit weird in that it states where this should not be used, instead of where it should be used. Inverting that might make it easier to understand for readers.

You could use something like this:

[target.'cfg(any(windows, target_os = "ios", target_os = "macos"))'.dependencies]
# Load libjack at runtime.
jack = "0.9"

[target.'cfg(all(unix, not(any(target_os = "ios", target_os = "macos"))))'.dependencies]
# Link libjack at build time.
jack = { version = "0.9", default-features = false }

This should be equivalent to what you wanted to say in the first place:

  • first snippet: for Windows, Mac OS, and iOS
  • second snippet: for all Unix-y platforms (except Mac OS and iOS)

Not really "simplified" on the whole, as I first implied, but I think it should be easier to understand for readers 😅 Hope that makes sense.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 6, 2022

Thanks for reviewing this @decathorpe. I also got useful feedback on the language in #lad on Libera Chat from people who know much more about JACK than I do. 😅

I like the suggestion to use the cfg(windows) shorthand. Unfortunately TOML doesn't allow to break these lines and they can become ridiculously long.

There are actually quite a bit of Apple targets supported by Rust which all use apple as the target_vendor. From the list of targets:

  • x86_64-apple-darwin
  • aarch64-apple-darwin
  • aarch64-apple-ios
  • aarch64-apple-ios-sim
  • x86_64-apple-ios (This exists?)
  • aarch64-apple-ios-macabi
  • aarch64-apple-tvos
  • armv7-apple-ios
  • armv7s-apple-ios
  • i386-apple-ios (Also, huh? This exists??)
  • i686-apple-darwin
  • x86_64-apple-ios-macabi
  • x86_64-apple-tvos

AFAIK nobody has ported JACK to iOS or tvOS, but I suppose it is theoretically possible so I think the bindings should be prepared for that. Also target_vendor = "apple" is simpler than any(target_os = "macos", target_os = "ios", target_os = "tvos").

The jack dependency should always be used, so I am keeping the two cfgs identical except for not.

@decathorpe
Copy link

Not sure if you're still looking for feedback wrt. the dlopen feature / packaging, but since I'm still getting notifications every time this repo is updated ... either way, using environment variables for this is a really brittle solution that involves lots of error-prone manual labor. Packages for Rust crates don't "inherit" any environment variables for dependent crates - so you have to specify them when building them in crate itself, in all dependent crates that use that feature, and in any applications that uses that feature, recursively - instead of one crate specifying a dependency with a certain feature enabled or disabled (which does propagate through the dependency graph, unlike environment variables).

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 17, 2022

I think you're misunderstanding the purpose of the environment variable. The point is to let someone building a downstream application choose whether to enable the dlopen feature without needing to pass down the feature through the Cargo.toml of the application and its dependency graph leading down to this crate. The recommendation is still to not use dlopen by default on Linux. The environment variable allows turning on it just for cross compiling. Refer to this discussion.

@Be-ing Be-ing force-pushed the dlib_docs branch 2 times, most recently from 44b6efa to b7d4e29 Compare February 22, 2022 02:48
@Be-ing
Copy link
Contributor Author

Be-ing commented Mar 1, 2022

ping @wmedrano

@wmedrano wmedrano merged commit 0d3eaf2 into RustAudio:main Mar 1, 2022
@Be-ing Be-ing deleted the dlib_docs branch March 1, 2022 22:24
@Be-ing
Copy link
Contributor Author

Be-ing commented Mar 2, 2022

Thanks for merging. Glad we got this working.

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

Successfully merging this pull request may close these issues.

3 participants