[BUGFIX] Prevent fatal error due to uparsed lines when makemkvcon rips successfully (#1688)#1699
Conversation
…s successfully (automatic-ripping-machine#1688) Check for buffer content only if the process returns an error.
|
|
Great job on the pr, @joelishness! |
microtechno9000
left a comment
There was a problem hiding this comment.
Nice fix, and simple
f7f1515
into
automatic-ripping-machine:main
|
Not so sure if this is such a nice fix. The way I read it, The line now is effektivly the same as "if False" since it will never get triggered. |
Can you clarify, not sure I follow |
|
We raise if The proper fix would be to move the line which outputs the logger warning for unparsed lines before the initial raise and remove the raise from the conditional branch. That way, the logger warning would still be visible indicating that the parser needs adjustments. |
| @@ -1191,7 +1191,7 @@ def run(options, select): | |||
| yield data | |||
| if proc.returncode: | |||
There was a problem hiding this comment.
This prevents failed processes from getting any further than here.
| if proc.returncode: | ||
| raise MakeMkvRuntimeError(proc.returncode, cmd, output=os.linesep.join(buffer)) | ||
| if buffer: | ||
| if buffer and proc.returncode: |
There was a problem hiding this comment.
So we will never see it here.
| if buffer: | ||
| if buffer and proc.returncode: | ||
| logging.warning(f"Cannot parse {len(buffer)} lines: {os.linesep.join(buffer)}") | ||
| raise MakeMkvRuntimeError(proc.returncode, cmd, output=os.linesep.join(buffer)) |
There was a problem hiding this comment.
The fix would be to just remove the raise.
ukos-git
left a comment
There was a problem hiding this comment.
I forgot to submit the comments.
[BUGFIX] Prevent fatal error due to uparsed lines when makemkvcon rips successfully (#1688)
Description
Check for buffer content only if the process returns an error.
Created Pull Request for suggestion by @swolfe2
Fixes #1688 BUG: Parser Fragility causes Fatal Error on successful rips (makemkv.py)
Type of change
How Has This Been Tested?
Checklist:
Changelog:
Logs
Error case demonstrating the issue that with MakeMKV debug logging, MakeMKV rip was successful (exit 0) yet ARM throws fatal error.
ARM and MakeMKV debug logs attached:
logs-Alien3-ng.zip
No error case demonstrating that without MakeMKV debug logging, MakeMKV rip was successful (exit 0) and ARM finished successfully.
ARM and MakeMKV debug logs attached:
logs-Alien3-ok-no-debug.zip
No error case demonstrating patch effectiveness with MakeMKV debug logging, MakeMKV rip was successful (exit 0) and ARM finished successfully.
logs-Alien3-ok-debug.zip