Support using system llhttp library (#10759) #10760
Conversation
Add a new `USE_SYSTEM_DEPS` environment variable that can be used to enable building against the system install of llhttp library rather than the vendored one. This is implemented using the `pkgconfig` package to query llhttp install via pkg-config. The package is only used when `USE_SYSTEM_DEPS` is enabled, however due to limitations of `pyproject.toml` it is listed unconditionally as a build dependency. That said, flake8 claims that the dependency is not listed and I don't really know how to solve that: ``` setup.py:36:5: I900 'pkgconfig' not listed as a requirement ``` I have also updated `aiohttp/_cparser.pxd` to reference the `llhttp.h` header via its name rather than full path. I have poisoned the system header to confirm that the correct header is used both when building against the system library and the vendored library.
| @@ -17,7 +18,11 @@ | |||
| NO_EXTENSIONS = True | |||
| if IS_GIT_REPO and not (HERE / "vendor/llhttp/README.md").exists(): | |||
| if ( | |||
| not USE_SYSTEM_DEPS | |||
There was a problem hiding this comment.
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.
| @@ -26,19 +31,37 @@ | |||
| # NOTE: makefile cythonizes all Cython modules | |||
| if USE_SYSTEM_DEPS: | |||
There was a problem hiding this comment.
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.
CodSpeed Performance ReportMerging #10760 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #10760 +/- ##
=======================================
Coverage 98.72% 98.72%
=======================================
Files 125 125
Lines 37793 37858 +65
Branches 2085 2091 +6
=======================================
+ Hits 37310 37375 +65
Misses 335 335
Partials 148 148
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -1,6 +1,7 @@ | |||
| [build-system] | |||
| requires = [ | |||
| "setuptools >= 46.4.0", | |||
| "pkgconfig", | |||
There was a problem hiding this comment.
Can we keep this list sorted? Is there a minimum version we'd need?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
setup.py Outdated
| @@ -8,6 +8,7 @@ | |||
| raise RuntimeError("aiohttp 4.x requires Python 3.9+") | |||
| USE_SYSTEM_DEPS: bool = bool(os.environ.get("USE_SYSTEM_DEPS")) | |||
There was a problem hiding this comment.
This should be specific to aiohttp since we don't want generic vars to influence our builds unexpectedly.
There was a problem hiding this comment.
Well, I can do that. However, what we're trying to do (in general, see the linked doc) is to standardize on a "common" environment variable that distributions can use to disable all vendored libraries altogether — i.e. following the principle "it's better for the build to fail than for us to miss a vendored library". However, if these are eventually going to be replaced by config_settings, I suppose it's a lost cause anyway.
There was a problem hiding this comment.
In that case, I'd suggest having two env vars — one scoped and one global.
I might be able to preserve this during refactoring. I don't even have a timeline so let's not worry about it now.
On a side note, I've been meaning to explore better ways of configuring linking in similarly packaged projects. Namely, ansible-pylibssh. There, I have expandvars integrated and a way to have externally set compiler vars like CFLAGS. And so the build automation is able to set those independently.
I feel like it might be a good approach and would be more generic. That project doesn't graft libssh into the source tree, though.
There was a problem hiding this comment.
Done (i.e. added fallback to USE_SYSTEM_DEPS when AIOHTTP_USE_SYSTEM_DEPS is not set).
There was a problem hiding this comment.
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
What do these changes do?
Add a new
USE_SYSTEM_DEPSenvironment variable that can be used to enable building against the system install of llhttp library rather than the vendored one.This is implemented using the
pkgconfigpackage to query llhttp install via pkg-config. The package is only used whenUSE_SYSTEM_DEPSis enabled, however due to limitations ofpyproject.tomlit is listed unconditionally as a build dependency. That said, flake8 claims that the dependency is not listed and I don't really know how to solve that:I have also updated
aiohttp/_cparser.pxdto reference thellhttp.hheader via its name rather than full path. I have poisoned the system header to confirm that the correct header is used both when building against the system library and the vendored library.Are there changes in behavior for the user?
pkgconfigbuild requirementUSE_SYSTEM_DEPSwill be using the system llhttp libraryIs it a substantial burden for the maintainers to support this?
I don't think so. The code uses
pkg-config, so it should be pretty forward-secure to potentially nonstandardllhttpinstalls. On top of that, presumably the feature will be primarily used by downstreams / expert users, who would rather be willing to submit fixes, should any issues arise (though that implies the burden of reviewing them).Related issue number
Discussion #10759.
Checklist
CONTRIBUTORS.txtCHANGES/foldername it
<issue_or_pr_num>.<type>.rst(e.g.588.bugfix.rst)if you don't have an issue number, change it to the pull request
number after creating the PR
.bugfix: A bug fix for something the maintainers deemed animproper undesired behavior that got corrected to match
pre-agreed expectations.
.feature: A new behavior, public APIs. That sort of stuff..deprecation: A declaration of future API removals and breakingchanges in behavior.
.breaking: When something public is removed in a breaking way.Could be deprecated in an earlier release.
.doc: Notable updates to the documentation structure or buildprocess.
.packaging: Notes for downstreams about unobvious side effectsand tooling. Changes in the test invocation considerations and
runtime assumptions.
.contrib: Stuff that affects the contributor experience. e.g.Running tests, building the docs, setting up the development
environment.
.misc: Changes that are hard to assign to any of the abovecategories.
Make sure to use full sentences with correct case and punctuation,
for example:
Use the past tense or the present tense a non-imperative mood,
referring to what's changed compared to the last released version
of this project.