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

Multiple content encodings and Brotli support #2045

Closed
wants to merge 10 commits into from
Closed

Multiple content encodings and Brotli support #2045

wants to merge 10 commits into from

Conversation

monnerat
Copy link
Contributor

@monnerat monnerat commented Nov 3, 2017

Although I wrote a test for the multi-encoding, I have not been able to find a real server resource providing such data.

I've checked Brotli successfully both with tests and in the real world.

This is implemented as an output streaming stack of unencoders, the last
calling the client write procedure.

New test 230 checks this feature.

Bug: #2002
Reported-By: Daniel Bankhead
This uses the brotli external library (https://github.com/google/brotli).
Brotli becomes a feature: additional curl_version_info() bit and
structure fields are provided for it and CURLVERSION_NOW bumped.

Tests 314 and 315 check Brotli content unencoding with correct and
erroneous data.

Some tests are updated to accomodate with the now configuration dependent
parameters of the Accept-Encoding header.
Also complete initialisation of version info data structure.
@bagder
Copy link
Member

bagder commented Nov 3, 2017

How about an added test or two for it as well? (oops, I missed that they already existed!)

And if you're brave: try adding a travis build that uses it!

@monnerat
Copy link
Contributor Author

monnerat commented Nov 3, 2017

And if you're brave: try adding a travis build that uses it!

I have already thought about it but no idea how it works and how to install brotli in travis/ubuntu.
I have to dig into it. Hints are welcome!

@bagder
Copy link
Member

bagder commented Nov 3, 2017

You can make travis download, configure and install custom software with some clever extra hacking. I did that in another project you can look at for some inspiration: https://github.com/NEAT-project/neat/blob/master/.travis.yml

@monnerat
Copy link
Contributor Author

monnerat commented Nov 3, 2017

You can make travis download, configure and ...

Thanks, will try.

vszakats added a commit to curl/curl-for-win that referenced this pull request Nov 4, 2017
@vszakats
Copy link
Member

vszakats commented Nov 4, 2017

I've made a patch that adds Brotli support to the .m32 Makefiles. I can submit it separately, but it may be even nicer if you could add it to this patch-set — if you agree, of course:

Here:
vszakats@168ed68

and here:

diff --git a/lib/Makefile.m32 b/lib/Makefile.m32
index 1389b8539..325cdc7af 100644
--- a/lib/Makefile.m32
+++ b/lib/Makefile.m32
@@ -23,7 +23,8 @@
 ###########################################################################
 #
 ## Makefile for building libcurl.a with MingW (GCC-3.2 or later or LLVM/Clang)
-## and optionally OpenSSL (1.0.2a), libssh2 (1.5), zlib (1.2.8), librtmp (2.4)
+## and optionally OpenSSL (1.0.2a), libssh2 (1.5), zlib (1.2.8), librtmp (2.4),
+## brotli (1.0.1)
 ##
 ## Usage:   mingw32-make -f Makefile.m32 CFG=-feature1[-feature2][-feature3][...]
 ## Example: mingw32-make -f Makefile.m32 CFG=-zlib-ssl-sspi-winidn
@@ -38,6 +39,10 @@
 ifndef ZLIB_PATH
 ZLIB_PATH = ../../zlib-1.2.8
 endif
+# Edit the path below to point to the base of your Brotli sources.
+ifndef BROTLI_PATH
+BROTLI_PATH = ../../brotli-1.0.1
+endif
 # Edit the path below to point to the base of your OpenSSL package.
 ifndef OPENSSL_PATH
 OPENSSL_PATH = ../../openssl-1.0.2a
@@ -175,6 +180,9 @@ endif
 ifeq ($(findstring -zlib,$(CFG)),-zlib)
 ZLIB = 1
 endif
+ifeq ($(findstring -brotli,$(CFG)),-brotli)
+BROTLI = 1
+endif
 ifeq ($(findstring -idn2,$(CFG)),-idn2)
 IDN2 = 1
 endif
@@ -280,6 +288,11 @@ ifdef ZLIB
   CFLAGS += -DHAVE_LIBZ -DHAVE_ZLIB_H
   DLL_LIBS += -L"$(ZLIB_PATH)" -lz
 endif
+ifdef BROTLI
+  INCLUDES += -I"$(BROTLI_PATH)"
+  CFLAGS += -DHAVE_BROTLI
+  DLL_LIBS += -L"$(BROTLI_PATH)/lib" -lbrotlidec
+endif
 ifdef IDN2
   INCLUDES += -I"$(LIBIDN2_PATH)/include"
   CFLAGS += -DUSE_LIBIDN2
diff --git a/src/Makefile.m32 b/src/Makefile.m32
index 728c814a4..ffc359149 100644
--- a/src/Makefile.m32
+++ b/src/Makefile.m32
@@ -23,7 +23,8 @@
 ###########################################################################
 #
 ## Makefile for building curl.exe with MingW (GCC-3.2 or later or LLVM/Clang)
-## and optionally OpenSSL (1.0.2a), libssh2 (1.5), zlib (1.2.8), librtmp (2.4)
+## and optionally OpenSSL (1.0.2a), libssh2 (1.5), zlib (1.2.8), librtmp (2.4),
+## brotli (1.0.1)
 ##
 ## Usage:   mingw32-make -f Makefile.m32 CFG=-feature1[-feature2][-feature3][...]
 ## Example: mingw32-make -f Makefile.m32 CFG=-zlib-ssl-sspi-winidn
@@ -38,6 +39,10 @@
 ifndef ZLIB_PATH
 ZLIB_PATH = ../../zlib-1.2.8
 endif
+# Edit the path below to point to the base of your Brotli sources.
+ifndef BROTLI_PATH
+BROTLI_PATH = ../../brotli-1.0.1
+endif
 # Edit the path below to point to the base of your OpenSSL package.
 ifndef OPENSSL_PATH
 OPENSSL_PATH = ../../openssl-1.0.2a
@@ -184,6 +189,9 @@ endif
 ifeq ($(findstring -zlib,$(CFG)),-zlib)
 ZLIB = 1
 endif
+ifeq ($(findstring -brotli,$(CFG)),-brotli)
+BROTLI = 1
+endif
 ifeq ($(findstring -idn2,$(CFG)),-idn2)
 IDN2 = 1
 endif
@@ -294,6 +302,11 @@ ifdef ZLIB
   CFLAGS += -DHAVE_LIBZ -DHAVE_ZLIB_H
   curl_LDADD += -L"$(ZLIB_PATH)" -lz
 endif
+ifdef BROTLI
+  INCLUDES += -I"$(BROTLI_PATH)"
+  CFLAGS += -DHAVE_BROTLI
+  curl_LDADD += -L"$(BROTLI_PATH)/lib" -lbrotlidec
+endif
 ifdef IDN2
   CFLAGS += -DUSE_LIBIDN2
   curl_LDADD += -L"$(LIBIDN2_PATH)/lib" -lidn2

@monnerat
Copy link
Contributor Author

monnerat commented Nov 4, 2017

@vszakats: thanks for the patch: I'm currently fighting with travis CI, but I will apply it immediately after it.

Once landed in master, there probably will be some updates needed for other non-autotool Makefiles too.

@vszakats
Copy link
Member

vszakats commented Nov 5, 2017

Thank you @monnerat!

@monnerat
Copy link
Contributor Author

monnerat commented Nov 5, 2017

@vszakats : you're welcome, although this is still not pushed to master. I won't forget your commit when I'll do :-)

@monnerat
Copy link
Contributor Author

monnerat commented Nov 5, 2017

Pushed in master: commits dbcced8, 11bf179, 609aa62 and c675c40

@monnerat monnerat closed this Nov 5, 2017
@monnerat monnerat deleted the content-encoding branch November 5, 2017 14:45
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants