Claude Fable review of meson-python

[henryiii]

Trying to make use of the one week 50% access to Fable! :) Here's a review of meson-python, following what I've done in other projects I maintain: https://iscinumpy.dev/post/claude-code-reviews/, such as scikit-build-core: scikit-build/scikit-build-core#1317 - I've been really happy with the results, the FP rate is nearly zero. Happy to generate fixes for things if other maintainers want me to!

(Also, congrats, no high-severity bugs. :) )

🤖 AI text below 🤖

Review complete — I read all six source modules in full and verified the suspicious findings against git history. Here's what I found, ordered by how much I'd prioritize each.

Bugs

1. _MESON_REQUIRED_VERSION is out of sync with pyproject.toml (mesonpy/__init__.py:95). The constant says 0.63.3 (unchanged since the runtime check was added in cc2e326), but pyproject.toml requires meson >= 0.64.0, and >= 1.2.3 on Python 3.12+. The comment on that line — and CLAUDE.md — say to keep them in sync. Beyond bumping it, the runtime check should probably be conditional on the Python version like the requirement is, otherwise meson 1.0 passes the check on Python 3.12 where it's known broken. Relatedly, on meson < 1.3.0 the python.allow_limited_api introspection at __init__.py:896-899 comes back None and silently disables the user's limited-api = true — worth confirming that's intended.

2. No-op str.replace in the editable path hook (mesonpy/_editable.py:352). path.replace(os.altsep, os.sep) discards its result; it needs path = path.replace(...). On POSIX altsep is None so nothing is affected, but on Windows the intended separator normalization never happens before path.rpartition(os.sep).

3. Missing check=True on RPath reads (mesonpy/_rpath.py:27 and :75). The macOS otool -l and ELF patchelf --print-rpath invocations don't check the return code. If the tool fails (missing patchelf, unreadable file), _get_rpath returns nothing, fix_rpath becomes a silent no-op, and the wheel ships with an RPATH still pointing at the build directory — a broken wheel with no error. The Solaris variant already passes check=True; the other two should match.

4. direct_url.json isn't valid JSON (mesonpy/__init__.py:548-550). The editable wheel writes source_dir.as_uri() — a bare unquoted file:///... string — where PEP 610 specifies a JSON object like {"url": "...", "dir_info": {"editable": true}}. It's latent because pip overwrites the file at install time (nothing in the test suite exercises it), but any other consumer parsing it will choke. Either write the proper object or drop the file.

5. Regex typo in the wheel filename pattern (mesonpy/_wheelfile.py:26). (:?-(?P<build>[^-]+))? was clearly meant to be (?:...) — as written it's a capturing group that permits a stray literal colon. Harmless today since meson-python never generates build tags, but it's wrong as a validator.

6. StopIteration can mask the split-package error (mesonpy/__init__.py:160). In the purelib/platlib conflict branch, next(entry for entry in wheel_files[other] ...) has no default. If the package was first registered by an install_subdirs entry whose directory walked to zero files, packages has the entry but wheel_files[other] has none, and the user gets a raw StopIteration/RuntimeError instead of the helpful BuildError. Use next(..., None) and degrade the message.

7. Symlink cycles hang sdist() (mesonpy/__init__.py:1063). The while member.issym(): resolution loop never terminates if the archive contains a symlink cycle (a -> b, b -> a). Malformed input, but a bounded loop or visited-set would turn a hang into a warning like the other symlink cases.

8. Outdated 32-bit-on-aarch64 tag (mesonpy/_tags.py:192). A 32-bit interpreter on an aarch64 kernel is tagged linux_armv7l; packaging ≥ 24.0 switched this case to linux_armv8l (Ability to install armv7l manylinux/musllinux wheels on armv8l pypa/packaging#690). Wheels tagged the old way are increasingly inconsistent with what pip/packaging compute on the install side.

Performance

• WheelFileWriter.write (mesonpy/_wheelfile.py:96-98) slurps each file fully into memory to hash and write it. For scientific wheels with very large shared objects that's a real memory spike; streaming in chunks through zipfile.open(zinfo, 'w') while updating the sha256 incrementally fixes it without changing behavior.
• _editable.walk() calls dirnames.sort() inside the per-entry loop (mesonpy/_editable.py:229), re-sorting once per subdirectory; the twin logic in __init__.py:177 correctly sorts once after the pruning loop. Trivial cost, but the two copies of this function have quietly diverged, which is the more interesting problem given _editable.py must stay standalone.

Simplifications

• mesonpy/__init__.py:919: the one .format(style=style, cmd=...) call in a file that otherwise uses f-strings.
• _env_ninja_command (__init__.py:1236) reuses the name version for both the required version parameter and the subprocess output — correct but needlessly confusing.
• _string_or_strings (__init__.py:643): list([value,] if isinstance(value, str) else value) → [value] if isinstance(value, str) else list(value).
• The sdist loop copies member twice when a symlink was resolved (__init__.py:1073 and :1085); harmless, but the second copy's comment ("before starting to modify it") no longer tells the whole story.

Modernizations (floor is Python 3.9)

• @functools.lru_cache() / lru_cache(maxsize=None) → functools.cache at __init__.py:203, :959, :964.
• _tags.py:_get_cpython_abi contains a version < (3, 8) pymalloc branch that can never run, and the whole function exists only as a fallback for CPython ≤ 3.8.7 on Windows (per its own comment) — on 3.9+ EXT_SUFFIX always parses, so _get_cpython_abi and its _get_config_var helper are effectively dead. Worth either deleting or keeping deliberately with a note.
• Every file has from __future__ import annotations, yet the TYPE_CHECKING blocks still import List, Dict, Optional, Union, Tuple, Type, DefaultDict from typing — and _editable.py:310 already uses lowercase dict[str, str], so the style is inconsistent. Enabling ruff's UP (pyupgrade) rule set would mechanically convert to builtin generics and PEP 604 unions, and catch the functools.cache upgrades too.
• Tooling nit: bare ruff check (the documented command) fails with E902 on the intentionally-dangling fixture symlinks tests/packages/symlinks/{baz,qux}.py; it only passes today because pre-commit filters the file list. Adding those two paths to tool.ruff.exclude makes the documented command work.

Typos

Comment-only, but there are a few: "determninistically" (__init__.py:176, _editable.py:228), "libray" (__init__.py:466), "correcly" (__init__.py:1213), "unexising" (_tags.py:118), "reelases" (_tags.py:148), "determiniation" (_tags.py:175), "edition.." (_util.py:36), and a stray space in def _log(string: str , ...) (__init__.py:224).

Overall the codebase is in good shape — tight scope, well-commented, and most of the historical platform workarounds are still justified. The items I'd actually act on first are the meson version drift (1), the RPath check=True (3), and the path.replace no-op (2); each is a small, test-friendly fix. Happy to implement any of these — for the bug fixes I'd add the regression tests first per your convention.

[rgommers]

Thanks @henryiii! This is useful I think. Good to hear that it's all lower-prio things.

Happy to generate fixes for things if other maintainers want me to!

This hasn't come up before in this project, so I'll state my position here: I'm fine with PRs that contain AI-generated code, as long as they are opened by a human who takes full responsibility for its contents (this is what most projects in the ecosystem seem to have settled on, e.g. https://scipy.github.io/devdocs/dev/conduct/ai_policy.html). The same goes for content in issues, and anywhere else in the project space (it looks like you did audit this content by hand for false positives, so all good I think).

@dnicolodi can you comment as well? I see you already opened a few PRs (gh-862, gh-863) to address some of the issues found by this review (xref'ing this issue may be useful).

[dnicolodi]

I find this report useful too. However, some points raised are not correct. I cannot say of that is because @henryiii didn't audit them: understanding that the AI claim is not correct takes some extra thinking and some familiarity with the problem domain.

I have some moral reserves on the use of LLMs, but I don't have the strength to try to explain these. Therefore, I am fine with LLM generated content, as long as I am interacting with an human that expresses themselves in human ways and not with a machine. The way this report is written is fine, but I don't want to have to read walls of machine generated content.

I see you already opened a few PRs (gh-862, gh-863) to address some of the issues found by this review (xref'ing this issue may be useful).

Writing the patch for these was very quick while verifying the issue, thus I went ahead and submitted them. I wanted to tag this issue in the commit messages but AFAIK there isn't a way to do that that GitHub recognizes that does not cause the issue to be closed once a single PR is merged. I was going to comment on this issue with the linked PRs. I'm happy to do it in a better way, if one exists.

[rgommers]

there isn't a way to do that that GitHub recognizes that does not cause the issue to be closed

If you just say "xref gh-xxx" instead of Closes gh-xxx" it shouldn't close it. Sometimes doing it in the PR description only, rather than in the commit message, is useful, to avoid generating extra cross-link notifications on the issue, is the way to go.

[dnicolodi]

If you just say "xref gh-xxx" instead of Closes gh-xxx" it shouldn't close it

AFAIK this does not link the issue to the pull request, the link is still only pull request to issue (in the sense that the pull request description contains a link to the issue, but nothing more).

[rgommers]

Right, not in the right sidebar, but it does show up here (see "dnicolodi mentioned this ..." right above) and it's clear when looking at the PR as well.

[dnicolodi]

1. _MESON_REQUIRED_VERSION is out of sync with pyproject.toml

#862

2. No-op str.replace in the editable path hook (mesonpy/_editable.py:352).

Is there a bug because of the missing normalization or is the normalization unnecessary? Things seem to work with the code as it is, but the interactions in this code are quite complex, thus there may be a latent bug... Needs some more thinking on my side.

3. Missing check=True on RPath reads (mesonpy/_rpath.py:27 and :75).

This is intentional: we have only heuristic to determine which files need to have RPATH adjusted. Some of the files may not be valid object files, thus a otool or patchelf failure should not be fatal. otool and patchelf are guaranteed to be available. Anyhow, extra RPATH entries does not result in a broken wheel.

4. direct_url.json isn't valid JSON (mesonpy/__init__.py:548-550).

#864

5. Regex typo in the wheel filename pattern (mesonpy/_wheelfile.py:26).

#863

6. StopIteration can mask the split-package error (mesonpy/__init__.py:160). In the purelib/platlib conflict branch, next(entry for entry in wheel_files[other] ...) has no default. If the package was first registered by an install_subdirs entry whose directory walked to zero files, packages has the entry but wheel_files[other] has none, and the user gets a raw StopIteration/RuntimeError instead of the helpful BuildError. Use next(..., None) and degrade the message.

7. Symlink cycles hang sdist() (mesonpy/__init__.py:1063). The while member.issym(): resolution loop never terminates if the archive contains a symlink cycle (a -> b, b -> a). Malformed input, but a bounded loop or visited-set would turn a hang into a warning like the other symlink cases.

8. Outdated 32-bit-on-aarch64 tag (mesonpy/_tags.py:192).

This is wrong.

• WheelFileWriter.write (mesonpy/_wheelfile.py:96-98) slurps each file fully into memory to hash and write it. For scientific wheels with very large shared objects that's a real memory spike.

Sure, because the compiler that just compiled these large shared objects did so streaming to disk...

• _editable.walk() calls dirnames.sort() inside the per-entry loop (mesonpy/_editable.py:229)

#865

• mesonpy/__init__.py:919: the one .format(style=style, cmd=...) call in a file that otherwise uses f-strings.

This looks nicer with the format().

• _env_ninja_command (__init__.py:1236) reuses the name version for both the required version parameter and the subprocess output — correct but needlessly confusing.

I think this is fine as it is.

• _string_or_strings (__init__.py:643): list([value,] if isinstance(value, str) else value) → [value] if isinstance(value, str) else list(value).

#866

• The sdist loop copies member twice when a symlink was resolved (__init__.py:1073 and :1085); harmless, but the second copy's comment ("before starting to modify it") no longer tells the whole story.

I don't understand what the problem is. Avoiding the extra copy would increase complexity considerably for close to no gain.

• @functools.lru_cache() / lru_cache(maxsize=None) → functools.cache at __init__.py:203, :959, :964.

#866

• _tags.py:_get_cpython_abi contains a version < (3, 8)

#866

• Every file has from __future__ import annotations, yet the TYPE_CHECKING blocks still import List, Dict, Optional, Union, Tuple, Type, DefaultDict from typing — and _editable.py:310 already uses lowercase dict[str, str], so the style is inconsistent. Enabling ruff's UP (pyupgrade) rule set would mechanically convert to builtin generics and PEP 604 unions, and catch the functools.cache upgrades too.

I though that subscriptable builtins were added in 3.10. Anyhow, I think that updating these as the code is modified is a bit nicer than doing all of them in one sweep. Back when I tested it, ruff's UP rules didn't really respect the minimum Python version and upgraded to the latest syntax. Maybe I was wrong, or this has changed.

• Tooling nit: bare ruff check (the documented command) fails with E902 on the intentionally-dangling fixture symlinks tests/packages/symlinks/{baz,qux}.py; it only passes today because pre-commit filters the file list. Adding those two paths to tool.ruff.exclude makes the documented command work.

pre-commit filters the file list? I don't think so. Anyhow, I get the same error locally, but it looks like a bug in ruff to me.

Comment-only, but there are a few: "determninistically" (__init__.py:176, _editable.py:228), "libray" (__init__.py:466), "correcly" (__init__.py:1213), "unexising" (_tags.py:118), "reelases" (_tags.py:148), "determiniation" (_tags.py:175), "edition.." (_util.py:36), and a stray space in def _log(string: str , ...) (__init__.py:224).

#866

[henryiii]

My policy for AI generated text is that it's always marked - anything below the AI disclaimer is AI generated. Anything without a disclaimer is written by me. (And I approve or manually post everything). I don't like reading AI generated text either (especially from the commercial models - GLM 5.1 actually produced simi-readable text, haven't tested 5.2 as much), but I think it's much better when it's clearly marked, then I don't feel obligated to read it, and can better utilize the information in it.

The report itself is directly as produced (and as I described) - I find these very useful, but working through them (as above) is best with deep knowledge of the codebase, which I don't have here.

2: It says it only is a problem on Windows? I'd take anything it could not verify (I'm running on a Mac) with more skepticism than something it could verify. (It seems to have in general done less verification than normal, often it builds reproducers before reporting any issue in the "bugs" section).

8: this does look like it is wrong. Are 6-7 wrong too?

I don't understand what the problem is. Avoiding the extra copy would increase complexity considerably for close to no gain.

I think the comment here is that the comment is wrong, not that you should try to avoid double copies. Generally feel free to ignore any simplification that you don't like ("correct but needlessly confusing") - it's subjective at best!

I though that subscriptable builtins were added in 3.10

No, they were added in 3.9. 3.10 added the | syntax for types. That might have been the issue with UP too (I use it in all my repos and haven't had any issues with it confusing the floor, and I have 3.8, 3.9, and 3.10's).

[dnicolodi]

2: It says it only is a problem on Windows?

Yes, if anything it is a problem only on Windows. But I am still unsure whether the normalization is required at all. I need to dig deeper in that part of the code (and probably produce some tests).

8: this does look like it is wrong. Are 6-7 wrong too?

6 and 7 are correct observations. However, the conditions for triggering them are between very unlikely to impossible. I haven't had time to determine whether they are really impossible and I left these for later.

I don't understand what the problem is. Avoiding the extra copy would increase complexity considerably for close to no gain.

I think the comment here is that the comment is wrong, not that you should try to avoid double copies. Generally feel free to ignore any simplification that you don't like ("correct but needlessly confusing") - it's subjective at best!

I don't find this confusing. We are copying an object before modifying it. The fact that we may already be operating on a copy is not very relevant. I'll let this as it is.

I though that subscriptable builtins were added in 3.10

No, they were added in 3.9. 3.10 added the | syntax for types. That might have been the issue with UP too (I use it in all my repos and haven't had any issues with it confusing the floor, and I have 3.8, 3.9, and 3.10's).

I'll revisit enabling the UP rules. However, I would avoid code churn to update the typing annotations to the modern syntax without other simplifications or advantages.

[eli-schwartz]

I have some moral reserves on the use of LLMs, but I don't have the strength to try to explain these. Therefore, I am fine with LLM generated content, as long as I am interacting with an human that expresses themselves in human ways and not with a machine. The way this report is written is fine, but I don't want to have to read walls of machine generated content.

(There are people that have done the work to write up explanations for this. I suppose that I have the same or stronger reservations, but I'm unsure to what extent I should be vocalizing them if I'm not also active in a project. Still Gentoo has an unconditional ban and the plan is to add an explicit ban to https://github.com/mesonbuild/meson...)

I though that subscriptable builtins were added in 3.10. Anyhow, I think that updating these as the code is modified is a bit nicer than doing all of them in one sweep. Back when I tested it, ruff's UP rules didn't really respect the minimum Python version and upgraded to the latest syntax. Maybe I was wrong, or this has changed.

I'll revisit enabling the UP rules. However, I would avoid code churn to update the typing annotations to the modern syntax without other simplifications or advantages.

A similar thing can be said about quoted annotations. Meson still has many of these, even though we are now consistent about __future__.annotations -- but I usually review and tell people to omit quotes only for new or modified code. Over time as lines get edited and annotations change these naturally get converted.

I think in general it's a good idea to only do this sort of "style, zero runtime effect" work incrementally. In general, tooling tends to be pretty poor at "only convert newly added code to enforce style rules". Manual review still works as well as it ever did (which I think is pretty decent).

[henryiii]

<offtopic>

FWIW, I don't believe an explicit ban works. That just makes people hide use of LLMs, while a strict disclosure policy is more likely to be followed. (That likely won't perfectly work either, as I know some developers that will not disclose their use of LLMs because it's "their" code and they don't consider it important how it was arrived at, they "own" it.) And then there's the question of what is an LLM; does autocomplete count? Do local models count? Does any automated change, like formatters, count? If it's based on the training data, there are ethically trained models, do those count? What about people who have seen copyrighted code, do they also get banned, since they "trained" on it? What about using code that has AI contributions, like the Linux Kernel, CPython, etc? What about using LLMs for review? What about using them to set up and run dozens of downstream projects and let you know if there are any regressions (which is what I'm doing in scikit-build-core currently).

My plan for learn.scientific-python's guide is to provide three proposed AI_POLICY.md templates, for "All in", "Moderate", and "Minimal", and not provide one for fully banned (though that's really easy to write yourself, so besides the reasoning above, also not much point). Haven't written them properly yet, though, or had much (just a tiny bit of positive) feedback on the plan.

(Also, I have a post about my adventures in AI here)

</offtopic>

Anyway, feel free to close when you are done!