-
Notifications
You must be signed in to change notification settings - Fork 933
fix(macCatalyst): assume a provided UDID is valid #2642
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
Conversation
|
The CI failure seems unrelated to this PR |
cipolleschi
left a comment
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.
It makes sense to me.
|
gentle ping on this one, it's the last patch I'm carrying around in my build demo :-) |
|
@thymikee Still the last patch I'm carrying on the react-native-firebase build demo and has an approval :-) - curious if it can merge? Only risk is if it is a macCatalyst build (already small user population!) and they specify a machine UDID (advanced usage, super small user population) then if the specified UDID doesn't exist the app start will fail. But who would specify a non-existent UDID and not be paying attention to see the failure and understand it? Seems like an easy win...and allows macCatalyst app starts to actually start... Cheers |
thymikee
left a comment
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.
catalyst is only handled in runOnDevice. Can we scope it to that code branch below?
8822b4a to
f702812
Compare
|
Hey @thymikee - thanks for the review. I altered the patch and re-pushed (after rebasing to current main) based on your feedback and confirmed it still works in my demo where I exercise simulator debug+release and macCatalyst https://github.com/mikehardy/rnfbdemo/commits/main/patches approach now is to nullish-ly check for device?.type in the main simulator pathway - which should be semantically equivalent to it's unpatched check in effect ...only then do we fall in to the non-simulator branch, and only there do we see if device was undefined and then emit our warning but do the new "trust the user vs returning error" stance and just execute it |
| `Could not find a device with udid: "${chalk.bold( | ||
| args.udid, | ||
| )}". ${printFoundDevices(devices)}`, |
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.
Can we still print this?
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.
absolutely - thanks again for the review - I think I focused in too much on the macCatalyst case and while I was still emitting a similar warning below, it was without the devices list which I think is really helpful in the general case.
So I put that back and re-pushed. What do you think?
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.
@thymikee re-pushed, hopefully addressing your concern. Cheers 👋
f702812 to
96a21d3
Compare
works around a problem where macCatalyst UDIDs are not correct when fetched from system, so attempting to find them in list of UDIDs always fails and macCatalyst apps will not start
96a21d3 to
569f0e4
Compare
| return logger.error( | ||
| // We may be able to recover from this for macCatalyst app starts, | ||
| // but the general case is this is a problem and device list will help | ||
| logger.warn( |
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.
no return, and warn not error now as we may recover from this for macCatalyst with this patch
but still print the device list because that should help the general case where this was an incorrectly specified UDID
thymikee
left a comment
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.
Thank you!
Summary
This patch works around a problem where macCatalyst UDIDs are not correct when fetched from system, so attempting to find them in list of UDIDs always fails and macCatalyst apps will not start
It basically just assumes - but with an informative warning - that if you specify a UDID you know what you're doing and it will try to use the one you specified instead of simply erroring out.
Test Plan
I've been carrying this patch around forever. It's the only way I can get my build demonstrator for react-native-firebase to correctly build-test and run macCatalyst version of react-native + react-native-firebase
Should be noted this very same build test is the source of a large number of macCatalyst build fixes over the years, it's been quite helpful in upstreaming things to make macCatalyst work, this is just another one
https://github.com/mikehardy/rnfbdemo/blob/0f16f47dc9823a592673d849d981db1fb8a96bf9/patches/%40react-native-community%2Bcli-platform-apple%2B15.0.1.patch#L1-L24
Checklist
react-nativecheckout (instructions).