FIX: fall back to kpsewhich when luatex starts but is unusable#31984
FIX: fall back to kpsewhich when luatex starts but is unusable#31984kikeenator wants to merge 2 commits into
Conversation
|
Thank you for opening your first PR into Matplotlib! If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks. We also ask that you please finish addressing any review comments on this PR and wait for it to be merged (or closed) before opening a new one, as it can be a valuable learning experience to go through the review process. You can also join us on discourse chat for real-time discussion. For details on testing, writing docs, and our review process, please see the developer guide. We strive to be a welcoming and open project. Please follow our Code of Conduct. |
| if lk: | ||
| path = lk.search(filename) | ||
| else: | ||
| if not path: # luatex unavailable or unusable; fall back to kpsewhich. |
There was a problem hiding this comment.
I'm not sure we want to fall back if lk succeeds but returns None due to a missing path (it's just extra cost for little gain); maybe the two cases need to be distinguished?
There was a problem hiding this comment.
My thought process behind that was that a missing path leads to an error in any way, so its really time-complexity versus code complexity/readability question. The two assumptions I made are the following: 1) The time-save is only ever constant, since not finding a file leads to error and abort 2) Calling kpsewhich (the fallback), is rather quick anyway.
Both assumptions arent fact checked, and even if they hold, the question is still a philosohpical one. I decided against the more efficient solution since I believe its chasing an efficiency dead-end in a robust piece of code.
I am not in the position to judge, I dont know coding philosophy for this project. If youd like I can change it so the cases are distinguished and the fallback isnt called for a really missing part.
There was a problem hiding this comment.
If missing fonts always led to hard crashes then I agree it would be OK to have a slower path if the implementation is much simpler, but I believe this is not the case, essentially due to the case described at #28212 (comment) (although admittedly I didn't check very carefully): we need to support vf fonts with partially missing tfm subfonts. (It's a fairly technical issue, feel free to ask if it's not clear after reading that thread.) As noted in that issue there may be cleaner designs that involver lazier vf loading, but that's not what we have now.
There was a problem hiding this comment.
I was missing the context, thanks for pointing it out. Great learning case for me.
I now changed the code so that search returns "" if communication with luatex was successfull but the path was not found (instead of None like before). I cascaded this change to the find_tex_file function so it now only falls backs if the luatex search genuienly didnt work.
I also added a test that explicitly checks that the fallback isnt called in that specific case.
Thanks for your patience and explanation, really appreciated.
|
Looks good to me, can you squash the commits? |
find_tex_file only fell back to kpsewhich when the luatex process failed to start. If luatex started but then died or became unusable at runtime, its search() returned an empty result and find_tex_file raised FileNotFoundError without ever trying kpsewhich. Make _LuatexKpsewhich.search() return None when luatex is unusable (a broken pipe or an empty read) and "" when luatex works but the file is missing, and fall back to kpsewhich only when search() returns None, so a genuinely missing file does not trigger a needless kpsewhich call. Closes matplotlib#31983
c19e8fb to
9e9e91f
Compare
Done. |
| assert data == correct | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("readline, write_error, expected", [ |
There was a problem hiding this comment.
To be honest I find these mock-based tests rather difficult to follow. Back then, for ffmpeg I wrote test_failing_ffmpeg which uses a different approach (mock ffmpeg extenally as a simple executable); would it be applicable here? Would the resulting test be simpler?
Note that this is not strictly needed, I'm just trying to keep this simpler (IMO).
There was a problem hiding this comment.
Ok. As I see it, we have five different behaviour modes:
- Luatex dies on startup
- Luatex communication fails (oserror)
- Luatex returns a bad communication
- Luatex returns a valid filepath
- Luatex returns that the file is missing
Now 1-3 should fall back to kpsewhich, 4-5 explicitly shouldnt.
I read your ffmpeg test case. We can do the same here for all test cases except 2, since a mid-write oserror is hard to simulate. But the granularity we loose by omitting that test case is not critical in my opinion, since its outcome is identical to case 3.
We currently have three test functions. The first checks the search function, the second/third check the general "find_tex_file" routing. The first is the one that uses the mock-based testing and I would propose to rewrite that to use the same simulated-executable logic from the ffmpeg testcase. For the other two its more efficient and more readable to simulate the search function (see current implementation). Checking with this small granularity can itself be critized, tell me if my test-cases were too restrictive/detailed. Otherwise if you are fine with the proposal, I will rewrite the search testcase.
There was a problem hiding this comment.
On your second question about simplicity: Honestly I think the mock-tests are a better fit. They are cross platform (not posix-only), deterministic (not OS-dependent), and cover test-case 2. Also mock is more standardized and not a custom-built executable simulation. The only advantage in the ffmpeg-style executable test-case I see is that it doesnt hardwire the internal method sequence with poll, then stdin, then stdout. If its just about readability, I could maybe compartmentalize better or write more understandable comments. Thats my input, but obviously decision stays with you.
There was a problem hiding this comment.
If you prefer the mock-based tests I won't insist. My main concern is that the test becomes highly coupled with the internal implementation (poll, stdin, stdout, semantics of _LuatexKpsewhich and _check_and_log_subprocess). I agree that slightly expanding the comments, as you suggest, may alleviate these concerns.
There was a problem hiding this comment.
I have expanded commenting for the functions, also explicitly stating the trade-off made by using mock-based testing, so that a future maintainer will not struggle figuring out why internal rearrangement or other non-invasive changes break testing. I find the comments to be quite explanatory now, but as always, someone with more distance than me should check readability and understandability.
If there is still anything causing a headache, give me a quick heads up.
Clarify search()'s None/"" return contract in dviread, and document what each test covers, why the search() test is mock-based, and the coupling tradeoff that implies.
find_tex_file spawns a luatex instance via which it can locate files in the path of a latex installation. If this doesnt work, we fallback to calling kpsewhich directly. Currently this only happens when the luatex process failed to start. If luatex started but then died or became unusable at runtime, its search() returned an empty result and find_tex_file raised FileNotFoundError without ever trying kpsewhich. In practice this means a user whose luatex is broken cant use usetex at all, even though kpsewhich works fine on their machine
I fixed this by making _LuatexKpsewhich.search() return None on a broken pipe or empty read, and restructuring find_tex_file to fall back to kpsewhich whenever the luatex route yields no usable path.
AI disclosure:
I used AI to get feedback on my proposal of how to fix, with the idea and diagnosis being fully mine (obviously inspired by the issue creators hotfix to give credit where its due). I also used it to draft test cases. I understand the code fully and can explain and defend all decisions undertaken.
Closes #31983