Fix several issues found during Windows QA #793
Merged
Conversation
This file contains 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
apyrgio
force-pushed
the
2024-04-qa-fixes
branch
from
3c12e4f to
7586cd5
Compare
deeplow
reviewed
May 3, 2024
There was a problem hiding this comment.
Thanks for digging into this complex termination rabbit hole. I have a few suggestions in their own threads.
Other than that, my last suggestion is to add already changelog lines because this fixed at least two separate things and will be easier to attribute now than when releasing.
Fix a failing test case on Windows, due to a character that cannot exist in a filename.
On Windows, if we somehow attempt to archive the same document twice (e.g, because it got archived once, and then we copy it back), we will get an error, because Windows does not overwrite the target path, if it already exists. Fix this issue by always removing the previously archived version, when performing the next archival action, and update our tests.
On Windows, if we don't use the `startupinfo=` argument of subprocess.Popen, then a terminal window will flash while running the command. Use `startupinfo=` when killing a container, as we do for every other command.
The `exit()` [1] function is not necessarily present in every Python environment, as it's added by the `site` module. Also, this function is "[...] useful for the interactive interpreter shell and should not be used in programs" For this reason, we replace all such occurrences with `sys.exit()` [2], which is the canonical function to exit Python programs. [1]: https://docs.python.org/3/library/constants.html#exit [2]: https://docs.python.org/3/library/sys.html#sys.exit
In d632908 we improved our `replace_control_chars()` function, by replacing every control or invalid Unicode character with a placeholder one. This change, however, made our debug logs harder to read, since newlines were not preserved. There are indeed various cases in which replacing newlines is wise (e.g., in filenames), so we should keep this behavior by default. However, specifically for reading debug logs, we add an option to keep newlines to improve readability, at no expense to security.
Remove an extra call to `replace_control_chars()`, as well as an unnecessary method.
You're right, I've added some changelog entries in 639dccd. I do not refer to #791, since it's an issue that arose due to an incomplete fix to #749. Therefore, I just refer to #749. |
deeplow
approved these changes
May 9, 2024
Gracefully terminate certain conversion processes that may get stuck when writing lots of data to stdout. Also, handle a race condition when a conversion process terminates slightly after the associated container. Fixes #791
Create the build directory first, and then add the PySide6 package in it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fix several minor to medium issues that were found while doing QA on Windows for 0.6.1 (see #785 (comment)), as well as an issue reported by @naglis.
Fixes #790
Fixes #791