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.

Sign up for GitHub

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
+6 −17
Closed

Upgrade zlib-ng #10375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions .cargo/config.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,2 @@
[alias]
dev = "run --package uv-dev --features dev"

# statically link the C runtime so the executable does not depend on
# that shared/dynamic library.
#
# 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).

5 changes: 2 additions & 3 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 2 additions & 3 deletions crates/uv-performance-flate2-backend/Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 2 additions & 4 deletions crates/uv-performance-flate2-backend/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,9 @@ edition = "2021"
doctest = false

# Use `zlib-ng` on all supported platforms.
[target.'cfg(not(any(target_arch = "s390x", target_arch = "powerpc64", target_os = "freebsd")))'.dependencies]
[target.'cfg(not(any(target_arch = "s390x", target_os = "freebsd")))'.dependencies]
flate2 = { version = "1.0.28", default-features = false, features = ["zlib-ng"] }
# See: https://github.com/rust-lang/libz-sys/issues/225
libz-ng-sys = { version = "<1.1.20" }

# Use `zlib-rs` everywhere else.
[target.'cfg(any(target_arch = "s390x", target_arch = "powerpc64", target_os = "freebsd"))'.dependencies]
[target.'cfg(any(target_arch = "s390x", target_os = "freebsd"))'.dependencies]
flate2 = { version = "1.0.28", default-features = false, features = ["zlib-rs"] }
Loading