Skip to content

Support using system llhttp library (#10759) #10760

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
+51 −7
Open

Support using system llhttp library (#10759) #10760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cb9ac7d
Support using system llhttp library (#10759)
mgorny Apr 20, 2025
96f1cb0
Add "llhttp" to spelling wordlist
mgorny Apr 21, 2025
0b7c184
Implement review comments
mgorny Apr 21, 2025
9b16cdc
Add pkgconfig to test requirements to satisfy flake8
mgorny Apr 21, 2025
9eff9e8
Add a fallback to "generic" USE_SYSTEM_DEPS envvar
mgorny Apr 21, 2025
006048f
Apply suggestions from code review
mgorny Apr 22, 2025
c9d281c
Document build-time environment variables
mgorny Apr 22, 2025
7ce83ea
Fix a typo
mgorny Apr 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGES/10759.packaging.rst
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Preview: https://aiohttp--10760.org.readthedocs.build/en/10760/changes.html#packaging-updates-and-notes-for-downstreams

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Added support for building against system ``llhttp`` library -- by :user:`mgorny`.

This change adds support for :envvar:`AIOHTTP_USE_SYSTEM_DEPS` environment variable that
can be used to build aiohttp against the system install of the ``llhttp`` library rather
than the vendored one.
2 changes: 1 addition & 1 deletion aiohttp/_cparser.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from libc.stdint cimport int32_t, uint8_t, uint16_t, uint64_t


cdef extern from "../vendor/llhttp/build/llhttp.h":
cdef extern from "llhttp.h":

struct llhttp__internal_s:
int32_t _index
Expand Down
11 changes: 11 additions & 0 deletions docs/glossary.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,17 @@
Environment Variables
=====================

.. envvar:: AIOHTTP_NO_EXTENSIONS

If set to a non-empty value while building from source, aiohttp will be built without speedups
written as C extensions. This option is primarily useful for debugging.

.. envvar:: AIOHTTP_USE_SYSTEM_DEPS

If set to a non-empty value while building from source, aiohttp will be built against
the system installation of llhttp rather than the vendored library. This option is primarily
meant to be used by downstream redistributors.

.. envvar:: NETRC

If set, HTTP Basic Auth will be read from the file pointed to by this environment variable,
Expand Down
1 change: 1 addition & 0 deletions docs/spelling_wordlist.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ kwargs
latin
lifecycle
linux
llhttp
localhost
Locator
login
Expand Down
1 change: 1 addition & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
[build-system]
requires = [
"pkgconfig",
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep this list sorted? Is there a minimum version we'd need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. My idea was to keep setuptools first since it's the backend (i.e. the primary dependency), but I can change order, no problem.

I don't think we do need a minimal version. The two functions we're using are available since the first release, 0.1.0. I've just tried forcing == 0.1.0 and the build succeeded.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense. I often make this separation by having an empty line and adding code comments marking essential and plugin sections of the list. But since it wasn't here, I forgot about that habit :)

"setuptools >= 46.4.0",
]
build-backend = "setuptools.build_meta"
Expand Down
1 change: 1 addition & 0 deletions requirements/test.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ coverage
freezegun
isal
mypy; implementation_name == "cpython"
pkgconfig
proxy.py >= 2.4.4rc5
pytest
pytest-cov
Expand Down
37 changes: 31 additions & 6 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
raise RuntimeError("aiohttp 4.x requires Python 3.9+")


USE_SYSTEM_DEPS = bool(
os.environ.get("AIOHTTP_USE_SYSTEM_DEPS", os.environ.get("USE_SYSTEM_DEPS"))
)
NO_EXTENSIONS: bool = bool(os.environ.get("AIOHTTP_NO_EXTENSIONS"))
HERE = pathlib.Path(__file__).parent
IS_GIT_REPO = (HERE / ".git").exists()
Expand All @@ -17,7 +20,11 @@
NO_EXTENSIONS = True


if IS_GIT_REPO and not (HERE / "vendor/llhttp/README.md").exists():
if (
not USE_SYSTEM_DEPS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, I think this block does not need to be executed with NO_EXTENSIONS but I didn't change the existing behavior. Please let me know if you prefer that I changed that.

and IS_GIT_REPO
and not (HERE / "vendor/llhttp/README.md").exists()
):
print("Install submodules when building from git clone", file=sys.stderr)
print("Hint:", file=sys.stderr)
print(" git submodule update --init", file=sys.stderr)
Expand All @@ -26,19 +33,37 @@

# NOTE: makefile cythonizes all Cython modules

if USE_SYSTEM_DEPS:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, I could also add and not NO_EXTENSIONS here, to avoid calling pkg-config when we're not building extensions (though technically speaking, the other branch is also unnecessary then). That said, I'm not sure if we really need to special-case the case of setting USE_SYSTEM_DEPS when no extensions are built.

import shlex

import pkgconfig

llhttp_sources = []
llhttp_kwargs = {
"extra_compile_args": shlex.split(pkgconfig.cflags("libllhttp")),
"extra_link_args": shlex.split(pkgconfig.libs("libllhttp")),
}
else:
llhttp_sources = [
"vendor/llhttp/build/c/llhttp.c",
"vendor/llhttp/src/native/api.c",
"vendor/llhttp/src/native/http.c",
]
llhttp_kwargs = {
"define_macros": [("LLHTTP_STRICT_MODE", 0)],
"include_dirs": ["vendor/llhttp/build"],
}

extensions = [
Extension("aiohttp._websocket.mask", ["aiohttp/_websocket/mask.c"]),
Extension(
"aiohttp._http_parser",
[
"aiohttp/_http_parser.c",
"aiohttp/_find_header.c",
"vendor/llhttp/build/c/llhttp.c",
"vendor/llhttp/src/native/api.c",
"vendor/llhttp/src/native/http.c",
*llhttp_sources,
],
define_macros=[("LLHTTP_STRICT_MODE", 0)],
include_dirs=["vendor/llhttp/build"],
**llhttp_kwargs,
),
Extension("aiohttp._http_writer", ["aiohttp/_http_writer.c"]),
Extension("aiohttp._websocket.reader_c", ["aiohttp/_websocket/reader_c.c"]),
Expand Down
Loading