mdevenv: fixes/cleanups for #15086 #15128
Open
+3
−16
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We don't test shell integration in CI -- as is obvious from the fact that commit 202d15d added additional code that, in the testsuite would fatally error. I don't think it is a good idea to add hacky workarounds for software nobody understands or uses and isn't tested in CI. Which is a description that has always fit "fish" like a glove (it is a shell that doesn't even support being run as a login shell, among numerous other issues). In mesonbuild#15086 (review) I raised concerns about its inclusion, but due to the holiday season was unable to respond in the narrow window of time between being asked for my reasons and the PR being merged without review. Still, the zsh changes are great, let's keep them. Zsh is a widely used shell that is well understood without hacks.
It was proposed to be added, but then questioned and removed in mesonbuild#15086 (comment) > I'm not sure this is correct. IIUC, this file is passed as a > configuration file to bash thus it needs to be written with the system > encoding, which in Python should be what you get when encoding is not > specified. It is insane(ly broken) to use a non-ascii-compatible encoding, and we exclusively write ascii. We also test in CI that the encoding= argument is *explicitly* passed, and we do so precisely because python 3.15 changes the meaning of `encoding=None` from "system encoding" to "utf-8", so in the most absolute sense, it is simply wrong to review an `encoding=` PR and say to remove the encoding to get the system encoding. The bash logic has had that problem forever; the zsh logic only since the recent PR. Switch both to explicitly require utf-8, which is ascii-compatible and everyone should use anyway. Fixes: ``` Traceback (most recent call last): File ".../mesonbuild/mesonmain.py", line 193, in run return options.run_func(options) ~~~~~~~~~~~~~~~~^^^^^^^^^ File ".../mesonbuild/mdevenv.py", line 226, in run tmprc = tempfile.NamedTemporaryFile(mode='w') File "/usr/lib/python3.13/tempfile.py", line 574, in NamedTemporaryFile encoding = _io.text_encoding(encoding) EncodingWarning: 'encoding' argument not specified ERROR: Unhandled python exception This is a Meson bug and should be reported! ```
prompt_command already has "[]" pre-added, so zsh ended up with e.g. ``` [[trivial test]] $ ``` whereas bash used: ``` [trivial test] $ ```
xclaesse
reviewed
Oct 16, 2025
| tmprc.flush() | ||
| args.append("--rcfile") | ||
| args.append(tmprc.name) | ||
| elif args[0].endswith('fish'): |
There was a problem hiding this comment.
The SIGINT issue was fixed upstream in 2019 and should be reverted. Don't know about the rest.
|
I don't have a comment on the first patch, but the I'm happy to merge those separately if the fish patch needs further discussion. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.