fix(platform-ios): fix `sourceDir` detection #1444

tido64 · 2021-07-12T13:45:43Z

Summary:

This is a minimal changes version of #1436. After speaking with @grabbou, I learned that most of the props in config are going away, so we agreed to make as little changes as possible and instead focus on next.

Resolves #1435
Resolves microsoft/react-native-test-app#375

Test Plan:

Curren tests should pass:

 PASS   unit  packages/platform-android/src/link/patches/__tests__/makeSettingsPatch.test.ts
 PASS   unit  packages/platform-ios/src/commands/runIOS/__tests__/findMatchingSimulator.test.ts
 PASS   unit  packages/platform-android/src/config/__tests__/getProjectConfig.test.ts
 PASS   unit  packages/cli/src/commands/init/__tests__/template.test.ts
 PASS   unit  packages/cli/src/tools/__tests__/packageManager-test.ts
 PASS   unit  packages/platform-ios/src/link/__tests__/getHeaderSearchPath.test.ts
 PASS   unit  packages/cli/src/commands/link/__tests__/link.test.ts
 PASS   unit  packages/platform-android/src/commands/runAndroid/__tests__/runOnAllDevices.test.ts
 PASS   unit  packages/cli/src/tools/windows/__tests__/androidWinHelpers.test.ts
 PASS   unit  packages/cli/src/commands/upgrade/__tests__/upgrade.test.ts (5.274 s)
 PASS   unit  packages/cli/src/commands/bundle/__tests__/getAssetDestPathAndroid-test.ts
 PASS   unit  packages/platform-ios/src/config/__tests__/findProject.test.ts
 PASS   unit  packages/cli/src/commands/init/__tests__/editTemplate.test.ts (6.21 s)
 PASS   unit  packages/platform-android/src/config/__tests__/findPackageClassName.test.ts
 PASS   unit  packages/platform-android/src/link/patches/__tests__/makeBuildPatch.test.ts
 PASS   unit  packages/platform-ios/src/commands/runIOS/__tests__/parseXctraceIOSDevicesList.test.ts
 PASS   unit  packages/platform-ios/src/commands/runIOS/__tests__/parseIOSDevicesList.test.ts
 PASS   unit  packages/platform-ios/src/link-pods/__tests__/removePodEntry.test.ts
 PASS   unit  packages/platform-ios/src/link-pods/__tests__/isInstalled.test.ts
 PASS   unit  packages/platform-ios/src/link/__tests__/writePlist.test.ts
 PASS   unit  packages/cli/src/tools/__tests__/copyFiles.test.ts
 PASS   unit  packages/platform-ios/src/link/__tests__/createGroup.test.ts
 PASS   unit  packages/platform-android/src/config/__tests__/getDependencyConfig.test.ts
 PASS   unit  packages/platform-ios/src/link-pods/__tests__/findLineToAddPod.test.ts
 PASS   unit  packages/platform-ios/src/config/__tests__/findPodspec.test.ts
 PASS   unit  packages/cli/src/commands/doctor/healthchecks/__tests__/androidHomeEnvVariable.test.ts
 PASS   unit  packages/cli/src/commands/bundle/__tests__/getAssetDestPathIOS-test.ts
 PASS   unit  packages/platform-ios/src/link/__tests__/addSharedLibraries.test.ts
 PASS   unit  packages/platform-ios/src/commands/runIOS/__tests__/findXcodeProject.test.ts
 PASS   unit  packages/platform-ios/src/link/__tests__/getGroup.test.ts
 PASS   unit  packages/platform-ios/src/config/__tests__/getProjectConfig.test.ts
 PASS   unit  packages/cli/src/commands/bundle/__tests__/filterPlatformAssetScales-test.ts
 PASS   unit  packages/platform-ios/src/link/__tests__/isInstalled.test.ts
 PASS   unit  packages/platform-ios/src/link/__tests__/removeProjectFromProject.test.ts
 PASS   unit  packages/platform-ios/src/link/__tests__/removeSharedLibrary.test.ts
 PASS   unit  packages/cli/src/tools/config/__tests__/findAssets-test.ts
 PASS   unit  packages/cli/src/tools/config/__tests__/index-test.ts (12.208 s)
 PASS   unit  packages/platform-ios/src/link-pods/__tests__/findMarkedLinesInPodfile.test.ts
 PASS   unit  packages/platform-ios/src/link/__tests__/getHeadersInFolder.test.ts
 PASS   unit  packages/platform-ios/src/link/__tests__/removeProjectFromLibraries.test.ts
 PASS   unit  packages/platform-ios/src/link/__tests__/mapHeaderSearchPaths.test.ts
 PASS   unit  packages/platform-ios/src/link-pods/__tests__/findPodTargetLine.test.ts
 PASS   unit  packages/platform-android/src/link/__tests__/isInstalled.test.ts
 PASS   unit  packages/platform-android/src/config/__tests__/readManifest.test.ts
 PASS   unit  packages/platform-android/src/link/patches/__tests__/makePackagePatch.test.ts
 PASS   unit  packages/cli/src/commands/init/__tests__/validate.test.ts
 PASS   unit  packages/platform-ios/src/link/__tests__/hasLibraryImported.test.ts
 PASS   unit  packages/cli/src/tools/config/__tests__/findProjectRoot-test.ts
 PASS   unit  packages/cli/src/commands/doctor/healthchecks/__tests__/androidStudio.test.ts (10.519 s)
 PASS   unit  packages/platform-ios/src/link/__tests__/addProjectToLibraries.test.ts
 PASS   unit  packages/cli/src/commands/doctor/healthchecks/__tests__/jdk.test.ts (13.899 s)
 PASS   unit  packages/cli/src/commands/doctor/healthchecks/__tests__/androidSDK.test.ts (13.944 s)
 PASS   unit  packages/platform-ios/src/link/__tests__/addFileToProject.test.ts
 PASS   unit  packages/platform-android/src/config/__tests__/findAndroidDir.test.ts
 PASS   unit  packages/platform-android/src/config/__tests__/findManifest.test.ts
 PASS   unit  packages/platform-android/src/link/patches/__tests__/makeImportPatch.test.ts
 PASS   unit  packages/platform-android/src/link/patches/__tests__/makeStringsPatch.test.ts
 PASS   unit  packages/platform-ios/src/link-pods/__tests__/getDependenciesFromPodfileLock.test.ts
 PASS   unit  packages/platform-ios/src/link/__tests__/getTargets.test.ts
 PASS   unit  packages/cli/src/commands/doctor/healthchecks/__tests__/androidNDK.test.ts (11.249 s)
 PASS   unit  packages/platform-ios/src/link/__tests__/getBuildProperty.test.ts
 PASS   unit  packages/platform-ios/src/link/__tests__/getPlistPath.test.ts
 PASS   unit  packages/cli/src/tools/config/__tests__/findDependencies-test.ts
 PASS   unit  packages/platform-android/src/link/patches/__tests__/applyParams.test.ts
 PASS   unit  packages/tools/src/__tests__/groupFilesByType.test.ts
 PASS   unit  packages/platform-android/src/link/patches/__tests__/normalizeProjectName.test.ts
 PASS   unit  jest/__tests__/replaceProjectRootInOutput.test.ts
 PASS   unit  packages/platform-ios/src/config/__tests__/findPodfilePath.test.ts
 PASS   unit  packages/platform-ios/src/link/__tests__/getPlist.test.ts

 PASS   unit  packages/cli/src/tools/__tests__/loadMetroConfig-test.ts
 PASS   unit  packages/cli/src/commands/link/__tests__/makeHook.test.ts
1;
 PASS   e2e  __e2e__/unknown.test.ts
 PASS   e2e  __e2e__/default.test.ts
 PASS   e2e  __e2e__/uninstall.test.ts (11.808 s)
 PASS   unit  packages/cli/src/commands/info/__tests__/info.test.ts (8.752 s)
 PASS   e2e  __e2e__/install.test.ts (10.138 s)
 PASS   e2e  __e2e__/init.test.ts (30.857 s)
 PASS   e2e  __e2e__/config.test.ts (135.075 s)
 PASS   e2e  __e2e__/root.test.ts (138.565 s)

Test Suites: 2 skipped, 79 passed, 79 of 81 total
Tests:       10 skipped, 2 todo, 287 passed, 299 total
Snapshots:   28 passed, 28 total
Time:        147.844 s
Ran all test suites in 2 projects.

See also react-native-community#1054 and react-native-community#1436. Resolves react-native-community#1435.

thymikee · 2021-07-15T14:27:55Z

@grabbou wdyt?

kelset · 2021-08-03T13:10:05Z

@thymikee is this blocked on @grabbou, or could we get it in? It's been open for a while 😓

thymikee · 2021-08-03T13:23:30Z

I didn't have the time to properly review this yet, so would still like @grabbou or @Esemesek to take a look

grabbou · 2021-08-04T21:21:19Z

Apologies for the waiting time. The reason I didn't act on this PR earlier was that I was thinking how to move forward with this one. Like @tido64 said, all the logic around sourceDir and projectPath is going away in the nearest future, as they are not needed for the autolinking. We only care about a Podfile.

That said, I am hesitant to add additional complexity to something that already works, as well as change the way we resolve a Podfile - at the risk of breaking this for our other users.

Instead, I would work on adding a temporary configuration option to override what does not work. For example, we could allow overriding sourceDir or a podfile in case it didn't work out of the box.

grabbou · 2021-08-04T21:22:26Z

packages/platform-ios/src/config/index.ts

+  // This is a temporary fix for #1435. In certain repos, the Xcode project can
+  // be generated by a tool. The only file that we can assume to exist on disk
+  // is `Podfile`.
+  const sourceDir = podfile ? path.dirname(podfile) : path.dirname(projectPath);


I would not change this one to be honest, sourceDir is not needed for autolinking, so there's no need to adjust it for cases when xcodeproj doesn't exist. It is needed for legacy link command and in the case of a missing xcodeproj, that would fail anyway.

It looks like it is being used for autolinking:

cli/packages/platform-ios/native_modules.rb

Line 40 in 44f5413

project_root = Pathname.new(config["project"]["ios"]["sourceDir"])

Without this change, we hit errors similar to this when running pod install:

[!] No podspec found for `ReactTestApp-DevSupport` in `..`

You’re right 😅

tido64 · 2021-08-04T21:51:57Z

That said, I am hesitant to add additional complexity to something that already works, as well as change the way we resolve a Podfile - at the risk of breaking this for our other users.

But it's not fundamentally changing how you're resolving a Podfile today. The logic is still the same for the success case. Only in the case of it failing, are we resorting to a glob search. I don't understand how that is a risk? If it was failing before, it wouldn't be any worse with the current change.

grabbou · 2021-08-05T04:34:00Z

Reopening as it looks like that’s our only way to go in this case for time being.

grabbou · 2021-08-05T04:36:24Z

After taking another pass and your explanations @tido64, it looks good 👍

* fix(platform-ios): fix `sourceDir` detection See also #1054 and #1436. Resolves #1435. * yarn lint --fix

thymikee · 2021-08-05T17:47:19Z

@tido64 published @react-native-community/[email protected] with this fix as well. Thanks!

tido64 · 2021-08-06T21:08:50Z

Thanks for backporting it, @thymikee. ~~Did it get published? I don't see it in the feed yet: https://www.npmjs.com/package/@react-native-community/cli?activeTab=versions~~ Never mind! I was looking at the wrong package 😛

joshchoo · 2021-10-08T02:10:00Z

Hi @tido64,

I'm facing a problem with podspec path resolution on one of the packages I'm using: react-native-emarsys-wrapper.
The package doesn't use auto-linking, and it defines a sample Podfile in sample/ios/Podfile.

This change causes sourceDir to resolve to sample/ios instead of ios because of the location of the sample Podfile. Consequently, native_modules.rb can't find the podspec file and logs the warning "use_native_modules! skipped the react-native dependency react-native-emarsys-wrapper. No podspec file was found".

I'm currently working around this by patching react-native-emarsys-wrapper to remove sample/ios/Podfile to avoid incorrect sourceDir resolution. Is there a better way to resolve sourceDir, such as editing react-native.config.js, to fix it? Or should I wait for this temporary fix to be removed?

Environment

React Native: 0.66.0

tido64 · 2021-10-08T09:52:50Z

@joshchoo: You can explicitly disable it in your react-native.config.js: https://github.com/react-native-community/cli/blob/master/docs/autolinking.md#how-can-i-disable-autolinking-for-unsupported-library

This way you don't have to patch the package.

Is the sample folder an essential part of the package? Otherwise, I would exclude it from the published package. As far as I know, the "next" version that will make this temporary fix unnecessary is being worked on, but I don't have any other information.

joshchoo · 2021-10-08T10:12:46Z

@tido64, thanks for your help! Disabling autolinking via react-native.config.js works for my case.

fix(platform-ios): fix sourceDir detection

a86abe3

See also react-native-community#1054 and react-native-community#1436. Resolves react-native-community#1435.

tido64 requested review from Esemesek, grabbou and thymikee as code owners July 12, 2021 13:45

yarn lint --fix

3eae16f

grabbou mentioned this pull request Jul 15, 2021

feat: look for Podfile in the source directory too #1445

Closed

tido64 mentioned this pull request Jul 21, 2021

refactor: Use react-native-test-app to manage our test app react-native-menu/menu#142

Merged

grabbou reviewed Aug 4, 2021

View reviewed changes

grabbou closed this Aug 4, 2021

grabbou reopened this Aug 4, 2021

grabbou closed this Aug 4, 2021

tido64 mentioned this pull request Aug 4, 2021

feat(cli-patform-ios): allow setting a path to a Podfile #1459

Closed

grabbou reopened this Aug 5, 2021

grabbou approved these changes Aug 5, 2021

View reviewed changes

grabbou merged commit 59cfbd9 into react-native-community:master Aug 5, 2021

thymikee pushed a commit that referenced this pull request Aug 5, 2021

fix(platform-ios): fix sourceDir detection (#1444)

d5089ec

* fix(platform-ios): fix `sourceDir` detection See also #1054 and #1436. Resolves #1435. * yarn lint --fix

tido64 deleted the tido/fix-sourcedir-detection branch August 6, 2021 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(platform-ios): fix `sourceDir` detection #1444

fix(platform-ios): fix `sourceDir` detection #1444

Uh oh!

tido64 commented Jul 12, 2021 •

edited

Loading

Uh oh!

thymikee commented Jul 15, 2021

Uh oh!

kelset commented Aug 3, 2021

Uh oh!

thymikee commented Aug 3, 2021

Uh oh!

grabbou commented Aug 4, 2021

Uh oh!

grabbou Aug 4, 2021

Uh oh!

tido64 Aug 4, 2021

Uh oh!

grabbou Aug 5, 2021

Uh oh!

tido64 commented Aug 4, 2021

Uh oh!

grabbou commented Aug 5, 2021

Uh oh!

grabbou commented Aug 5, 2021

Uh oh!

thymikee commented Aug 5, 2021

Uh oh!

tido64 commented Aug 6, 2021 •

edited

Loading

Uh oh!

joshchoo commented Oct 8, 2021

Uh oh!

tido64 commented Oct 8, 2021

Uh oh!

joshchoo commented Oct 8, 2021

Uh oh!

Uh oh!

fix(platform-ios): fix sourceDir detection #1444

fix(platform-ios): fix sourceDir detection #1444

Uh oh!

Conversation

tido64 commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Test Plan:

Uh oh!

thymikee commented Jul 15, 2021

Uh oh!

kelset commented Aug 3, 2021

Uh oh!

thymikee commented Aug 3, 2021

Uh oh!

grabbou commented Aug 4, 2021

Uh oh!

grabbou Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

tido64 Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

grabbou Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

tido64 commented Aug 4, 2021

Uh oh!

grabbou commented Aug 5, 2021

Uh oh!

grabbou commented Aug 5, 2021

Uh oh!

thymikee commented Aug 5, 2021

Uh oh!

tido64 commented Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshchoo commented Oct 8, 2021

Uh oh!

tido64 commented Oct 8, 2021

Uh oh!

joshchoo commented Oct 8, 2021

Uh oh!

Uh oh!

fix(platform-ios): fix `sourceDir` detection #1444

fix(platform-ios): fix `sourceDir` detection #1444

tido64 commented Jul 12, 2021 •

edited

Loading

tido64 commented Aug 6, 2021 •

edited

Loading