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

Upgrade zlib-ng #10375

Closed
wants to merge 1 commit into from
Closed

Upgrade zlib-ng #10375

wants to merge 1 commit into from

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Jan 7, 2025

Summary

If we remove this static linking, we can upgrade zlib-ng, which in turn lets us enable it on PowerPC.

@charliermarsh charliermarsh added the performance Potential performance improvement label Jan 7, 2025
@charliermarsh charliermarsh marked this pull request as ready for review January 7, 2025 18:30
#
# See: https://github.com/astral-sh/ruff/issues/11503
[target.'cfg(all(target_env = "msvc", target_os = "windows"))']
rustflags = ["-C", "target-feature=+crt-static"]
Copy link
Member Author

Choose a reason for hiding this comment

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

We did add this for a reason (astral-sh/ruff#11503) so we need to decide if it's important to retain.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean root cause of that issue was in zlib-ng?

Copy link
Contributor

Choose a reason for hiding this comment

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

No the issue is fundamental to windows and how it deals with ~system libraries. At least one thing we would regard as equivalent to "glibc" on linux is not shipped by default on windows (VCRUNTIME). When you ship a Windows application you are either expected to include an installer ("redistributable") for the version of VCRUNTIME that you need and invoke it to tell windows to add it to the system, or, you're expected to statically link it.

The above config chooses the latter, making us more statically linked in the same way that statically linking musl does. This is good for installing our binaries on any random Windows machine without additional installation stuff.

However it has a similar side-effect that musl does: if you need to dynamically link any other libraries you will likely get errors, because (handwaving) the other libraries dynamically link to VCRUNTIME-et-al and treat the contents of VCRUNTIME as protocol you need to interoperate with.

zlib-ng failing to link properly when we have this setting enabled is indicative of it wanting to dynamically link extra things, and running into the fact that we've essentially opted into "no you don't". (I haven't verified this but it's the only thing that makes sense to me).

By deleting this config (and doing nothing else), we are essentially opting into a lottery where we really hope all our users happened to have installed an appropriate version of VCRUNTIME before they install uv. This was the original issue ruff ran into. So we shouldn't remove it without a solution for redistributables (or research indicating this is just guaranteed to be on all supported systems now).

@zanieb zanieb requested review from Gankra and konstin January 9, 2025 00:32
@musicinmybrain
Copy link
Contributor

If you don’t end up merging this for some reason, please consider at least making the version bound libz-ng-sys = { version = "<1.1.20" } Windows-specific. (The comment above it references a Windows-specific issue, rust-lang/libz-sys#225.) Otherwise, I have to patch out the version bound in Fedora in order to use the system rust-libz-ng-sys-1.1.20 package, which is a minor but unnecessary inconvenience.

For now, I’ll just watch this PR, since it would remove the libz-ng-sys version bound altogether.

@charliermarsh
Copy link
Member Author

I tried that, but you actually can't make it Windows-specific. Cargo doesn't let you use multiple dependencies when linking a native library, apparently:

error: failed to select a version for `libz-ng-sys`.
    ... required by package `uv-performance-flate2-backend v0.1.0 (/Users/crmarsh/workspace/puffin/crates/uv-performance-flate2-backend)`
versions that meet the requirements `<1.1.20` are: 1.1.16, 1.1.15, 1.1.14, 1.1.13, 1.1.12, 1.1.11, 1.1.10, 1.1.9, 1.1.8, 1.1.7

the package `libz-ng-sys` links to the native library `z-ng`, but it conflicts with a previous package which links to `z-ng` as well:
package `libz-ng-sys v1.1.20`
    ... which satisfies dependency `libz-ng-sys = ">=1.1.20"` of package `uv-performance-flate2-backend v0.1.0 (/Users/crmarsh/workspace/puffin/crates/uv-performance-flate2-backend)`
Only one package in the dependency graph may specify the same links value. This helps ensure that only one copy of a native library is linked in the final binary. Try to adjust your dependencies so that only one package uses the `links = "z-ng"` value. For more information, see https://doc.rust-lang.org/cargo/reference/resolver.html#links.

@musicinmybrain
Copy link
Contributor

I tried that, but you actually can't make it Windows-specific. Cargo doesn't let you use multiple dependencies when linking a native library, apparently:

That kind of almost makes sense – mostly – in retrospect… I think… possibly. Anyway, thanks for attempting it!

#
# See: https://github.com/astral-sh/ruff/issues/11503
[target.'cfg(all(target_env = "msvc", target_os = "windows"))']
rustflags = ["-C", "target-feature=+crt-static"]
Copy link
Contributor

Choose a reason for hiding this comment

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

No the issue is fundamental to windows and how it deals with ~system libraries. At least one thing we would regard as equivalent to "glibc" on linux is not shipped by default on windows (VCRUNTIME). When you ship a Windows application you are either expected to include an installer ("redistributable") for the version of VCRUNTIME that you need and invoke it to tell windows to add it to the system, or, you're expected to statically link it.

The above config chooses the latter, making us more statically linked in the same way that statically linking musl does. This is good for installing our binaries on any random Windows machine without additional installation stuff.

However it has a similar side-effect that musl does: if you need to dynamically link any other libraries you will likely get errors, because (handwaving) the other libraries dynamically link to VCRUNTIME-et-al and treat the contents of VCRUNTIME as protocol you need to interoperate with.

zlib-ng failing to link properly when we have this setting enabled is indicative of it wanting to dynamically link extra things, and running into the fact that we've essentially opted into "no you don't". (I haven't verified this but it's the only thing that makes sense to me).

By deleting this config (and doing nothing else), we are essentially opting into a lottery where we really hope all our users happened to have installed an appropriate version of VCRUNTIME before they install uv. This was the original issue ruff ran into. So we shouldn't remove it without a solution for redistributables (or research indicating this is just guaranteed to be on all supported systems now).

@charliermarsh
Copy link
Member Author

Closing based on @Gankra's great diagnosis.

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

Successfully merging this pull request may close these issues.

4 participants