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

Add new feature flag "static" #159

Closed
wants to merge 2 commits into from
Closed

Add new feature flag "static" #159

wants to merge 2 commits into from

Conversation

NobodyXu
Copy link
Contributor

Resolved #158

Signed-off-by: Jiahao XU Jiahao_XU@outlook.com

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Copy link
Contributor Author

Not sure why the CI for windows i686 failed.

@@ -200,7 +200,9 @@ fn main() {
}

// println!("cargo:rustc-link-lib=zstd");
let (defs, headerpaths) = if cfg!(feature = "pkg-config") {
let (defs, headerpaths) = if cfg!(feature = "pkg-config")
&& !cfg!(feature = "static")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should make it an error to have both pkg-config and static features, rather than silently dropping one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer static to override pkg-config, since it is possible for one dependency to request pkg-config and another to request static.

In this situation, it is not always possible to modify or it maybe hard to remove that and I would prefer statically linked to dynamically linked zstd.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a dependency asks for pkg-config, we should honor this decision and not discard it.

Though in general asking for pkg-config is not something re-usable libraries should be doing, it's more of a end-of-the-line binary decision.

@gyscos
Copy link
Owner

gyscos commented Jul 21, 2022

Note that static linking is already the default.
I believe the only way to build dynamically right now is using the pkg-config feature, which is a conscious decision.

  • If you're not using this feature right now, then you are statically linking already.
  • If you are using this feature right now and it leads to dynamic linking, then something explicitly asked for dynamic linking. It would be safer to find where this came from and fix that, than to silently disregard this request.

The main reason to add a static feature (and it may be a valid one!) is to detect if the situation ever changes: if some dependency starts requiring pkg-config, rather than silently switching to dynamic linking, it would at least block compilation to make the change visible.

Also note that pkg-config does not have to result in dynamic linking. It will try static linking if the libzstd.a file is available, but if it's not - for example the archlinux zstd package does not ship static libraries - it will fallback to dynamic linking.

In that case, it may be better to detect when pkg-config fails to enable static linking (it uses a private function to check that), and only then trigger a build failure.
Now - how to detect this failure? We can either re-implement this function locally? Talk to the pkg_config maintainers and make it public? There doesn't seem to be an easy-to-use field in the resulting Library that would indicate this. :^S

@NobodyXu
Copy link
Contributor Author

@gyscos I just realized that pkg_config function in build.rs set pkg_config::Config::statik to true, so maybe we don't need this new feature afterall?

@gyscos
Copy link
Owner

gyscos commented Jul 21, 2022

Also note that pkg-config does not have to result in dynamic linking. It will try static linking if the libzstd.a file is available, but if it's not - for example the archlinux zstd package does not ship static libraries - it will fallback to dynamic linking.

It's an unfortunate decision, as it does not appear to make this very visible to the caller.

Seems like there's some demand to change things, but nothing done so far:

rust-lang/pkg-config-rs#68
rust-lang/pkg-config-rs#102
rust-lang/pkg-config-rs#37

@NobodyXu NobodyXu closed this by deleting the head repository Oct 13, 2022
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.

Add a feature flag "static" to ensure libzstd is statically linked
2 participants