Skip to content

Use either brotli or brotlicffi.#1618

Merged
lovelydinosaur merged 5 commits into
masterfrom
use-either-brotli-or-brotlicffi
Aug 13, 2021
Merged

Use either brotli or brotlicffi.#1618
lovelydinosaur merged 5 commits into
masterfrom
use-either-brotli-or-brotlicffi

Conversation

@lovelydinosaur

Copy link
Copy Markdown
Contributor

See #1605 (comment)

  • The brotli package is recommended for CPython.
  • The brotlicffi package is recommended for PyPy.

Note the following:

  • We have to use Decompressor.decompress if brotlicffi is installed, but Decompressor.process if brotli is installed.
  • We can use Decompressor.finish() if brotlicffi is installed.
  • We use the oddly-cased brotli.error instead of brotli.Error because it is supported as a synonym in both packages.

Also worth calling out the the older brotlipy package might be installed on the system. In which case it will shadow the brotli package namespace. Ug.

Comment thread httpx/_decoders.py
Comment on lines 107 to 112
if hasattr(self.decompressor, "decompress"):
self._decompress = self.decompressor.decompress
# The 'brotlicffi' package.
self._decompress = self.decompressor.decompress # pragma: nocover
else:
# The 'brotli' package.
self._decompress = self.decompressor.process # pragma: nocover

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I thinkbrotlicffi also has a process alias for decompress:

https://github.com/python-hyper/brotlicffi/blob/68c0431673e6c59549079e6588b043a707262c80/src/brotlicffi/_api.py#L423

How about just use self.decompressor.process?

@lovelydinosaur lovelydinosaur Apr 30, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True. Tho I double checked and the brotlipy package doesn't, so what we want to do here depends on if we want to take care around the possibility that on some systems the older brotlipy package might have been installed. (Or not.)

  • If we do care about that awkward case, then we should just update the comment strings here.
  • If we don't case about that case, then we should just always use Decompressor().process().

Perhaps we could check which of brotli vs brotlipy is installed the _compat.py module, and raise an error if brotlipy is installed and is shadowing the brotli namespace.

For example:

    if hasattr(brotli.Decompressor, 'decompress'):
        raise RuntimeError(
            "The brotlipy library appears to be installed, and is shadowing the 'brotli' namespace. "
            "Uninstall brotlipy and use brotlicffi instead."
        )

@lovelydinosaur lovelydinosaur merged commit d514312 into master Aug 13, 2021
@lovelydinosaur lovelydinosaur deleted the use-either-brotli-or-brotlicffi branch August 13, 2021 10:52
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.

3 participants