-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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.
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
Move to absolute file paths for module.__file__ #62616
Comments
$ touch blah.py; ./python -c "import blah; print(blah.__file__)"
./blah.py Should really change that to be an absolute path since the file's location is not accurate if you call os.chdir(). |
Isn't this because the first entry of sys.path is '' when python is started without a script? All other paths on sys.path are already absolute paths (tested with a relative path in $PYTHONPATH). If that's correct, just adding os.getcwd() instead of '' as the first entry of sys.path would fix this issue, and would also make sure that imports of modules next to the script keep working when the script uses os.chdir. The disadvantage is that some users might depend on the current behavior :-( |
That's exactly why it currently happens. What I would do is change importlib to just associate '' with os.getcwd() in FileFinder. That locks down where the module was found to an absolute path while still allowing '' to function as the cwd and be dynamically calculated. |
Brett's plan sounds good to me. We should also check we make sure __main__.__file__ is always absolute. |
PathFinder or FileFinder? Changing PathFinder._path_importer_cache(cls, path) seems to fix the issue. See line 1302 in _bootstrap.py. if path == '':
- path = '.'
+ path = _os.getcwd() $ touch blah.py; ./python -c "import blah; print(blah.__file__)"
/home/mmay/cpython/blah.py |
I actually meant FileFinder but PathFinder is a good point. In FileFinder you need to change |
A few days ago I tried the change:
I'm traveling for the next few days but I'll probably give it a second look when I get some free time. |
Make sure to run "make" to rebuild the frozen module - editing |
Thanks for the heads up, Nick. I've worked with _bootstrap.py before, so I'm familiar with the cycle. |
Figured it was worth mentioning, since I've been caught by forgetting I suspect you're right that it's just a case of '.' passing the truth |
Nick, it was definitely a good thing to mention. I had to learn the "edit, build, test" cycle the hard way my first time. It took me a good 15-20 minutes to figure out why none of my edits seemed to change anything :) Anyhow, here's how I see the issue. It seems like we have three main options: In option one, we only modify PathFinder._path_importer_cache(). if path == '':
- path = '.'
+ path = _os.getcwd() This associates the cwd with FileFinder(cwd) in sys.path_importer_cache In option two, we only modify FileFinder.__init__(). - self.path = path or '.'
+ if not path or path == '.':
+ path = _os.getcwd()
+ self.path = path This associates '.' with FileFinder(cwd) in sys.path_importer_cache. In option three, we modify both PathFinder and FileFinder. In PathFinder._path_importer_cache() we remove the line that reassigns path to '.' if path is the empty string.
In FileFinder.__init__(), we set path to _os.getcwd() if path is false.
+ self.path = path or _os.getcwd() This associates the empty string with FileFinder(cwd) in sys.path_importer_cache. What are your thoughts? Which solution would you all support? |
So this is bringing up a sticky situation that I ran across when initially implementing all of this: what should sys.path_importer_cache use as a key? '' would be what happens with Madison's option 3, and with option 1 it would be os.getcwd(). Now if you iterate through sys.path you won't find what '' connects to. Then again, it won't be accurate if you change the directory either. So the question becomes should sys.path_importer_cache reflect exactly what is in sys.path or what should be cached for a finder based on what import should do (i.e. the full path and change based on the cwd)? |
I quickly ran the tests with each of the above edits to see what bits of the test suite would break. With option 1 (edits to PathFinder only):
With option 2 (edits to FileFinder only):
With option 3 (edits to PathFinder and FileFinder):
So using the cwd as a key in sys.path_importer_cache seems to break fewer tests than using '' as a key does. Perhaps that counts for something. In test_importlib, the only test to fail was In regard to sys.path_importer_cache behavior -- at first glance, I'd lean toward either:
Key: os.getcwd() => Value: FileFinder(os.getcwd())
Key: '.' => Value: FileFinder('.') or with a minor tweak for consistency with sys.path: Key: '' => Value: FileFinder('.') Have you given the issue any more thought, Brett? |
I'm currently leaning towards having sys.path_importer_cache store the actual directory name. |
Makes sense to me. It the problem that sys.path will still have '' in it and it may be hard to figure out what it means (and how it maps to sys.path_importer_cache)? |
It seems to me that by *not* doing that, we may have a bug if the cwd changes and the import machinery returns a stateful importer that remembers the path name. Using the actual filesystem path for everything other than sys.path sounds like a much better option. |
To answer Eric's question: yes. Since no one seems to be screaming that sys.path be the only place to have the concept of a relative path, then let's use only absolute paths except in sys.path for ''. |
Exactly what I meant by "Creating a new entry in sys.path_importer_cache after changing directories and importing a new module", but expressed much more succinctly :) Good to see we're on pretty much the same page. Here's a (very) preliminary patch for the issue that stores the directory name as a key in sys.path_importer_cache and ensures module.__file__ is an absolute path. I've also modified test_path_importer_cache_empty_string in test_importlib/import_/test_path.py to use os.getcwd() instead of os.curdir as the key for sys.path_importer_cache. Finally, I've added a test to test_import.py that tests that module.__file__ is equivalent to os.path.abspath(module.__file__). Look it over when you get a chance and let me know what you would change. Thanks... |
Here's a minor revision to that patch removing an unnecessary @skip_if_dont_write_bytecode decorator from the test I added to test_import.py. No docs changes are included in the current patch -- I'm guessing this should probably wait until we have all the other details ironed out. |
New changeset 76184b5339f2 by Brett Cannon in branch 'default': |
I went with option 1 (changed PathFinder to consider '' the cwd) which allowed me to flat-out remove the special-casing for '' in FileFinder. Thanks for the initial patch, Madison! |
New changeset 33844153cd02 by Brett Cannon in branch 'default': |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: