react-native-community / cli

The React Native Community CLI - command line tools to help you build RN apps
MIT License
2.41k stars 905 forks source link

fix(macCatalyst): construct correct path for executable #2510

Open mikehardy opened 2 months ago

mikehardy commented 2 months ago

Summary:

it appears targetBuildDir var now has -maccatalyst in it before it gets to this method, so no need to add it again

This was tweaked in #2312 but appears to have regressed again - are macCatalyst builds checked in CI πŸ€” ?

Without this change, the path is incorrect (note the duplicate -maccatalyst in path):


Error: spawn /Users/mike/Library/Developer/Xcode/DerivedData/rnfbdemo-gsuyjvwnbefbzvbfqtovnhthepew/Build/Products/Debug-maccatalyst-maccatalyst/rnfbdemo.app/Contents/MacOS/rnfbdemo ENOENT
    at ChildProcess._handle.onexit (node:internal/child_process:284:19)
    at onErrorNT (node:internal/child_process:477:16)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
Emitted 'error' event on ChildProcess instance at:
    at ChildProcess._handle.onexit (node:internal/child_process:290:12)
    at onErrorNT (node:internal/child_process:477:16)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
  errno: -2,
  code: 'ENOENT',
  syscall: 'spawn /Users/mike/Library/Developer/Xcode/DerivedData/rnfbdemo-gsuyjvwnbefbzvbfqtovnhthepew/Build/Products/Debug-maccatalyst-maccatalyst/rnfbdemo.app/Contents/MacOS/rnfbdemo',
  path: '/Users/mike/Library/Developer/Xcode/DerivedData/rnfbdemo-gsuyjvwnbefbzvbfqtovnhthepew/Build/Products/Debug-maccatalyst-maccatalyst/rnfbdemo.app/Contents/MacOS/rnfbdemo',
  spawnargs: []
}

Highlight:

/Users/mike/Library/Developer/Xcode/DerivedData/rnfbdemo-gsuyjvwnbefbzvbfqtovnhthepew/Build/Products/Debug-maccatalyst-maccatalyst/rnfbdemo.app/Contents/MacOS/rnfbdemo

Test Plan:

I build and test with macCatalyst all the time: https://github.com/mikehardy/rnfbdemo/blob/main/make-demo.sh

I added a unit test that exercises the method, sending it old (without -maccatalyst) and new (with -maccatalyst) inputs to verify it handles both, and verify I didn't regress the more common ios build code path

Checklist

mikehardy commented 2 months ago

Note: this was against CLI v14.1.0 - I'm currently making sure rn75 works This may be changed in rn76 RCs - I'll be checking those later

szymonrybczak commented 2 months ago

are macCatalyst builds checked in CI πŸ€” ?

Unfortunately no :(

Note: this was against CLI v14.1.0 - I'm currently making sure rn75 works This may be changed in rn76 RCs - I'll be checking those later

Maybe to make this solution more future-proof we'll fallback to other value if initial one fails? πŸ€”

mikehardy commented 2 months ago

I guess it is hard in CI, since you need an Xcode development team to do a macCatalyst build - it was pretty annoying to set up in my build test rig, at least πŸ˜…

Would you like me to alter the patch to actually check existence of the path so it can try both and return the one that works ?

szymonrybczak commented 2 months ago

I guess it is hard in CI, since you need an Xcode development team to do a macCatalyst build - it was pretty annoying to set up in my build test rig, at least πŸ˜…

Ah, right 😞

Would you like me to alter the patch to actually check existence of the path so it can try both and return the one that works ?

Yes, IMO that's the safest solution.

mikehardy commented 2 months ago

I added a unit test and handle both cases now, force-pushed, and while the idea is roughly the same code is now different so re-requesting review, thanks you all