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

Makefile: change compilation and linking of libbrotli to optional (and off by default) #1379

Merged
merged 4 commits into from Dec 5, 2022

Conversation

LeGEC
Copy link
Contributor

@LeGEC LeGEC commented Nov 13, 2022

Database name

all databases -- affects libbrotli at build time

Pull request description

Describe what this PR fix

fixes #1378

add a NO_LIBBROTLI option to opt out of libbrotli at build time.

This PR has been modified to :
add a USE_BROTLI option in the Makefile to build and link libbrotli

this is a (arguably small) breaking change :
if you previously ran regular make commands :

make
make link_brotli
make pg_build
# etc ...

and still need to support the brotli compression format (for retrocompatibility), you now need to set USE_BROTLI=1 at build time :

export USE_BROTLI=1
make
make link_brotli
make pg_build
# etc ...

or somehow set it in your build pipeline

@LeGEC LeGEC requested a review from a team as a code owner November 13, 2022 09:32
@LeGEC LeGEC force-pushed the 1378-makefile-opt-out-libbrotli branch from f4ea154 to 9203b79 Compare November 13, 2022 09:37
Makefile Outdated Show resolved Hide resolved
@LeGEC LeGEC force-pushed the 1378-makefile-opt-out-libbrotli branch from 9203b79 to e6bcc48 Compare November 16, 2022 15:02
@LeGEC LeGEC changed the title Makefile: add an option to opt out of libbrotli Makefile: add compilation and linking of libbrotli under USE_LIBBROTLI=1 option Nov 16, 2022
@LeGEC LeGEC changed the title Makefile: add compilation and linking of libbrotli under USE_LIBBROTLI=1 option Makefile: change compilation and linking of libbrotli to optional (and off by default) Nov 16, 2022
note: this is a breaking change on the Makefile :
if you need to support brotli in your builds, you now need to add
    USE_BROTLI=1
in your build environment.

e.g: change:
    make link_brotli
    make pg_build

to the equivalent of:
    export USE_BROTLI=1
    make link_brotli
    make pg_build
@LeGEC LeGEC force-pushed the 1378-makefile-opt-out-libbrotli branch from e6bcc48 to 06a838b Compare November 16, 2022 16:57
@LeGEC
Copy link
Contributor Author

LeGEC commented Nov 16, 2022

I tried to update the workflow files (by mimicking places where USE_LZO or USE_LIBSODIUM was set), feel free to correct that part.

Also : I tried updating docs/README.md , do review if the places where USE_BROTLI=1 is mentioned are relevant.

@LeGEC
Copy link
Contributor Author

LeGEC commented Nov 16, 2022

Well, apparently, I missed the mongodb tests :

https://github.com/wal-g/wal-g/actions/runs/3481421063/jobs/5823020774#step:8:4982

@LeGEC
Copy link
Contributor Author

LeGEC commented Dec 5, 2022

I pushed a commit where I changed the tests for redis and mongodb : the tests are run using zstd instead of brotli.

I would much rather not change those tests in this specific PR and confirm that the main change (e.g: set USE_BROTLI if you want to use brotli) still allow to run the tests that depend on brotli unmodified, but I fail to understand where I missed passing the USE_BROTLI=1 environment variable.

Can someone have a second look at the modifications I committed, and point me to what step requires adding USE_BROTLI=1 ?

@serprex
Copy link
Contributor

serprex commented Dec 5, 2022

It's in the docker compose files you'd want to look at

See #634

search for Dockerfiles that run 'make [some_walg_build_rule]' at
docker build time, and set "ENV USE_BROTLI 1" in those Dockerfiles
@LeGEC LeGEC force-pushed the 1378-makefile-opt-out-libbrotli branch from d878d5d to aa4a6a0 Compare December 5, 2022 19:07
@serprex serprex self-requested a review December 5, 2022 19:27
@LeGEC
Copy link
Contributor Author

LeGEC commented Dec 5, 2022

setting the env value in the relevant Docker files seems to do the job.

@serprex serprex dismissed usernamedt’s stale review December 5, 2022 20:31

feedback to name config USE_BROTLI was implemented

@serprex serprex merged commit e6efdcf into wal-g:master Dec 5, 2022
@serprex serprex mentioned this pull request Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

option to disable libbrotli in Makefile
3 participants