run-ios bug: Try/catch removed from warnAboutManuallyLinkedLibs.ts causing exceptions to propagate/crash

[Saadnajmi]

Environment

System:
  OS: macOS 11.4
  CPU: (8) x64 Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz
  Memory: 921.95 MB / 16.00 GB
  Shell: 3.2.57 - /bin/bash
Binaries:
  Node: 16.4.0 - /usr/local/bin/node
  Yarn: 1.22.10 - /usr/local/bin/yarn
  npm: 7.18.1 - /usr/local/bin/npm
  Watchman: 4.9.0 - /usr/local/bin/watchman
Managers:
  CocoaPods: 1.10.1 - /usr/local/bin/pod
SDKs:
  iOS SDK:
    Platforms: iOS 14.5, DriverKit 20.4, macOS 11.3, tvOS 14.5, watchOS 7.4
  Android SDK:
    API Levels: 28, 29, 30
    Build Tools: 28.0.3, 29.0.3, 30.0.1
    System Images: android-30 | Google APIs Intel x86 Atom
    Android NDK: Not Found
IDEs:
  Android Studio: 4.1.2 4.1.2
  Xcode: 12.5.1/12E507 - /usr/bin/xcodebuild
Languages:
  Java: 11.0.1 - /usr/bin/javac
npmPackages:
  @react-native-community/cli: Not Found
  react: 17.0.1 => 17.0.1 
  react-native: 0.64.1 => 0.64.1 
  react-native-macos: ^0.63.0 => 0.63.36 
npmGlobalPackages:
  *react-native*: Not Found

Description

This refers to microsoft/react-native-test-app#375

I was working on moving a community module to use React Native Test App to manage the test app of a community module (https://github.com/react-native-menu/menu), and ran into an error running run-ios. After investigation, it seems the issue happened upstream beginning in #1270 .

A try-catch was removed from packages/platform-ios/src/link/warnAboutManuallyLinkedLibs.ts, causing the exception to bubble up and crash. This seems like an error, because an Android fix shouldn't have broken something downstream for iOS.

Reproducible Demo

A little bit contrived since I reproed this using a downstream module, but:

git clone git@github.com:react-native-menu/menu.git
cd menu
mv example/ example2/ # This is to just move the existing test app out of the way temporarily
yarn add react-native-test-app --dev
yarn init-test-app
# The options I chose
# ✔ What is the name of your test app? … MenuExample
# ✔ Which platforms do you need test apps for? › Android, iOS
# ✔ Where should we create the new project? … example
yarn
yarn build:ios
pod install --project-directory=ios
yarn ios

[tido64]

I have a draft PR with a proposed solution ☝️

If you can, please give it a try and let me know how it goes.

cc @grabbou, @thymikee

[grabbou]

For this particular thing to work, we need to update the config to properly detect a Podfile.

It is a mix of features and bugs, I will try to be as explicit as possible, the list of changes is really minimal.

---

First of all, we need to set a proper path to a project. Since .xcodeproj doesn't live within the project directory, but inside node_modules, we need to update the configuration to reflect that.

Luckily, this can be done right now with a configuration:

module.exports = {
  project: {
    ios: {
      project: "node_modules/react-native-test-app/ios/ReactTestApp.xcodeproj",
    },
  },
};

When we do it, we can see the output of react-native config looks as follows:

  "ios": {
      "sourceDir": "/Users/grabbou/Repositories/menu/example/node_modules/react-native-test-app/ios",
      "folder": "/Users/grabbou/Repositories/menu/example",
      "pbxprojPath": "/Users/grabbou/Repositories/menu/example/node_modules/react-native-test-app/ios/ReactTestApp.xcodeproj/project.pbxproj",
      "podfile": null,
      "podspecPath": null,
      "projectPath": "/Users/grabbou/Repositories/menu/example/node_modules/react-native-test-app/ios/ReactTestApp.xcodeproj",
      "projectName": "ReactTestApp.xcodeproj",
      "libraryFolder": "Libraries",
      "sharedLibraries": [],
      "plist": [],
      "scriptPhases": []
    },

As you can see, sourceDir points to a folder within node_modules, where the project is located. It is not a surprise, since it is just a path.dirname on a supplied project value.

However, podfile, and podspecPath are null, this is because we look for them within sourceDir. I believe this is a right default heuristic.

Rather than fighting with direction mechanisms and globs, I would just let people set a path to a podfile.

In other words, make it possible to pass the "podfile" as a part of config:

module.exports = {
  project: {
    ios: {
      project: "node_modules/react-native-test-app/ios/ReactTestApp.xcodeproj",
      podfile: "ios/Podfile"
    },
  },
};

which will be useful in the future anyway.

This is needed to make autolinking to work.

[tido64]

Thanks for the detailed explanation, @grabbou. There is one thing I don't quite understand though. It sounds like the only piece of information that we need from the user is where the Podfile is. .xcworkspace is always generated next to it, so we can derive both sourceDir and project. What do we need everything else for? It doesn't look like we need the .xcodeproj for anything. Even if we do need it, we should be getting the path from .xcworkspace. Assuming that .xcodeproj is always next to Podfile is just wrong. It could be anywhere as it can be shared between multiple projects.

[grabbou]

What do we need everything else for?

We don't need it, it's been used for legacy link. That's why I don't want to derive anything from podfile location, since in the next version that we should get started on:

It sounds like the only piece of information that we need from the user is where the Podfile is

As you have noticed yourself - it will become the only thing (well, except for maybe scriptPhases) that is needed.

---

I agree that we should not assume that these exist next to each other, but again, the default values were meant to satisfy the most popular setup (aka the default one). This needs a general clean up, so I feel like adding an override is kinda safe when defaults are broken, while we work on the new thing.

[grabbou]

If you look at the next branch

https://github.com/react-native-community/cli/blob/next/packages/platform-ios/src/config/index.ts#L41-L45

only three properties are used. I didn't remove the project check back then, but we can safely replace that with a podfile (like in your PR).

[grabbou]

For context, we used to rely on an .xcodeproj. Because projects were all different, we started with a detection mechanism (glob) to eliminate naming collisions when people would use something different than iOS, at a different level too.

In retrospection, I don't think we necessarily need to support non-standard locations. If someone's using something different than iOS or is having a non-standard project structure, overrides should be the way to go, instead of a complex heuristics, like we sometimes have.

When CocoaPods support arrived, it was first optional, then became the default. Since it was "by default" next to xcodeproj, we used the sourceDir, which is derived from an xcodeproj. Almost from the very beginning, the presence of a project was dictating that it was a valid React Native project. As it can be observed in this thread, it is no longer the case and that is yet another reason to upgrade this code.

[tido64]

Thanks for the context. It makes total sense given the history. Is manual linking still used with CLI version 5.x and 6.x?

[grabbou]

Update this is being addressed in 596d849#diff-324776bfd410a79f7aaa6bc69a310a8515358fe9abfb81c69ae3744ca784c9fcL45 where I have made Podfile detection the source of truth.