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.

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

Make shell_tools smarter #4394

Closed
balopat opened this issue Aug 9, 2021 · 2 comments · Fixed by #5566
Closed

Make shell_tools smarter #4394

balopat opened this issue Aug 9, 2021 · 2 comments · Fixed by #5566
Assignees
Labels
area/dev kind/health For CI/testing/release process/refactoring/technical debt items

Comments

@balopat
Copy link
Contributor

balopat commented Aug 9, 2021

Description of the issue

There are two main issues with shell_tools:

  1. The subprocess API got much better since python 3.7 - run_cmd and run_shell functionality could be simplified and aligned better with the new subprocess.run API (with capture_output=True).
  2. There are Windows failures due to missing primitives. This comes up for example when creating a virtual env:
dev_tools\env_tools.py:55: in create_virtual_env
    'virtualenv', None if verbose else '--quiet', '-p', python_path, venv_path, out=sys.stderr
dev_tools\shell_tools.py:196: in run_cmd
    err,
c:\hostedtoolcache\windows\python\3.7.9\x64\lib\asyncio\base_events.py:587: in run_until_complete
    return future.result()
dev_tools\shell_tools.py:120: in _async_wait_for_process
    process = await future_process
c:\hostedtoolcache\windows\python\3.7.9\x64\lib\asyncio\subprocess.py:217: in create_subprocess_exec
    stderr=stderr, **kwds)
c:\hostedtoolcache\windows\python\3.7.9\x64\lib\asyncio\base_events.py:1544: in subprocess_exec
    bufsize, **kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <_WindowsSelectorEventLoop running=False closed=False debug=False>
protocol = <SubprocessStreamProtocol>
args = ('virtualenv', '-p', 'c:\\hostedtoolcache\\windows\\python\\3.7.9\\x64\\python.exe', 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\cirq-pytest\\test_isolated')
shell = False, stdin = None, stdout = -1, stderr = -1, bufsize = 0, extra = None
kwargs = {}

    async def _make_subprocess_transport(self, protocol, args, shell,
                                         stdin, stdout, stderr, bufsize,
                                         extra=None, **kwargs):
        """Create subprocess transport."""
>       raise NotImplementedError
E       NotImplementedError
@balopat balopat added kind/health For CI/testing/release process/refactoring/technical debt items area/dev labels Aug 9, 2021
@pavoljuhas
Copy link
Collaborator

I can take care of this one.

@pavoljuhas
Copy link
Collaborator

pavoljuhas commented Jun 15, 2022

AFAICT, the functionality in run_cmd and run_shell can be fully replaced with the standard subprocess.run.
The functions have a few extra arguments (log_run_to_stderr, abbreviate_non_option_arguments) for outputting the executed command. I intend to add a new function shell_tools.run which will be a thin wrapper to subprocess.run with an extra option to show the executed command; this function will be then used to replace run_cmd and run_shell.

For sake of digestible reviews, the change will be split to several PRs:

  • add a new function shell_tools.run
  • replace shell_tools.run_cmd with shell_tools.run
  • replace shell_tools.run_shell with shell_tools.run
  • purge run_cmd, run_shell and any associated local helpers in the shell_tools module

CirqBot pushed a commit that referenced this issue Jun 16, 2022
Add shell_tools.run, a thin wrapper to subprocess.run with
a few extra arguments for showing the executed commands.
It also supplies updated default values for subprocess.run
to approximate behavior of run_cmd and run_shell functions.
The purpose of this function is to replace the run_cmd and
run_shell functions in the shell_tools module.

Partially implements #4394
CirqBot pushed a commit that referenced this issue Jun 17, 2022
Stop using shell_tools.run_shell which is slated for removal.

Partially implements #4394
CirqBot pushed a commit that referenced this issue Jun 17, 2022
Stop using `shell_tools.run_cmd()` which is slated for removal.
Get command output with `shell_tools.run()` instead.
Update `output_of()` to receive command-line arguments in
a list or string in order to have the same interface as `run()`.
Remove filtering of the optional None arguments,
leave it up to callers to take care of that.

Partially implements #4394
CirqBot pushed a commit that referenced this issue Jun 20, 2022
Stop using shell_tools.run_cmd which is slated for removal.

Partially implements #4394
CirqBot pushed a commit that referenced this issue Jun 21, 2022
Remove obsolete functions `shell_tools.run_cmd` and `shell_tools.run_shell`;
as they are now replaced by `shell_tools.run`.
Remove related classes and auxiliary functions `CommandOutput`, `TeeCapture`,
`_async_forward`, `_async_wait_for_process`.

This completes and closes #4394
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
Add shell_tools.run, a thin wrapper to subprocess.run with
a few extra arguments for showing the executed commands.
It also supplies updated default values for subprocess.run
to approximate behavior of run_cmd and run_shell functions.
The purpose of this function is to replace the run_cmd and
run_shell functions in the shell_tools module.

Partially implements quantumlib#4394
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
Stop using shell_tools.run_shell which is slated for removal.

Partially implements quantumlib#4394
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
…#5541)

Stop using `shell_tools.run_cmd()` which is slated for removal.
Get command output with `shell_tools.run()` instead.
Update `output_of()` to receive command-line arguments in
a list or string in order to have the same interface as `run()`.
Remove filtering of the optional None arguments,
leave it up to callers to take care of that.

Partially implements quantumlib#4394
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
Stop using shell_tools.run_cmd which is slated for removal.

Partially implements quantumlib#4394
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
Remove obsolete functions `shell_tools.run_cmd` and `shell_tools.run_shell`;
as they are now replaced by `shell_tools.run`.
Remove related classes and auxiliary functions `CommandOutput`, `TeeCapture`,
`_async_forward`, `_async_wait_for_process`.

This completes and closes quantumlib#4394
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dev kind/health For CI/testing/release process/refactoring/technical debt items
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants