search

curl / curl-for-win

Reproducible curl binaries for Linux, macOS and Windows
https://curl.se/windows/
MIT License
694 stars 207 forks source link

zstd support #34

Closed gvollant closed 2 years ago

gvollant commented 2 years ago

Hello

commit 6318ab34cbbb421b3723f7ee0a6652d2c6ba0cb3 removed zstd support with this text:

zstd release 1.5.1 broke the build in multiple ways.

Ref: https://ci.appveyor.com/project/curlorg/curl-for-win/builds/41967466
Ref: https://ci.appveyor.com/project/curlorg/curl-for-win/builds/41968643

@Cyan4973 @felixhandte do you have an idea on the problem ?

I was very happy to see the windows build of curl comiled with zstd 1.5.0

vszakats commented 2 years ago

@gvollant: zstd has apparently been fixed after those fallouts. I've experimentally restored support just recently in e80600697ddc9bc079e9afcf1ce4e448e0b95518. It's now enabled in the alternate build called "big". It adds 150 KB (which is lower than in some pre-1.5.0 versions).

Every dependency is an extra weight/attention/time to carry, so is zstd support "common enough" on the server side to justify it? (I mean outside facebook properties)

(Moving brotli support to the "big" builds is also on the map, as I feel it doesn't contribute much in practice.)

Cyan4973 commented 2 years ago

I confirm that v1.5.1 was a short-lived version that featured a subtle build issue related to the presence of a single assembly file within the source tree, resulting in weird outcomes depending on the exact platform being used during the build.

v1.5.1 was superseded within a month by v1.5.2, which is current and fixed the issue. No more build issue have been reported since this update.

gvollant commented 2 years ago

@vszakats I understand the weight attention, but I think there is a "chicken and egg" problem between server and client for zstd (and brotli) support. So it'll be great if brotli and zstd came back on the standard windows binary distributed on the curl website.

By example, an experimental zstd nginx support was written before curl support zstd. It probably wait more client support to become more used https://github.com/tokers/zstd-nginx-module

vszakats commented 2 years ago

I've (re-)enabled zstd in default builds. Hoping this helps a little bit in server-side adoption.