tempfile: minor changes to handle tempfile encoding exceptions - #4753
Conversation
* add file-level default encoding, * move encoding contents before creating temporary file, * extend handled exception type messages, * ensure closing temporary file handle.
…iable for encoding. Only encoding bytes should be cause of an exception.
… simplify temp file encoding exception handler
…d RELEASE.txt content.
jcbrill
changed the title
IMO these two aren't necessary to catch. Python will generate an understandable error message (in fact, that message is encapsulated in the ones shown) |
I wouldn't call the above real-world exception traceback particularly user-friendly or necessarily clear compared to an explicit exception type given the encoding may be set in third-party code. If directed, will change though. Edit: traceback from master and not PR. |
|
@jcbrill I'd keep your lookuperror handler. Nobody wants a giant stack trace when running a build tool, and we can always reenable with --debug=stacktrace right? |
|
The tests I did on an earlier iteration of this didn't do that (generate full stacktrace)... don't know what's different. |
At present, the current iteration will return the giant stack trace as well. To keep the listings short above, I manually removed text and replaced it with
Unfortunately, that appears to be my doing. Earlier iterations raised an When derived from It is useful to have the exception name in the exception message. It seemed redundant to add it to the reason but it would keep the exception listing short. Something along the lines of the following would be equivalent to the spirit of the current PR. Possible reason text: The test file would have to be updated accordingly. Not handling The key change in the PR was to quickly identify the root cause and location of the exception from the name and reason. I can try and fix it today and push an update soon. |
…s.Errors.UserError instead of Exception Changes: * TempFileEncodeError derived from SCons.Errors.UserError instead of Exception. * Add TempFileEncodeError class name to exception reason text. * Update tests for expected exception reason text changes.
|
I have revised the code, the test file, and pushed the changes. The four examples with the revised code:
The revised code with with |
…G.py test Hard-coded value to force regex failure inadvertently included in last commit.
|
If the new error inherits from |
It'd be nice if it didn't suppress the exception name for subclasses though as the name can convey useful information. Something like this: The |
…construction to prefix reason text with class name.
|
No other changes are planned unless requested/directed subject to the CI tests passing. There were minor changes to the TempFileEncodeError exception class and exception reason text. The tests were updated accordingly.
Correct (see last example below). Example output from test runs based on the latest commit:
|
…in factory method.
…ethod with constructor; simplify tests.
| TEMPFILE_DEFAULT_ENCODING = "utf-8" | ||
|
|
||
|
|
||
| class TempFileEncodeError(SCons.Errors.UserError): |
There was a problem hiding this comment.
We're not really consistent on whether to define Error classes near the point of use, or in Errors.py. That question doesn't have to be resolved here (i.e., consider it a question, not a blocker). The Configure subsystem and the MSVC tool certainly define their own; many are also centralized into Errors.py.
There was a problem hiding this comment.
In this particular case, it could just as easily be an instance of SCons.Errors.UserError with the message/reason prefixed with something along the lines tempfile encoding error: [UnicodeError] ... instead of a standalone exception. I'm not sure this is an exception a user is interested in catching as there really is no way to to "fix it".
|
I have some other minor rearrangements ("better flow", not functional changes) in the base platform module, but I'm not going to try to sneak them into this. This actual code changes look okay to me. |
| tempfileencoding = {tempfileencoding} | ||
| defaultencoding = {defaultencoding} | ||
|
|
||
| DefaultEnvironment() |
There was a problem hiding this comment.
Can we add tools=[] here? It will speed up the test by avoiding tool initializations..
There was a problem hiding this comment.
Yes. That is a bug.
…s.Error.UserError instance and move generated test SConstruct to fixture. Changes: * Remove TempFileEncodeError and replace with instance of UserError. * Move SConstruct template to fixture and add command-line arguments for test configuration. * Rework test script to use fixture and match revised exception text.
|
Revised exception examples error messages:
|
|
Looks great. I'll merge once CI tests complete successfully as expected! |
3 participants
Provide explicit error messages for tempfile encoding failures and reduce the likelihood of an open tempfile file handle at exit.
Based on discussions #4749, #4746, and #4752.
Changes to
TempFileMunge(SCons\.Platform\.__init__.py):TempFileEncodeErrorexception when the tempfile contents encoding fails for known exception types.Example
TempFileEncodeErrorexceptions:UnicodeEncodeError(user-defined encoding):LookupError(user-defined encoding):TypeError(user-defined encoding):UnicodeError(user-modified default encoding):The
TempFileEncodeErrorexception text contains:TempFileEncodeError) is an indication what failed.[UnicodeEncodeError]) and reason is an indication of why it failed.TempFileMungeline indicates if a user-defined encoding value was defined or the default encoding value was used and/or modified.If nothing else, this should make it easier to decipher what us happening should the exception text be posted in the mailing list or in an issue.
Not addressed:
os.remove(file)) in theatexithandler has not been changed. Moving the tempfile encoding before the tempfile is opened and closing the tempfile on write errors should reduce the likelihood of errors upon removal. However, anti-virus software could always be a problem.Feature complete: no additional changes planned.
Contributor Checklist:
CHANGES.txtandRELEASE.txt(and read theREADME.rst).