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

proposal: use meson-python as build backend #1452

Closed
wants to merge 8 commits into from

Conversation

trim21
Copy link

@trim21 trim21 commented Dec 7, 2024

What do these changes do?

This is proposal and POC for using meson-python as build system.

Currently we need to maintain a build backend to support our use-case for pure-python/binary module, dynamic cython flags, which is supported by meson.

uv

uv related change are just for speedup, which can be removed.

coverage hit line changes

coverage dropped ~200 lines from removed packaging directory.

benchmark

benchmark of this PR is misleading becuase it use different compile flags, it should not be compared with master branch.

Cython upgrade:

cython check Py_UCS4 value range in trace mode, <Py_UCS4>-1 now will throw a ValueError, so a extra return value is need to indicate the _restore_ch return.

The upgrade of cython can be avoided by patch generated c file: 0c4e11c#diff-feabd8e812a476b3324d4cc9144ef3e2ff439b8afda577322f4ba99f6185d63b

Are there changes in behavior for the user?

No, only affect developers.

Related issue number

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@trim21 trim21 changed the title use meson proposal: use meson-python as build backend Dec 7, 2024
Copy link

codspeed-hq bot commented Dec 7, 2024

CodSpeed Performance Report

Merging #1452 will degrade performances by 7.55%

Comparing trim21:use-meson (633d842) with master (f304dd7)

Summary

⚡ 12 improvements
❌ 1 regressions
✅ 86 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark master trim21:use-meson Change
test_long_query 2,165.4 µs 482.1 µs ×4.5
test_long_query_with_pct 9.5 ms 3.4 ms ×2.8
test_quote_long_path 1,785.6 µs 403.8 µs ×4.4
test_quote_query_string 142.7 µs 71.4 µs +99.98%
test_quoter_ascii 69.7 µs 51.9 µs +34.22%
test_quoter_pct 140.6 µs 84.1 µs +67.17%
test_quoter_quote_utf8 389.3 µs 143.5 µs ×2.7
test_unquoter_long_ascii 4.5 ms 4.8 ms -7.55%
test_update_query_sequence_mapping 3.2 ms 2.9 ms +9.19%
test_url_build_no_netloc 353.1 µs 327.8 µs +7.72%
test_url_build_no_netloc_relative 344.1 µs 321.5 µs +7.02%
test_url_with_fragment 178.9 µs 161.8 µs +10.55%
test_with_query_sequence_mapping 2.9 ms 2.6 ms +11.24%

Copy link

codecov bot commented Dec 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.70%. Comparing base (f304dd7) to head (633d842).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1452      +/-   ##
==========================================
+ Coverage   96.14%   96.70%   +0.55%     
==========================================
  Files          31       26       -5     
  Lines        5970     5668     -302     
  Branches      364      348      -16     
==========================================
- Hits         5740     5481     -259     
+ Misses        204      177      -27     
+ Partials       26       10      -16     
Flag Coverage Δ
CI-GHA 96.70% <100.00%> (+0.55%) ⬆️
MyPy 48.01% <100.00%> (-1.70%) ⬇️
OS-Linux 98.83% <100.00%> (-0.74%) ⬇️
OS-Windows 98.81% <100.00%> (-0.82%) ⬇️
OS-macOS 98.81% <100.00%> (-0.51%) ⬇️
Py-3.10.11 98.79% <100.00%> (-0.51%) ⬇️
Py-3.10.15 98.79% <100.00%> (-0.74%) ⬇️
Py-3.11.10 98.79% <100.00%> (-0.74%) ⬇️
Py-3.11.9 98.79% <100.00%> (-0.51%) ⬇️
Py-3.12.7 98.79% <100.00%> (-0.74%) ⬇️
Py-3.13.0 98.79% <100.00%> (-0.51%) ⬇️
Py-3.13.1 98.79% <100.00%> (-0.74%) ⬇️
Py-3.9.13 98.74% <100.00%> (-0.51%) ⬇️
Py-3.9.20 98.74% <100.00%> (-0.74%) ⬇️
Py-pypy7.3.16 98.76% <ø> (-0.78%) ⬇️
Py-pypy7.3.17 98.78% <ø> (-0.78%) ⬇️
VM-macos-latest 98.81% <100.00%> (-0.51%) ⬇️
VM-ubuntu-latest 98.83% <100.00%> (-0.74%) ⬇️
VM-windows-latest 98.81% <100.00%> (-0.82%) ⬇️
pytest 98.83% <100.00%> (-0.74%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@trim21 trim21 force-pushed the use-meson branch 3 times, most recently from 19950c5 to 58546e8 Compare December 7, 2024 17:01
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Dec 7, 2024
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Thanks for the proposal but I'm not seeking to migrate away from setuptools anytime soon. This adds unnecessary maintenance burden on many levels, including replicating the configuration across many similarly packaged project within and outside aio-libs, keeping all of them in sync, learning several new tools and supporting this long-term (I don't suppose you're offering to support packaging of all aio-libs projects for years to come instead of me, are you?)

Other things I see being problematic:

  • another interference change for the downstream packagers
  • forcing contributors to install even more tooling
  • a bunch of formatting changes all over
  • some test matrix changes
  • requiring cython to build sdists
  • blocking the upgrade path to eventually using setuptools-scm
  • losing the reproducibility infrastructure
  • unrelated hacks making their way into a packaging PR
  • the patch being big to the point of discouraging the reviewers from scheduling looking into it
  • no prior discussion / agreement of the need or any acceptance really

So I'm strongly 👎 on this.

@trim21
Copy link
Author

trim21 commented Dec 8, 2024

Thanks for your response.

I don't suppose you're offering to support packaging of all aio-libs projects for years to come instead of me, are you?

You have convinced me here 😆

I wouldn't call it mergeable eigher, it present as a proposal and POC for another option without maintaining a build backend for further discussion.

It's indeed a dead end that does not lead to setuptools-scm and add maintenance complexity.

I'll close it.

Some misunderstanding:

It doesn't not require cpython to build sdist (ninja extra is optional and can be removed), it require cpython only when you compile binary wheel from sdist. on pypy it's will only build a pure python wheel.

ths hack on yarl/_quoting_c.pyx is caused by upgrading the cython to 3.1.0a1 to support pyx coverage, and will be fixed in 3.1, which is also not necessary. the formatter change in yarl/_quoting_c.pyi is also not expected not generated by ruff-format, both I'd very much to avoid but necessary to make ci pass at this time.

@trim21 trim21 closed this Dec 8, 2024
@webknjaz
Copy link
Member

webknjaz commented Dec 8, 2024

It doesn't not require cpython to build sdist (ninja extra is optional and can be removed), it require cpython only when you compile binary wheel from sdist.

I made a typo and meant Cython. What I meant is that the deps listed in [build-system].requires are unconditional and Cython would be unnecessarily pulled in for building the sdist (even if unused).

Among other things that are lost is the reference conversion in readme+changelog.

By the way, there's no Ruff in this project, so those changes are coming from your editor being misconfigured.

P.S. There's an effort in Cython to make it easier to use in a declarative way but it doesn't cover all the things I have currently implemented: cython/cython#6305. I hope that one day https://ofek.dev/extensionlib/ will take off and evolve into some sort of standard, which would then improve making C-extensions across different tools in the ecosystem.

@trim21
Copy link
Author

trim21 commented Dec 8, 2024

OH, sorry. It's not ruff-format but black. It will infer minimal target python version from pyproject's requires-python and cause this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants