-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Build System: update-checkout] avoid multiprocessing when n is 1 #78896
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
base: main
Are you sure you want to change the base?
Conversation
There's no need to fiddle with locks etc. when there's no actual parallelism going on.
@swift-ci please smoke test |
For context, this helped me work around an issue where I kept getting stuck waiting on a lock (for reasons I don't know). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine, but lack of testing worries me. I'm not proficient enough with Python to suggest a concrete testing strategy though.
Does update-checkout have tests? |
It does seem to have some tests here https://github.com/swiftlang/swift/tree/main/utils/update_checkout/tests |
The mock repo is configured with the [email protected] email address. If the user has globally configured git to sign commits this won't match the signature, preventing the tests from running properly.
@swift-ci please smoke test |
@MaxDesiatov Sorry about the delay. Added some single threaded versions of the existing tests. Let me know if you have any other comments. |
@swift-ci smoke test linux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please wait for a review from Eric Evan too. Feel free to ping them if the PR stalls.
@swift-ci smoke test linux |
There's no need to fiddle with locks etc. when there's no actual parallelism going on.