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

Remove sh usages #122

Merged
merged 6 commits into from
Apr 18, 2023
Merged

Remove sh usages #122

merged 6 commits into from
Apr 18, 2023

Conversation

felixfontein
Copy link
Collaborator

Needs to be rebased/updated once #120 has been merged.

@felixfontein felixfontein force-pushed the remove-sh branch 2 times, most recently from 78df7b4 to 00d3816 Compare April 10, 2023 19:08
@felixfontein
Copy link
Collaborator Author

@gotmax23 the failing tests show a problem with ansible_core.subprocess_util.async_log_run: if a single line is too large of the output (in this case stdout), asyncio.streams dies:

[...]
  File "/opt/hostedtoolcache/Python/3.11.2/x64/lib/python3.11/site-packages/antsibull_core/subprocess_util.py", line 40, in _stream_log
    line = await stream.readline()
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.11.2/x64/lib/python3.11/asyncio/streams.py", line 554, in readline
    raise ValueError(e.args[0])
ValueError: Separator is found, but chunk is longer than limit

This requires a very large output, which apparently happens for ansible-doc --metadata-dump --no-fail-on-errors when the ansible package is installed. For the 8.0.0 build I did recently, the maximum line length I found was 68115.

According to the asyncio/streams.py in Python 3.10, the default limit is 65536 bytes for a line. This can be adjusted by passing limit=HIGHER_LIMIT to asyncio.create_subprocess_exec(), but IMO this is only a band-aid, since there is no value for "unlimited". We might have to implement line parsing ourselves?

@felixfontein
Copy link
Collaborator Author

I've tried it out in ansible-community/antsibull-core#53, it seems to work well.

@felixfontein felixfontein force-pushed the remove-sh branch 2 times, most recently from c005f1b to 5a387fd Compare April 13, 2023 04:28
@felixfontein
Copy link
Collaborator Author

Restarting tests now that ansible-community/antsibull-core#53 has been merged.

@felixfontein felixfontein reopened this Apr 14, 2023
@felixfontein felixfontein changed the title [WIP] Remove sh usages Remove sh usages Apr 14, 2023
@felixfontein felixfontein marked this pull request as ready for review April 14, 2023 05:07
@felixfontein felixfontein requested a review from gotmax23 April 14, 2023 06:19
@felixfontein
Copy link
Collaborator Author

(Rebased to remove conflict.)

@gotmax23
Copy link
Collaborator

Thanks for working on this! I'll take a closer look later today or tomorrow.

Copy link
Collaborator

@gotmax23 gotmax23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is pretty straightfoward; I just have a couple comments 😄.

Copy link
Collaborator

@gotmax23 gotmax23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack. Let's do this 🎉!

@gotmax23 gotmax23 merged commit 5bb7bc5 into ansible-community:main Apr 18, 2023
@felixfontein felixfontein deleted the remove-sh branch April 18, 2023 16:47
@felixfontein
Copy link
Collaborator Author

@gotmax23 thanks for preparing this and reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants