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

run_command output to console if capture: false #13968

Conversation

julianneswinoga
Copy link

Currently long-running programs using run_command do not have any output until they have finished. This can give the indication that the configure step is (wrongfully) stuck. Having console output during a long-running run_command also aides in debugging.

This patch outputs stdout and stderr to console when capture: false, instead of previously just throwing it away (subprocess.DEVNULL).

From Popen docs:

With the default settings of None, no redirection will occur.

I also added a unit test, allplatformstests.py::test_run_command_output, to check this behavior using test cases/common/33 run program. I modified the test case a little to make the stderr output more explicit.

@eli-schwartz
Copy link
Member

Currently long-running programs using run_command do not have any output until they have finished. This can give the indication that the configure step is (wrongfully) stuck. Having console output during a long-running run_command also aides in debugging.

This patch outputs stdout and stderr to console when capture: false, instead of previously just throwing it away (subprocess.DEVNULL).

This doesn't handle long-running commands that do capture the stdout, then.

Maybe a better option is to log that we are running a command?

@julianneswinoga
Copy link
Author

Yeah, I think the better solution is to have an option to show output as it's running (like custom_target(console: true)) but I also felt adding in that support was a lot of change for my first MR here 🙂

Since the current implementation just throws away the output I felt that this was a good feature that doesn't change the existing functionality.

@julianneswinoga
Copy link
Author

julianneswinoga commented Dec 3, 2024

@eli-schwartz Would you prefer that I teach run_command about the console keyword rather than this MR?

@julianneswinoga
Copy link
Author

Closed in favor of #14104

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