Fix 5701: avoid dev_tools/modules.py print_version failing#5705
Fix 5701: avoid dev_tools/modules.py print_version failing#5705CirqBot merged 4 commits intoquantumlib:masterfrom mhucka:fix-5701
dev_tools/modules.py print_version failing#5705Conversation
The implementation of the command `print_version` in `dev_tools/modules.py` involves (eventually) calling the function `list_modules()`. That function finds modules by searching for files name `setup.py`, starting in the current directory. When the user runs `python dev_tools/modules.py print_version`, the current directory is the Cirq repo top-level directory. `list_modules()` works by calling `_parse_module()` on every relative directory path found by locating the `setup.py` files recursively. When it executes with a given directory path, `_parse_module()` tries to exec the `setup.py` file found therein. The specific error comes from the fact that the top-level `setup.py` file, _unlike_ all the sub-modules's `setup.py` files, includes imports. Specifically, these imports: ``` from dev_tools import modules from dev_tools.requirements import explode ``` Unfortunately, the current directory (i.e., the top-level Cirq directory) is not in Python's `sys.path` at the time `_parse_module()` exec's the `setup.py` file there, and so the import fails with ``` ModuleNotFoundError: No module named 'dev_tools' ``` This could probably be fixed in a number of ways. One is to make `_parse_module()` temporarily append the current path to `sys.path` before attempting to exec a `setup.py` file. Doing so should hopefully make Python import statements inside `setup.py` files work as expected. That's what this 2-line code change does.
mhucka
changed the title
dev_tools/modules.py print_version failing
|
Incidentally, another way to avoid the error is to put "." on the IMHO, the above would be more confusing to users than if |
|
Does running |
|
@MichaelBroughton If I understand you correctly, you mean to do something like the following? Unfortunately, no, that can't work, because what and that does work. (It's worth mentioning that it will only work if the user is running a Bash/sh/Bourne-derived shell, and not a shell derived from csh, but since elsewhere Cirq seems to assume Bash/sh, it wouldn't be a new requirement.) To be honest, the proposed 2-line patch seems simpler and less prone to user errors or unexpected shell behaviors, but maybe there are other reasons to go this route of using pypath. |
|
another unrelated problem I just noticed trying to figure out what the heck this tool is supposed to do is that if you don't provide a subcommand it fails with a bad error message instead of displaying a help message with subcommand info |
mpharrigan
left a comment
mpharrigan
left a comment
There was a problem hiding this comment.
this looks harmless and fixes a clearly broken script
|
Is anyone using devtools/modules.py print_version? Is it supposed to be a part of the release process. I suggest either:
|
|
@mpharrigan Regarding the behavior when not giving it a subcommand, I encountered that too, and fixed this in PR #5702, which was merged earlier today. |
|
ah, I'm behind |
|
As pointed out by @pavoljuhas, it's better to put the current directory first, in case the user's PYTHONPATH includes another Cirq working tree. Prepending it will make sure that sys.path searches the current repository first.
pavoljuhas
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
…lib#5705) The implementation of the command `print_version` in `dev_tools/modules.py` involves (eventually) calling the function `list_modules()`. That function finds modules by searching for files name `setup.py`, starting in the current directory. When the user runs `python dev_tools/modules.py print_version`, the current directory is the Cirq repo top-level directory. `list_modules()` works by calling `_parse_module()` on every relative directory path found by locating the `setup.py` files recursively. When it executes with a given directory path, `_parse_module()` tries to exec the `setup.py` file found therein. The specific error comes from the fact that the top-level `setup.py` file, _unlike_ all the sub-modules's `setup.py` files, includes imports. Specifically, these imports: ``` from dev_tools import modules from dev_tools.requirements import explode ``` Unfortunately, the current directory (i.e., the top-level Cirq directory) is not in Python's `sys.path` at the time `_parse_module()` exec's the `setup.py` file there, and so the import fails with ``` ModuleNotFoundError: No module named 'dev_tools' ``` This could probably be fixed in a number of ways. One is to make `_parse_module()` temporarily append the current path to `sys.path` before attempting to exec a `setup.py` file. Doing so should hopefully make Python import statements inside `setup.py` files work as expected. That's what this 2-line code change does.
5 participants
The implementation of the command
print_versionindev_tools/modules.pyinvolves (eventually) calling the functionlist_modules(). That function finds modules by searching for files namesetup.py, starting in the current directory. When the user runspython dev_tools/modules.py print_version, the current directory is the Cirq repo top-level directory.list_modules()works by calling_parse_module()on every relative directory path found by locating thesetup.pyfiles recursively. When it executes with a given directory path,_parse_module()tries to exec thesetup.pyfile found therein. The specific error comes from the fact that the top-levelsetup.pyfile, unlike all the sub-modules'ssetup.pyfiles, includes imports. Specifically, these imports:Unfortunately, the current directory (i.e., the top-level Cirq directory) is not in Python's
sys.pathat the time_parse_module()exec's thesetup.pyfile there, and so the import fails withThis could probably be fixed in a number of ways. One is to make
_parse_module()temporarily append the current path tosys.pathbefore attempting to exec asetup.pyfile. Doing so should hopefully make Python import statements insidesetup.pyfiles work as expected.That's what this 2-line code change does.