-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Make iOS update commands run in sequence instead of all at once #2024
Conversation
.github/actions/bumpVersion/index.js
Outdated
exec(`/usr/libexec/PlistBuddy -c "Set :CFBundleShortVersionString ${shortVersion}" ${PLIST_PATH}`), | ||
exec(`/usr/libexec/PlistBuddy -c "Set :CFBundleShortVersionString ${shortVersion}" ${PLIST_PATH_TEST}`), | ||
exec(`/usr/libexec/PlistBuddy -c "Set :CFBundleVersion ${cfVersion}" ${PLIST_PATH}`), | ||
exec(`/usr/libexec/PlistBuddy -c "Set :CFBundleVersion ${cfVersion}" ${PLIST_PATH_TEST}`), | ||
] | ||
.reduce((previousPromise, currentPromise) => previousPromise.then(() => currentPromise()), Promise.resolve()); |
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.
Cool! This is sure ugly code 🧌 It would be cool to make this into a utility at some point that we can use whenever we need it. Seems like it should work well for you in this case!
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.
Another possible cure for this would be to use execSync()
: https://nodejs.org/api/child_process.html#child_process_child_process_execsync_command_options so that they run synchronously.
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.
Oh, and also... I think exec()
in the array is already starting all the promises all at once. The code you added just makes sure that they are resolved one at a time... So to really get what you want, I think you need to do something more like this:
const updateCommands = [
`/usr/libexec/PlistBuddy -c "Set :CFBundleShortVersionString ${shortVersion}" ${PLIST_PATH}`,
`/usr/libexec/PlistBuddy -c "Set :CFBundleShortVersionString ${shortVersion}" ${PLIST_PATH_TEST}`,
`/usr/libexec/PlistBuddy -c "Set :CFBundleVersion ${cfVersion}" ${PLIST_PATH}`,
`/usr/libexec/PlistBuddy -c "Set :CFBundleVersion ${cfVersion}" ${PLIST_PATH_TEST}`
]
.reduce((previousPromise, command) => previousPromise.then(() => exec(command)), Promise.resolve());;
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.
Ahhh yeah execSync
seems like a much better choice for my use case! Thanks!
Updated! |
@ctkochan22 All you! |
cc @tgolen because I reused a trick I saw you used in
migrateOnyx
to get these promises to run one after another instead of all at the same time (although in my case the order doesn't matter).Details
My theory is that this workflow failed because we have multiple asynchronous commands attempting to read from and write to the same file at the same time. Ideas for how to verify that theory are welcome.
Fixed Issues
Fixes failed deploys
Tests
Not tested locally.