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

Port to sh v2 or ditch it entirely #34

Closed
gotmax23 opened this issue Feb 23, 2023 · 6 comments · Fixed by #119
Closed

Port to sh v2 or ditch it entirely #34

gotmax23 opened this issue Feb 23, 2023 · 6 comments · Fixed by #119
Labels
antsibull-meta An issue that affects multiple antsibull projects dependencies Pull requests that update a dependency file

Comments

@gotmax23
Copy link
Collaborator

Currently, we pin sh to >= 1.0.0 < 2.0.0, due to breaking changes. We should port antsibull to the new version or make our code compatible with both.

(Another option would be to ditch it altogether and just use subprocess.run or a helper function. I prefer to avoid extra dependencies when possible. Maybe sh is worth keeping, but I will note that it's not compatible with static analysis tools (we need to add pylint ignores every time) which causes some headache.))

@gotmax23 gotmax23 added the dependencies Pull requests that update a dependency file label Feb 23, 2023
@felixfontein
Copy link
Collaborator

I'm currently tending to ditch sh completely. While it doesn't have transitive dependencies, every removed dependency is a good one IMO ;)

@felixfontein
Copy link
Collaborator

(antsibull-core 2.x.y needs to keep sh >= 1.0.0 < 2.0.0 to avoid breakage with antsibull and antsibull-docs releases that claim compatibility with antsibull-core 2.x.y. But we can still stop using it, and plan removal for 3.0.0.)

@gotmax23
Copy link
Collaborator Author

gotmax23 commented Mar 1, 2023

I think I'll propose an antsibull_core.subprocess_utils module that has a run() (sync) and async_run() (async) function. They'll behave like subprocess.run but will always log stderr (flog.debug() by default).

@felixfontein
Copy link
Collaborator

There's also an async version of subprocess: https://docs.python.org/3/library/asyncio-subprocess.html This might be useful for some use-cases as well.

@gotmax23 gotmax23 added this to the antsibull-core v2 milestone Mar 21, 2023
@gotmax23 gotmax23 added the antsibull-meta An issue that affects multiple antsibull projects label Mar 21, 2023
@gotmax23 gotmax23 changed the title Port to sh v2 Port to sh v2 or ditch it entirely Mar 21, 2023
gotmax23 added a commit to gotmax23/antsibull-core that referenced this issue Mar 21, 2023
This adds code to run subprocesses asynchronously and integrates with
the logger. stdout and stderr are logged in real time and always
captured. We return a CompletedProcess object like regular subprocess
does.

Relates: ansible-community#34
gotmax23 added a commit to gotmax23/antsibull-core that referenced this issue Mar 21, 2023
This adds code to run subprocesses asynchronously and integrates with
the logger. stdout and stderr are logged in real time and always
captured. We return a CompletedProcess object like regular subprocess
does.

Relates: ansible-community#34
gotmax23 added a commit to gotmax23/antsibull-core that referenced this issue Mar 21, 2023
This adds code to run subprocesses asynchronously and integrates with
the logger. stdout and stderr are logged in real time and always
captured. We return a CompletedProcess object like regular subprocess
does.

Relates: ansible-community#34
gotmax23 added a commit to gotmax23/antsibull-core that referenced this issue Mar 21, 2023
This adds code to run subprocesses asynchronously and integrates with
the logger. stdout and stderr are logged in real time and always
captured. We return a CompletedProcess object like regular subprocess
does.

Relates: ansible-community#34
gotmax23 added a commit to gotmax23/antsibull-core that referenced this issue Mar 25, 2023
This adds code to run subprocesses asynchronously and integrates with
the logger. stdout and stderr are logged in real time and always
captured. We return a CompletedProcess object like regular subprocess
does.

Relates: ansible-community#34
gotmax23 added a commit to gotmax23/antsibull-core that referenced this issue Mar 25, 2023
This adds code to run subprocesses asynchronously and integrates with
the logger. stdout and stderr are logged in real time and always
captured. We return a CompletedProcess object like regular subprocess
does.

Relates: ansible-community#34
gotmax23 added a commit to gotmax23/antsibull-core that referenced this issue Mar 25, 2023
This adds code to run subprocesses asynchronously and integrates with
the logger. stdout and stderr are logged in real time and always
captured. We return a CompletedProcess object like regular subprocess
does.

Relates: ansible-community#34
felixfontein pushed a commit that referenced this issue Apr 5, 2023
* add subprocess_util module to replace sh

This adds code to run subprocesses asynchronously and integrates with
the logger. stdout and stderr are logged in real time and always
captured. We return a CompletedProcess object like regular subprocess
does.

Relates: #34

* subprocess_util: also support stdlib Logger

Both Logger classes have `.debug()`, `.info()`, etc. methods, so let's
explicitly declare support for both.

* subprocess_util: order arguments more logically

* add unit tests for subprocess_util
@gotmax23
Copy link
Collaborator Author

We have merged

  • 0388c0f - add subprocess_util module to replace sh
  • 39a4356 - venv: prepare for sh removal and add tests
  • 28f969c - replace remaining usage of sh with subprocess_util
  • 886bcc7 - Allow to handle UTF-8 decoding errors
  • d910be2 - Work around line length limit of asyncio.StreamReader

so we can close this, right?

@felixfontein
Copy link
Collaborator

Not really, antsibull-core still needs to be depending on sh until 3.0.0 (for backwards compatibility with certain older antsibull/antsibull-docs releases). I would keep this open until then.

@gotmax23 gotmax23 removed this from the antsibull-core v2 milestone Dec 1, 2023
@gotmax23 gotmax23 added this to the antsibull-core v3 milestone Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
antsibull-meta An issue that affects multiple antsibull projects dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants