fix: Set __file__ constant when using eval_file (#1300) #3233
Conversation
Skylion007
changed the title
tests/test_cmake_build/test.py
Outdated
| @@ -3,5 +3,7 @@ | |||
|
|
|||
| import test_cmake_build | |||
|
|
|||
| __file__ # Test this is properly set | |||
There was a problem hiding this comment.
What do you think about making this slightly stronger (and look more like a test)? E.g.
assert isinstance(__file__, str) # Test this is properly set.
There was a problem hiding this comment.
I knew it!
isinstance(__file__, str) == False on Python2
There was a problem hiding this comment.
Oh, because it is unicode even on Python 2? — I think that counts as a bug, too!
Although not a surprising one given how pybind11 works internally. I think in that case we want to keep the test as is (isinstance str), but encode in the C++ code.
Give me a moment to look around a bit what the expections are for file with Python 2.
There was a problem hiding this comment.
A bit random, but this looks very similar:
There was a problem hiding this comment.
Maybe more pragmatically: guard the new C++ code with #ifdef for Python 3, and the test with not env.PY2.
I.e. we're simply leaving Python 2 behind (instead of going through hoops for something that is practically dead already).
There was a problem hiding this comment.
I don't like that because it should be defined in Python 2, the previous code was checking for NameError.
There was a problem hiding this comment.
A bit random, but this looks very similar:
Nah, this is same code snippit is suppose to be done in the Python2 side to convert file to the appropriate format.
There was a problem hiding this comment.
I'm good with either:
- status quo for Python 2 (carry forward existing shortcoming),
- or fixing properly.
For the latter, is there a Python C API forsys.getfilesystemencoding()? If not we have to import, getattr, call.
Skylion007
changed the title
tests/test_cmake_build/test.py
Outdated
| @@ -3,5 +3,10 @@ | |||
|
|
|||
| import test_cmake_build | |||
|
|
|||
| from .. import env | |||
|
|
|||
| if env.PY3: | |||
There was a problem hiding this comment.
Without the matching guard in C++ we are as much creating a problem as we are solving one.
|
@rwgk Can you investigate fixing the encoding then for Python2, not familiar with searching the CPython documentation. |
include/pybind11/eval.h
Outdated
| @@ -136,6 +136,14 @@ object eval_file(str fname, object global = globals(), object local = object()) | |||
| pybind11_fail("File \"" + fname_str + "\" could not be opened!"); | |||
| } | |||
|
|
|||
| // Python2 API requires this to be encoded to filesystemencoding. | |||
There was a problem hiding this comment.
This looks great.
I see the env.PY3 is giving us trouble. There is another easy trick, JIC there isn't a better one:
if str is not bytes: # Python 2 is not supported.
(We'll find that easily when we clear out Python 2 with the firehose in a few months.)
|
Looks like a flake. |
Yep, download issue. Seeing that a lot. |
|
Actually, after thinking it over, I am tempted to just leave the global file in Python2 (as UTF-8 bytes) because there is chance it could be right (if the filesystem encoding is UTF-8), and it shouldn't break any previous code because any code that required file had to set it byitself with the global dict and this code snippit will not interfere with that path. @henryiii Is there an easy way to encode to the getfilesystemencoding ? I know there is a C-API for codecs which makes this easier. @rwgk Going to revert to the bytes method for now as it will usually be correct, if someone wants to fix it they can. |
There was a problem hiding this comment.
I’m not very worried about Python 2 support, as long as it doesn’t regress for now, I’m fine with it. AFAICT, this just means that __file__ wasn't previously available, but now it is - but on Python 2, it might be misencoded if not unicode.
It is possible that an invalid unicode sequence could show up here, but if something really bad happens, Python 2 users can just set __file__ manually.
|
(I'd also be okay to leave it unset on Python 2 at this stage) |
I think that really is the best, because ...
nobody wants to mess with Python 2 changes anymore. Just leaving everything as-is as much as possible is sure to be the least disruptive. |
Description
Closes #1300 by setting the
__file__constant when running eval_file.Suggested changelog entry: