Skip to content

BUG: do not mutate install_rpath in fix_rpath - #918

Closed
BlackHat-Dot wants to merge 1 commit into
mesonbuild:mainfrom
BlackHat-Dot:fix-rpath-mutation
Closed

BlackHat-Dot wants to merge 1 commit into
mesonbuild:mainfrom
BlackHat-Dot:fix-rpath-mutation

Conversation

@BlackHat-Dot

Copy link
Copy Markdown
Contributor

In RPATH.fix_rpath(), assigning 'new_rpath = install_rpath' and mutating it via '+=' and '.append()' mutated the caller's install_rpath list in place.

Additionally, _Entry defined mutable defaults 'install_rpath: List[str] = []' and 'build_rpath: List[str] = []', sharing mutable state across instances.

Copy install_rpath with 'list(install_rpath)' in fix_rpath(), annotate with Sequence[str], and use immutable tuple defaults '()' in _Entry.

In RPATH.fix_rpath(), assigning 'new_rpath = install_rpath' and mutating it via '+=' and '.append()' mutated the caller's install_rpath list in place.

Additionally, _Entry defined mutable defaults 'install_rpath: List[str] = []' and 'build_rpath: List[str] = []', sharing mutable state across instances.

Copy install_rpath with 'list(install_rpath)' in fix_rpath(), annotate with Sequence[str], and use immutable tuple defaults '()' in _Entry.
@rgommers

Copy link
Copy Markdown
Contributor

@BlackHat-Dot did you encounter an actual bug, or are you just running some AI tool over the code base and making random changes as a result? If you did encounter a bug, you really should clearly explain what it was, not just open a PR without any motivation and the PR description just talking about the line-by-line contents of the PR (which is useless).

@BlackHat-Dot

Copy link
Copy Markdown
Contributor Author

Hi @rgommers, I apologize for the noise and the poor description.

To be completely honest, I was reviewing the codebase and noticed the mutable default install_rpath = [] in _Entry and the install_rpath mutation in fix_rpath(). I did not encounter this as a failure in an active downstream package build.

I understand that speculative cleanup PRs without a real-world reproducer add unnecessary review burden. I'm sorry for the inconvenience, and I'll go ahead and close this PR.

@rgommers

Copy link
Copy Markdown
Contributor

thanks, in that case closing this is the right thing to do indeed

@rgommers rgommers added the invalid This doesn't seem right label Oct 12, 2026
@dnicolodi

Copy link
Copy Markdown
Member

It does not result in any issue, and the added tests are purely AI slop, nevertheless it is not desirable to mutate install_rpath.

@BlackHat-Dot

Copy link
Copy Markdown
Contributor Author

I closed this PR myself. To clarify how it came about: while testing something small in the codebase, I saw a behavior I wanted to reproduce, and I used AI to help write the tests. I should have explained that in the PR description instead of only describing the line-by-line changes. I also didn't show a real failure, so I understand the concerns. Sorry for the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

invalid This doesn't seem right

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants