loader.py: Wrong directory inserted into sys.path for modules
Nobody has claimed this yet.
Assessment
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Newbie friendliness
- 45/100
Research direction
Read invoke/loader.py, especially load and the path insertion around the referenced line, and compare module and package resolution. Update the inserted path so package-based tasks can import sibling modules such as barutils, then add tests covering both tasks.py and tasks/init.py cases.
Written by the indexing model from the issue text.
Description
Background
In loader.py, invoke uses this load function to load the tasks collection. This function attempts to detect whether tasks is a module or a package:
It first sets enclosing_dir to be the parent directory of whatever file the import resolved to (this is tasks.py for modules, and __init__.py within the package directory for packages). Then, it checks if the discovered ModuleSpec has a non-empty entry for .parent, which would indicate that the imported tasks is a package. If it is a package, it sets module_parent to be one level above enclosing_dir. If it is not a package, module_parent is set to be enclosing_dir.
Bug
This is all correct, however the path that gets inserted into the path is always enclosing_dir, not module_parent. If we have:
foo
| -- pyinvoke
| -- tasks.py
and then resolve the ModuleSpec for tasks, we correctly insert /foo/pyinvoke into the path. However, if we have:
foo
| -- pyinvoke
| -- tasks
| -- __init__.py
we end up inserting /foo/pyinvoke/tasks into the path, which is wrong, the path inserted should still be /foo/pyinvoke.
Note: #944 is similar but predates the refactor to load that happened in 2.1.3. In https://github.com/pyinvoke/invoke/commit/aa8b815d7a7a43c8ac814c6deb9c4555b8ac6b9e logic was added to fix the search path for config files, but that did not extend to search path for packages.
Note 2: This is all fine and dandy for most use cases where everything lives in tasks or tasks.py. This only broke because we have the following structure:
foo
| -- pyinvoke
| -- tasks
| -- __init__.py
| -- foo_task.py
| -- barutils
| -- __init__.py
| -- bar.py
From within foo_task.py we have from barutils.bar import bartender, which won't work when the inserted path is /foo/pyinvoke/tasks/ since barutils isn't in tasks.
Solution?
I'd like to propose that in:
we replace enclosing_dir with module_parent.
@bitprophet if this fix seems reasonable I can put in a PR and add some tests for this.
- Dominant language
- Python
- Stars
- 4.8k
- Forks
- 412
- PR merge metrics
- No merged PRs in 30d
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from pyinvoke/invoke
-
[Security] Shell injection via Context.cd() path argument — metacharacters not escaped (CWE-78) Open
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 70/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 70/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 64/100
Similar issues
-
bug confirmed issue
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
open-webui/open-webui#30750 · 1 comment ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
-
enhancement
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
OpenwaterHealth/openmotion-bloodflow-app#604 · 1 comment ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 70/100
-
good first issue
Difficulty 1/5 Under an hour Newbie friendliness 90/100