Skip to content

[utils] Make update-checkout output more readable #41714

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

Merged
merged 5 commits into from
Mar 9, 2022

Conversation

lorentey
Copy link
Member

@lorentey lorentey commented Mar 8, 2022

In Python 3, subprocess output is read as binary data by default, which isn’t what we want.

Instead of reading process output as byte strings, then manually decoding them into strings, simply pass text=True to functions in the subprocess module, so that we get properly decoded strings right out the box.

This fixes places that forget to do the decoding step — most especially, the update-checkout script. That script prints Git output as byte strings, which leads to unreadable results.

Additionally, in shell.run, use the same pipe for capturing both stdout and stderr. The distinction is pretty pointless in this use case; however, keeping the two channels separate means that we lose the original ordering of printed messages, which does matter.

In Python 3, subprocess output is read as binary data by default, which isn’t what we want.

Instead of reading process output as byte strings, then manually decoding them into strings, simply pass `text=True` to functions in the `subprocess` module, so that we get properly decoded strings right out the box.

This fixes places that forget to do the decoding step — most especially, the `update-checkout` script. That script prints Git output as byte strings, which leads to unreadable results.

Additionally, in shell.run, use the same pipe for capturing both stdout and stderr. The distinction is pretty pointless in this use case; however, keeping the two channels separate means that we lose the original ordering of printed messages, which does matter.
@lorentey lorentey requested a review from shahmishal March 8, 2022 01:00
@lorentey
Copy link
Member Author

lorentey commented Mar 8, 2022

@swift-ci test

@lorentey
Copy link
Member Author

lorentey commented Mar 8, 2022

Ah, I didn't know Python 2.7 was still a thing on the Linux bots 😬

  File "/usr/lib/python2.7/subprocess.py", line 216, in check_output
    process = Popen(stdout=PIPE, *popenargs, **kwargs)
TypeError: __init__() got an unexpected keyword argument 'text'

@lorentey
Copy link
Member Author

lorentey commented Mar 8, 2022

@swift-ci test

1 similar comment
@lorentey
Copy link
Member Author

lorentey commented Mar 9, 2022

@swift-ci test

@lorentey
Copy link
Member Author

lorentey commented Mar 9, 2022

Before:

Updating '/Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/swift-installer-scripts'
/Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/swift-installer-scripts
b"Your branch is up to date with 'origin/main'.\n"b"Already on 'main'\n"
/Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/swift-installer-scripts

/Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/swift-installer-scripts
b'HEAD is now at cee3f49 Merge pull request #77 from apple/add-support-for-version-dir\n'

After:

Updating '/Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/swift-installer-scripts'
Updating '/Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/swift-installer-scripts'
/Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/swift-installer-scripts
Already on 'main'
Your branch is up to date with 'origin/main'.

/Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/swift-installer-scripts

/Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/swift-installer-scripts
HEAD is now at cee3f49 Merge pull request #77 from apple/add-support-for-version-dir

Stdout and stderr are still garbled; I think the process pool isn't opening new pipes for each process.

@lorentey
Copy link
Member Author

lorentey commented Mar 9, 2022

@swift-ci test

@lorentey lorentey force-pushed the fix-update-checkout-output branch from 407e0ba to a11b8d9 Compare March 9, 2022 03:26
@lorentey
Copy link
Member Author

lorentey commented Mar 9, 2022

@swift-ci test

1 similar comment
@lorentey
Copy link
Member Author

lorentey commented Mar 9, 2022

@swift-ci test

@lorentey lorentey force-pushed the fix-update-checkout-output branch from e182dce to 4f9cf1d Compare March 9, 2022 04:08
@lorentey
Copy link
Member Author

lorentey commented Mar 9, 2022

@swift-ci test

@lorentey lorentey changed the title [utils] Force subprocess output to be read in text mode [utils] Make update-checkout output more readable Mar 9, 2022
@lorentey
Copy link
Member Author

lorentey commented Mar 9, 2022

And now it looks like this:

[swift-installer-scripts]               Updating '/Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/swift-installer-scripts'
[swift-installer-scripts]               + git checkout main
[swift-installer-scripts]               Already on 'main'
[swift-installer-scripts]               Your branch is up to date with 'origin/main'.

Things are in order, and interleaved messages are neatly labeled with their corresponding repository name.

@lorentey lorentey merged commit 0c529b0 into swiftlang:main Mar 9, 2022
@lorentey lorentey deleted the fix-update-checkout-output branch March 9, 2022 07:53
@shahmishal
Copy link
Member

Thanks @lorentey!

@shahmishal
Copy link
Member

It would be also nice if we stop printing all env variable with command line output.

@lorentey
Copy link
Member Author

@shahmishal Fix at #41778

@AnthonyLatsis AnthonyLatsis added the update-checkout Area → utils: the `update-checkout` script label Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
update-checkout Area → utils: the `update-checkout` script
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants