Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Set __file__ constant when using eval_file (#1300) #3233

Merged
merged 13 commits into from
Sep 9, 2021
Merged

fix: Set __file__ constant when using eval_file (#1300) #3233

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
82dc139
Set __file__ constant when using eval_file
Skylion007 Sep 1, 2021
f76ecc7
Use const ref
Skylion007 Sep 1, 2021
5ce8222
Use a move instead
Skylion007 Sep 1, 2021
38c57fb
Revert
Skylion007 Sep 1, 2021
f7ff7d5
Improve test
Skylion007 Sep 1, 2021
e9eb2c8
Guard test with Python version
Skylion007 Sep 1, 2021
93887fe
Fix tests
Skylion007 Sep 1, 2021
1c4d86c
Dont support Python2 API
Skylion007 Sep 1, 2021
bff887c
Drop Python2 eval __file__ support
Skylion007 Sep 1, 2021
3e34089
Hack
Skylion007 Sep 1, 2021
8b3c3ae
Semisupport Python2
Skylion007 Sep 2, 2021
2d5c38f
Take2
Skylion007 Sep 2, 2021
d5e56c3
Remove Python2 support
Skylion007 Sep 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions include/pybind11/eval.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ object eval_file(str fname, object global = globals(), object local = object())
pybind11_fail("File \"" + fname_str + "\" could not be opened!");
}

if (!global.contains("__file__")) {
global["__file__"] = std::move(fname);
}

#if PY_VERSION_HEX < 0x03000000 && defined(PYPY_VERSION)
PyObject *result = PyRun_File(f, fname_str.c_str(), start, global.ptr(),
local.ptr());
Expand Down
2 changes: 2 additions & 0 deletions tests/test_cmake_build/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,7 @@

import test_cmake_build

__file__ # Test this is properly set
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knew it!

isinstance(__file__, str) == False on Python2

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit random, but this looks very similar:

https://github.com/pypa/pip/pull/1538/files

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that because it should be defined in Python 2, the previous code was checking for NameError.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit random, but this looks very similar:

https://github.com/pypa/pip/pull/1538/files

Nah, this is same code snippit is suppose to be done in the Python2 side to convert file to the appropriate format.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 for sys.getfilesystemencoding()? If not we have to import, getattr, call.


assert test_cmake_build.add(1, 2) == 3
print("{} imports, runs, and adds: 1 + 2 = 3".format(sys.argv[1]))