[expo-cli] changed error handling for ios upload to use fastlane exit code

[mymattcarroll]

Why

• closes expo upload:ios on github actions (using macos-latest) succeeds and fails! #2368

Test plan

follow the steps in #2368, this should fix the issue.

If I could get some help running this locally, that would be greatly appreciated.

[mymattcarroll]

@dsokal Any chance this could get a look? We would really like to get uploads consistently working again. Been a bit of a pain getting false negatives.

[brentvatne]

can you explain the logic behind these code changes? it’s hard to judge code without tests when there isn’t a strong justification for the precise code being changed

[mymattcarroll]

Of course 👍

Using JSON.parse() to determine if a FastLane command has failed or succeeded is always going to be brittle logic. The FastLane CLI could release a change that outputs a non-json string. This change could even come in a patch version as I highly doubt the maintainers of the CLI consider changes to the output logs to be breaking changes.

The changes proposed are to use the exit code that is returned by the FastLane command to check if the command failed or not. This is much more robust as the exit code changing would (in my opinion) be a breaking change to the CLI. This would be much easier to maintain on Expo's side as I am sure your team would be reluctant to change major versions of the FastLane CLI without considering the breaking changes first.

[brentvatne]

@mymattcarroll - ok that makes sense. i'm going to wait for @dsokal to come back from vacation to review this because he is the owner of this code

[dsokal]

@mymattcarroll sorry it's taking so long but I just came back from vacation.

Using JSON.parse() to determine if a FastLane command has failed or succeeded is always going to be brittle logic. The FastLane CLI could release a change that outputs a non-json string. This change could even come in a patch version as I highly doubt the maintainers of the CLI consider changes to the output logs to be breaking changes.

Actually, there is no way that "fastlane" commands could release a change that outputs a non-json string without our knowledge. The reason is simple - we're not using fastlane directly. We have our tiny wrapper called @expo/traveling-fastlane-{darwin,linux} which has fastlane and the appropriate ruby version bundled inside. The traveling-fastlane commands (scripts) always return JSON objects. An exception from this rule is when there is some bug in the wrapper lib. Under the hood, traveling-fastlane is executing actual fastlane commands, and those commands can potentially print something to stderr but this is not expected behavior and often means that we haven't handled some case. The root cause of this issue seems to be related to the fact that we're relying on the JSON printed to stderr. If there is some case which we haven't handled - some fastlane log can find its way to stderr and we're unable to parse it in expo-cli hence issues similar to this one.

Unfortunately, from the PR description, I can't deduce what bug you're trying to fix here. Could you please elaborate? I'd be grateful if you could take a look at the abovementioned wrapper's code - https://github.com/expo/expo-cli/tree/master/packages/traveling-fastlane and debug the problem there without touching the expo-cli code. I believe, whatever the bug is, it should be possible to fix it there.

To sum up, I'd prefer not to merge this PR in the current form.

[mhoran]

At least in my case, and in the case of #2368, the following message is being printed to stdout, causing the JSON.parse to fail:

`skip_waiting_for_build_processing` used and no `changelog` supplied - skipping waiting for build processing

This behavior seems to have changed in a "recent" version of fastlane, hence comments that "reverting back to expo-cli@3.17.24 fixes the issue."

It doesn't look like any expo-cli code sets the changelog, so this makes sense. I guess the message either needs to be ignored, or a changelog needs to be set.

[tadhglewis]

any updates on this one?

[mymattcarroll]

My apologies, I have also been on vacation recently.

Actually, there is no way that "fastlane" commands could release a change that outputs a non-json string without our knowledge.

While fastlane itself may not have changed, a non-json string has been outputted from a CLI command that is being used in this code base programmatically. Relying on a json string as output from a CLI command (whether the command is code you personally wrote or fastlane itself) is always going to be brittle logic.

I would be more than happy to look into the wrapper and work out which part is logging a non-json string, but I will not have time in the coming weeks.

In regards to the issue that needs to be fixed: #2530 (comment) is the same issue that we are having.

This issue (which seems to be happening to more and more people) also explains the issue in quite alot of detail: #2368

[EvanBacon]

expo upload:ios has been deprecated in favor of eas submit -p ios. We've also removed fastlane from expo-cli due to the unstable nature of shipping ruby packages in a node module.

more info: https://github.com/expo/fyi/blob/master/expo-upload-ios.md