fix(macCatalyst): construct correct path for executable

[mikehardy]

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

• Documentation is up to date to reflect these changes.
• Follows commit message convention described in CONTRIBUTING.md

[mikehardy]

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]

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]

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]

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]

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

[cipolleschi]

left a nit, but lgtm

[cipolleschi]

shouldn't this be if-else if?

[mikehardy]

it certainly could be, but...it seemed to me the three cases were all mutually exclusive so simple if overrides/clobbers were semantically identical ?

case 1 - we're the default platform == ios && isCatalyst == false: default case join targetBuildDir + executableFolderPath

case 2 - we're the rare platform == ios && isCatalyst == true: clobber with result of maybe adding -maccatalyst to targetBuildDir -- note that catalyst is an ios platform subset, it will not go in to the platform == macos conditional as true

case 3 - we're the platform == macos: clobber join of targetBuildDir + fullProductName

Each of the three cases is unambiguously identifiable and unique so I thought a simple set of ifs + clobbers worked?

[mikehardy]

I just pushed a tiny change in response to this, for future developers so less explanation + thought is required I hope

1- I altered the isCatalyst conditional to explicitly contain a platform === 'ios' clause as that is the assumption, so now it is more clear that the catalyst block will not ever collide with the macos block

2- I updated the comments to reflect the same, and the idea that the default case and the two override blocks are exclusive of one another

[cipolleschi]

Yes, they were equivalent. But with

if <cond1> {
}
if <cond2> {
}
if <cond3> {
}

All three conditions are always evaluated. With

if <cond1> {
} else if <cond2> {
} else if <cond3> {
}

cond2 is evaluated if and only if cond1 is false. cond3 is evaluated if and only if both cond1 and cond2 are false.

So, the if-elseif approach is slightly more efficient, and makes it clear that the conditions are mutually exclusive.

[mikehardy]

@cipolleschi lots of discussion and we are in agreement with each other :-) - the question is that with the mixed signal I am hearing of "if/elseif/elseif is slightly better" vs "but I have approved the if/if/if version" - do you want me to convert it and re-push or not?

I don't want to swirl endlessly on this - it's a relatively small fix after all, but I'll do what you like (because, it's an easy conversion from if/if/if/ to if/elseif/elsif too).

So just let me know but I won't change+repush unless you ask, so we don't have another approval cycle unless desired

Cheers man