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

Retry popen on 127 osx #306

Merged
merged 3 commits into from
Jan 27, 2022

Conversation

autoantwort
Copy link
Contributor

@autoantwort autoantwort commented Dec 20, 2021

Close microsoft/vcpkg#21905 bullet point 1.

Code 127 normally means command not found, but that does not really makes sense here. Sometimes (not often) unzip returns 127 and sometimes not, when you retry it the return code is 0. I have no idea what the problem could be, but this implements a workaround by retrying the command once on mac when the return code is 127.

@autoantwort autoantwort force-pushed the retry-popen-on-127-osx branch from ff3b271 to ccd77dc Compare December 20, 2021 19:50
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

There are some clear (minor) changes needed, but I want someone else to take a look and see if we want to do this.

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

nevermind; I am so annoyed that this is the best way to get a string representation of a thread ID.

@ras0219-msft
Copy link
Contributor

I merged #305 since it seems obviously positive. I am less convinced about arbitrarily retrying: child processes are not guaranteed to be idempotent.

If we want to implement a retry around "unzip" specifically on mac, I think that would be reasonable. It should not be implemented for all processes though.

Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

See above comment.

@autoantwort autoantwort force-pushed the retry-popen-on-127-osx branch from ccd77dc to 23535ef Compare January 10, 2022 03:32
@autoantwort autoantwort force-pushed the retry-popen-on-127-osx branch from a86c952 to ecacaec Compare January 10, 2022 03:50
@ras0219-msft ras0219-msft merged commit 23a5f63 into microsoft:main Jan 27, 2022
@ras0219-msft
Copy link
Contributor

Thanks, LGTM!

@autoantwort autoantwort deleted the retry-popen-on-127-osx branch February 5, 2022 18:44
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.

[vcpkg-ci] Current CI problems
3 participants