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
base: master
from

Conversation

Projects
None yet
3 participants
@monnerat
Collaborator

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.

monnerat added some commits Nov 2, 2017

HTTP: support multiple Content-Encodings
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
HTTP: implement Brotli content encoding
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.
HTTP: remove assignments in for loop tests
Also complete initialisation of version info data structure.
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 3, 2017

Member

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!

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

This comment has been minimized.

Show comment
Hide comment
@monnerat

monnerat Nov 3, 2017

Collaborator

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!

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 3, 2017

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@monnerat

monnerat Nov 3, 2017

Collaborator

You can make travis download, configure and ...

Thanks, will try.

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Nov 4, 2017

Member

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
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

This comment has been minimized.

Show comment
Hide comment
@monnerat

monnerat Nov 4, 2017

Collaborator

@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.

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@vszakats

vszakats Nov 5, 2017

Member

Thank you @monnerat!

Member

vszakats commented Nov 5, 2017

Thank you @monnerat!

@monnerat

This comment has been minimized.

Show comment
Hide comment
@monnerat

monnerat Nov 5, 2017

Collaborator

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

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@monnerat

monnerat Nov 5, 2017

Collaborator

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

Collaborator

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 monnerat:content-encoding branch Nov 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment