Skip to content

Commit

Permalink
Fix broken brotli build (upstream brotli dropped makefile support and…
Browse files Browse the repository at this point in the history
… sources.lst file) (#150)

* Fix config file to no longer depend on a sources.lst file.

* Fix readme to show how to build with latest brotli.

Also make the build files real-life with optimizations (so everyone doesn't have to search for the best way to compile things).

* Fix typo

* Cleanup config file to not depend on out-of-date libs

* Remove unused variables.
  • Loading branch information
wyattoday authored Sep 7, 2023
1 parent 8f726ea commit 63ca02a
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 34 deletions.
30 changes: 23 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ ngx_brotli is a set of two nginx modules:
- ngx_brotli filter module - used to compress responses on-the-fly,
- ngx_brotli static module - used to serve pre-compressed files.

[![TravisCI Build Status](https://travis-ci.org/google/ngx_brotli.svg?branch=master)](https://travis-ci.org/google/ngx_brotli)

## Table of Contents

Expand All @@ -37,6 +36,29 @@ Both Brotli library and nginx module are under active development.

## Installation

### Statically compiled

Checkout the latest `ngx_brotli` and build the dependencies:

```
git clone --recurse-submodules -j8 https://github.com/google/ngx_brotli
cd ngx_brotli/deps/brotli
mkdir out && cd out
cmake -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=OFF -DCMAKE_C_FLAGS="-Ofast -m64 -march=native -mtune=native -flto -funroll-loops -ffunction-sections -fdata-sections -Wl,--gc-sections" -DCMAKE_CXX_FLAGS="-Ofast -m64 -march=native -mtune=native -flto -funroll-loops -ffunction-sections -fdata-sections -Wl,--gc-sections" -DCMAKE_INSTALL_PREFIX=./installed ..

This comment has been minimized.

Copy link
@xvybihal

xvybihal Sep 12, 2023

Hi, I am wondering why exactly these options are chosen for this README. I had a bad time getting the module to work yesterday. When I switched my builds to cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=./installed .. all is fine. It seems oddly specific, that's why I am wondering. Thank you for fixing the build.

This comment has been minimized.

Copy link
@wyattoday

wyattoday Sep 12, 2023

Author Contributor

See upstream Brotli project.

This comment has been minimized.

Copy link
@xvybihal

xvybihal Sep 12, 2023

I did, their README does not suggest those flags.

This comment has been minimized.

Copy link
@wyattoday

wyattoday Sep 12, 2023

Author Contributor

The release built type and the install location? Yes, it does.

If you're talking about the other flags, google them. They're about building fast, small builds. The Readme should show the best way to build things. It shouldn't force users to figure this out themselves (because the won't).

Yes, you'll need to modify them on less-used CPUs, but you can google how to do it.

This comment has been minimized.

Copy link
@xvybihal

xvybihal Sep 12, 2023

Thank you for the clarification 👍

cmake --build . --config Release --target brotlienc
cd ../../../..
```


$ cd nginx-1.x.x
$ export CFLAGS="-m64 -march=native -mtune=native -Ofast -flto -funroll-loops -ffunction-sections -fdata-sections -Wl,--gc-sections"
$ export LDFLAGS="-m64 -Wl,-s -Wl,-Bsymbolic -Wl,--gc-sections"
$ ./configure --add-module=/path/to/ngx_brotli
$ make && make install

This will compile the module directly into Nginx.


### Dynamically loaded

$ cd nginx-1.x.x
Expand All @@ -51,13 +73,7 @@ load_module modules/ngx_http_brotli_filter_module.so;
load_module modules/ngx_http_brotli_static_module.so;
```

### Statically compiled

$ cd nginx-1.x.x
$ ./configure --add-module=/path/to/ngx_brotli
$ make && make install

This will compile the module directly into Nginx.

## Configuration directives

Expand Down
31 changes: 4 additions & 27 deletions filter/config
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,6 @@ ngx_module_type=HTTP_FILTER
ngx_module_name=ngx_http_brotli_filter_module

brotli="$ngx_addon_dir/deps/brotli/c"
if [ ! -f "$brotli/include/brotli/encode.h" ]; then
brotli="/usr/local"
fi
if [ ! -f "$brotli/include/brotli/encode.h" ]; then
brotli="/usr"
fi
if [ ! -f "$brotli/include/brotli/encode.h" ]; then
cat << END

Expand All @@ -63,33 +57,16 @@ END
exit 1
fi

BROTLI_LISTS_FILE="$brotli/../scripts/sources.lst"

if [ -f "$BROTLI_LISTS_FILE" ]; then

BROTLI_LISTS=`cat "$BROTLI_LISTS_FILE" | grep -v "#" | tr '\n' '#' | \
sed 's/\\\\#//g' | tr -s ' ' '+' | tr -s '#' ' ' | \
sed 's/+c/+$brotli/g' | sed 's/+=+/=/g'`
for ITEM in ${BROTLI_LISTS}; do
VAR=`echo $ITEM | sed 's/=.*//'`
VAL=`echo $ITEM | sed 's/.*=//' | tr '+' ' '`
eval ${VAR}=\"$VAL\"
done

else # BROTLI_LISTS_FILE

BROTLI_OUTPUT_DIRECTORY="$brotli/../out"
BROTLI_ENC_H="$brotli/include/brotli/encode.h \
$brotli/include/brotli/port.h \
$brotli/include/brotli/types.h"
BROTLI_ENC_LIB="-lbrotlienc"

fi

ngx_module_incs="$brotli/include"
ngx_module_deps="$BROTLI_COMMON_H $BROTLI_ENC_H"
ngx_module_srcs="$BROTLI_COMMON_C $BROTLI_ENC_C \
$BROTLI_MODULE_SRC_DIR/ngx_http_brotli_filter_module.c"
ngx_module_libs="$BROTLI_ENC_LIB -lm"
ngx_module_deps="$BROTLI_ENC_H"
ngx_module_srcs="$BROTLI_MODULE_SRC_DIR/ngx_http_brotli_filter_module.c"
ngx_module_libs="-L$BROTLI_OUTPUT_DIRECTORY -lbrotlienc -lbrotlicommon -lm"
ngx_module_order="$ngx_module_name \
ngx_pagespeed \
ngx_http_postpone_filter_module \
Expand Down

5 comments on commit 63ca02a

@gnsaddy
Copy link

@gnsaddy gnsaddy commented on 63ca02a Sep 28, 2023

Choose a reason for hiding this comment

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

This commit broke brotli build in nginx ingress controller

/usr/bin/ld: cannot find -lbrotlienc
/usr/bin/ld: cannot find -lbrotlicommon

collect2: error: ld returned 1 exit status

@wyattoday
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Read the README file. This is a fix to a previous commit, but you need to update any automated scripts to build things correctly.

@gnsaddy
Copy link

Choose a reason for hiding this comment

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

Read the README file. This is a fix to a previous commit, but you need to update any automated scripts to build things correctly.

I'm following what mentioned in https://github.com/kubernetes/ingress-nginx/blob/e2ee3346db6ce667c4b9e79c6944ba405c741db1/images/nginx/rootfs/build.sh#L467C1-L472C21

cd "$BUILD_PATH"
git clone --depth=1 https://github.com/google/ngx_brotli.git
cd ngx_brotli
git submodule init
git submodule update

@wyattoday
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s a them-bug. Like I said, read the README. Or report it to them (kibernetes) and they can read the README and fix their scripts.

@strongjz
Copy link

@strongjz strongjz commented on 63ca02a Oct 1, 2023

Choose a reason for hiding this comment

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

thanks for investigating this @gnsaddy , we will get our scripts updated

Please sign in to comment.