Allow building against system Brotli #1182
Conversation
…ed brotli instead of vendored one
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Also interested. |
|
@eustas any chance we can get this merged? |
|
@eustas following up. Thanks. |
setup.py
Outdated
|
|
||
|
|
||
| def bool_from_environ(key: str): | ||
| value = os.environ.get(key) |
There was a problem hiding this comment.
you probably should use four space indentation like the rest of the file
+ Updated dependency resolution for edge cases in setup.py
|
@anthrotype Hi! Thanks for noting formatting issues. Looks like setup.py was inconsistent prior our changes so it's why my IDE decided to stick with 2 spaces. Reformatted the entire file for 4 spaces + adjusted system Brotli resolution based on some lessons learned from the initial PR. |
|
you're right, it was inconsistent before. Then perhaps better to limit the formatting changes as much as possible to prevent other PRs that touch setup.py to get into merge conflicts? E.g. #1206 |
|
@anthrotype You're right. But probably on the long distance it's better to have everything linted? Plus, spaces rarely cause major merge conflicts. IMO, we should wait for the PR you linked to be accepted. Then, I can rebase my PR on top of that + include formatting fixes. |
|
Would close #933 |
|
Hi. I'm back with brotli for a while. Could you rebase the PR, please? |
@eustas I have synced upstream. |
|
Thanks going to land soon |
|
Hi, two more things to do:
|
setup.py
Outdated
| from distutils import dep_util | ||
| from distutils import log | ||
|
|
||
| import pkgconfig |
There was a problem hiding this comment.
maybe this import pkgconfig should be moved down closer to where it is used, after if USE_SYSTEM_BROTLI:
this way it will not be required unless one explicitly request to build against system brotli
|
@eustas I moved |
|
@eustas CLA signed. |
|
@eustas can you rerun cicd? |
|
@eustas looks like the failing test is unrelated to our changes: https://github.com/google/brotli/actions/runs/15277204822/job/42974036605?pr=1182#:~:text=failed%3A%20Worker%20process%20did%20not%20return%20a%20WorkResponse%3A |
|
Yes, I know about that one. PR is mirrored to our internal repo. Tomorrow we will get second (internal) LGTM and land. Thanks! |
5 participants
This PR adds the capability to build Python bindings against system-provided Brotli instead of vendored one