Fix Xcode 10 errors relating to third-party #21458

mmccartney · 2018-10-03T04:02:16Z

Fixes #20774

The new Xcode build system uses parallel execution to run build steps that don't have an obvious dependency. Our Xcode project was written with the assumption that the Install Third Party build step is run before compiling the third-party libraries. To address this issue, this PR adds dependency information to the project to teach Xcode that ios-install-third-party.sh is generating the files under third-party. With this additional information, Xcode correctly waits for ios-install-third-party.sh to finish before advancing to the compile step.

In addition to the Xcode project changes, I had to make some changes to the script ios-install-third-party.sh so that

it would always execute the ios-configure-glog.sh script regardless of how it was invoked
it would always install the libraries even if Xcode had partially created the tree or if a previous install was interrupted

Test Plan:

Under Xcode 9 and Xcode 10, delete the third-party tree and build the React project
Under Xcode 9 and Xcode 10, delete the third-party tree and build the RNTester project
Create and run a sample project (react-native init AwesomeProject && cd AwesomeProject && react-native run-ios)

Release Notes:

[IOS] [BUGFIX] [third-party] - Fix build issue using new Xcode 10.0 build system

kelset · 2018-10-03T16:47:40Z

Hey thanks a lot for this PR, I'm going to test it asap!

kelset · 2018-10-03T22:29:26Z

👋 @mmccartney - just tested your PR locally and I think it's a step in the right direction but I still got this error while trying to run RNTester -> :-1: Build input file cannot be found: '/XXX/react-native/third-party/double-conversion-1.1.6/src/diy-fp.cc'

EDIT: to provide more context, my local test was basically running ./scripts/test-manual-e2e.sh from the react-native folder on a machine running macOS Mojave and Xcode 10.

radex · 2018-10-04T12:37:33Z

I think the root of the problem is that Xcode 10 is trying to optimize build by parallelizing build steps, and it doesn't understand that the third-party install script is what creates files in the third-party. So it tries to go ahead with compile steps and fails.

I think the output files settings need to be used to fix this, but I haven't researched this fully yet ;)

radex · 2018-10-04T12:51:04Z

btw @mmccartney @kelset, I just did this:

and I'm 90% sure it works. I fully cleaned build artifacts, and ran a build from CLI, not Xcode (steps that would trigger the issue every time), and it builds successfully. Unless there's some weird non-determinism going on, this should be a good fix

EDIT: Not so fast, Xcode seems not to be OK with just a folder. Working on a proper fix…

mmccartney · 2018-10-04T13:37:50Z

Yeah, I thought there was some parallelization happening but wasn't sure. I know it works if you run the script after npm install but before react-native run-ios. How do people feel about a postinstall script run by npm install ? I've never written one but would be willing to figure it out if people are open to it.

kelset · 2018-10-04T13:45:43Z

Go for it, if it's a viable solution we'll merge it 👍

radex · 2018-10-04T13:51:15Z

If you go this route (and it makes sense — I think — to put a dependency management script in… dependency management :P), I'd recommend removing the "Install Third Party" build phase from the Xcode project, as it doesn't do anything anymore

ikesyo · 2018-10-04T14:14:04Z

For CocoaPods users that is not needed, so I'm not for the postinstall (useless time consumption).

mmccartney · 2018-10-04T15:29:05Z

For CocoaPods users that is not needed, so I'm not for the postinstall (useless time consumption).

@ikesyo Are you saying that CocoaPods users are able to build react-native without the third-party artifacts?

ikesyo · 2018-10-04T15:46:43Z

Because those dependencies are handled with https://github.com/facebook/react-native/tree/master/third-party-podspecs, third-party directory is not needed for CocoaPods users.

http://facebook.github.io/react-native/docs/integration-with-existing-apps#configuring-cocoapods-dependencies

  # Third party deps podspec link
  pod 'DoubleConversion', :podspec => '../node_modules/react-native/third-party-podspecs/DoubleConversion.podspec'
  pod 'glog', :podspec => '../node_modules/react-native/third-party-podspecs/glog.podspec'
  pod 'Folly', :podspec => '../node_modules/react-native/third-party-podspecs/Folly.podspec'

mmccartney · 2018-10-04T16:15:00Z

@radex it seems to me the Install Third Party step needs to be moved up to the third-party target. I think my fix to the shell script itself is still needed though... I'll give it a try

mmccartney · 2018-10-04T17:52:59Z

I didn't end up moving the Install Third Party build step up. Instead I found that the build system wants a list of all outputs from the step in order to do dependency analysis. So, for each third-party source file that is referenced within the Xcode project, I listed it in a new third-party.xcfilelist file which gives the build system the information it was missing before.

radex · 2018-10-05T07:15:11Z

React/React.xcodeproj/project.pbxproj

@@ -3,7 +3,7 @@
 	archiveVersion = 1;
 	classes = {
 	};
-	objectVersion = 46;
+	objectVersion = 51;


are you sure about this? isn't that Xcode 10-only-compatible mode?

radex · 2018-10-05T07:18:10Z

I didn't end up moving the Install Third Party build step up. Instead I found that the build system wants a list of all outputs from the step in order to do dependency analysis. So, for each third-party source file that is referenced within the Xcode project, I listed it in a new third-party.xcfilelist file which gives the build system the information it was missing before.

I tried doing that, but it doesn't work correctly. Try deleting the third-party folder. Xcode will, at the beginning of the build, create each folder in the xcfilelist, and the install-third-party script will skip the download, because it thinks that it's already downloaded. We'd have to modify it to check if there are any files in the folder tree.

Also: I think outputFileListPaths makes the xcodeproj incompatible with Xcode 9.4 😭 — but you'd have to check

kelset · 2018-10-05T08:11:25Z

@radko93 I remember you having multiple Xcode instances, would you mind testing this PR to verify that it would be (or not) a BC for 9.x versions of Xcode? 🤔

radko93 · 2018-10-05T11:17:03Z

I will test this soon. If anyone else wants to do it than me please do.

mmccartney · 2018-10-05T15:57:25Z

@radex if outputFileListPaths is not compatible with Xcode 9, I think that outputPaths is and I could move all those files into the project. I was just trying to use a medium that is easier to change when the third-party libraries change.

Unfortunately I don't have an Xcode 9 laptop setup right now. I could probably get my hands on one next week. Sorry I didn't think about supporting older Xcode.

Xcode will, at the beginning of the build, create each folder in the xcfilelist, and the install-third-party script will skip the download

I'm not seeing this behavior.

@radko93 It may be good to hold off on testing for now. I haven't found any documentation on exactly how to test this sort of thing and just discovered that npm run-script build-ios-e2e is failing with the following error (and others like it):

error: Multiple commands produce '/Users/mmccartney/react-native/RNTester/build/Build/Products/Release-iphonesimulator/libdouble-conversion.a':
1) Target 'double-conversion-tvOS' (project 'React') has a command with output '/Users/mmccartney/react-native/RNTester/build/Build/Products/Release-iphonesimulator/libdouble-conversion.a'
2) Target 'double-conversion' (project 'React') has a command with output '/Users/mmccartney/react-native/RNTester/build/Build/Products/Release-iphonesimulator/libdouble-conversion.a'

(of course, the same command fails on the master branch due to missing third-party 😆 )

radex · 2018-10-06T12:57:27Z

if outputFileListPaths is not compatible with Xcode 9, I think that outputPaths is and I could move all those files into the project.

You can do that, but it will be more difficult to automate when third-party libraries get updated. You could very easily make a script that writes an .xcfilelist file, but modifying an Xcode project file requires extra dependencies that I don't know if react-native already includes in its tooling

mmccartney · 2018-10-06T15:58:17Z

You can do that, but it will be more difficult to automate when third-party libraries get updated

Yeah but honestly there are already so many file references inside the Xcode project that I don't think it matters. Remember, I simply used all the references already in project.pbxproj to build the list. There are a few other source references (e.g. #include <folly/File.h>) that I didn't bother including.

I'll go ahead and move them into outputPaths and revert the objectVersion changes (Xcode did that, not me 😉 ).

Can someone comment on whether npm run-script build-ios-e2e is supposed to work? It obviously doesn't work on master but these tvOS vs iOS errors aren't making sense to me and make me wonder if it's unrelated...

radex · 2018-10-06T16:36:56Z

Can someone comment on whether npm run-script build-ios-e2e is supposed to work? It obviously doesn't work on master but these tvOS vs iOS errors aren't making sense to me and make me wonder if it's unrelated...

I couldn't get this script to work on my machine, but a sure way to test whether Xcode 10 builds correctly when third-party is missing for me was:

open RNTester.xcodeproj in Xcode 10
Make sure to delete the third-party folder!
hit ⌘⇧K (clear build products)
hit ⌘B (build)

mmccartney · 2018-10-08T13:18:32Z

@radex Thanks for the instructions. That worked for me after I ran git clean -dfx to get rid of previous artifacts (e.g. xcuserdata folders) that were causing it to fail.

I'm satisfied with my latest changes. Please review and @radko93 if you are available to test, I think this is ready.

mmccartney · 2018-10-08T15:13:51Z

I saw some CI failures that seem unrelated to my changes so I just rebased from master.

radko93 · 2018-10-09T07:47:14Z

@mmccartney I cannot test it on this branch since I can't do a complete yarn install because detox is broken (@kelset?). Then postinstall step that you've added is not being run

kelset · 2018-10-09T08:23:05Z

Yeah we are importing the Detox commit, expect it to land in the next 24hrs more or less.

radko93 · 2018-10-09T09:35:07Z

@mmccartney please rebase and let me know after it gets fixed and let me know afterwards

…ling third-party

…e existing file references from <group> to <project> relative so build system makes connection with outputFiles

mmccartney · 2018-10-09T13:20:19Z

@radko93 I rebased

mmccartney · 2018-10-09T16:01:13Z

@radko93 I removed the postinstall step because of feedback from others. Nothing to test there. This PR now is a fix to the Xcode project so that it works with the new build system. It should work equally well with the old build system on Xcode 9. I have tested with Legacy Build System as well as the New Build System using Xcode 10. I will update the test plan (sorry about the oversight).

mmccartney · 2018-10-10T16:03:34Z

@kelset Do you know if the detox commit has landed? I see a lot of new commits on master but nothing seems to be related to detox. It looks like all the CI checks passed except AppVeyor which appears to have errors unrelated to my work?

Let me know if there is anything more for me to do to get this integrated.

kelset · 2018-10-10T16:17:41Z

Do you know if the detox commit has landed?

Not yet, @TheSavior is actively working on it atm.

It looks like all the CI checks passed except AppVeyor which appears to have errors unrelated to my work?

Yeah you can ignore AppVeyor.

Aside from that, at the moment I don't have much time to review the final version of this PR, I'll ask a couple people to take a look asap.

facebook-github-bot

hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

react-native-bot · 2018-10-12T20:55:09Z

@mmccartney merged commit b44c5ae into facebook:master.

Summary: Fixes facebook#20774 The new Xcode build system uses parallel execution to run build steps that don't have an obvious dependency. Our Xcode project was written with the assumption that the **Install Third Party** build step is run _before_ compiling the `third-party` libraries. To address this issue, this PR adds dependency information to the project to teach Xcode that `ios-install-third-party.sh` is generating the files under `third-party`. With this additional information, Xcode correctly waits for `ios-install-third-party.sh` to finish before advancing to the compile step. In addition to the Xcode project changes, I had to make some changes to the script `ios-install-third-party.sh` so that 1. it would always execute the `ios-configure-glog.sh` script regardless of how it was invoked 2. it would always install the libraries even if Xcode had partially created the tree or if a previous install was interrupted Pull Request resolved: facebook#21458 Differential Revision: D10365495 Pulled By: hramos fbshipit-source-id: c88042583f21d2447a16f6ae2b6abb929c659a26

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 3, 2018

react-native-bot added the ✅Release Notes label Oct 3, 2018

kelset added the Platform: iOS iOS applications. label Oct 3, 2018

hramos added the 🐛Bug Fix label Oct 3, 2018

mmccartney requested a review from hramos as a code owner October 4, 2018 13:48

radex reviewed Oct 5, 2018

View reviewed changes

kelset mentioned this pull request Oct 5, 2018

0.57.2 Discussion react-native-community/releases#46

Closed

mmccartney force-pushed the master branch from a46db51 to ec46f2e Compare October 8, 2018 15:13

radex approved these changes Oct 8, 2018

View reviewed changes

kelset mentioned this pull request Oct 9, 2018

☂️ Supporting Xcode 10 and the new Xcode build system #19573

Closed

8 tasks

mmccartney and others added 5 commits October 9, 2018 08:19

Fix Xcode 10 errors relating to third-party

cef5e5a

use postinstall to run ios-install-third-party.sh

1642559

make Xcode wait for ios-install-third-party.sh to finish before compi…

904628d

…ling third-party

make ios-install-third-party.sh ignore incomplete installs

cacccc4

move outputFiles into Xcode project to be Xcode 9.x compatible; chang…

0bbb388

…e existing file references from <group> to <project> relative so build system makes connection with outputFiles

mmccartney force-pushed the master branch from ec46f2e to 0bbb388 Compare October 9, 2018 13:20

xzilja mentioned this pull request Oct 12, 2018

[ios] Fails to build with default XCode 10 settings invertase/react-native-firebase#1529

Closed

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Oct 12, 2018

facebook-github-bot approved these changes Oct 12, 2018

View reviewed changes

facebook-github-bot closed this in b44c5ae Oct 12, 2018

facebook locked as resolved and limited conversation to collaborators Oct 12, 2018

react-native-bot added the Merged This PR has been merged. label Oct 12, 2018

hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2019

react-native-bot added the Bug label May 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Xcode 10 errors relating to third-party #21458

Fix Xcode 10 errors relating to third-party #21458

mmccartney commented Oct 3, 2018 •

edited

kelset commented Oct 3, 2018

kelset commented Oct 3, 2018 •

edited

radex commented Oct 4, 2018 •

edited

radex commented Oct 4, 2018 •

edited

mmccartney commented Oct 4, 2018

kelset commented Oct 4, 2018

radex commented Oct 4, 2018

ikesyo commented Oct 4, 2018 •

edited

mmccartney commented Oct 4, 2018

ikesyo commented Oct 4, 2018 •

edited

mmccartney commented Oct 4, 2018

mmccartney commented Oct 4, 2018

radex Oct 5, 2018

radex commented Oct 5, 2018

kelset commented Oct 5, 2018

radko93 commented Oct 5, 2018

mmccartney commented Oct 5, 2018 •

edited

radex commented Oct 6, 2018

mmccartney commented Oct 6, 2018

radex commented Oct 6, 2018

mmccartney commented Oct 8, 2018

mmccartney commented Oct 8, 2018

radko93 commented Oct 9, 2018 •

edited

kelset commented Oct 9, 2018

radko93 commented Oct 9, 2018

mmccartney commented Oct 9, 2018

mmccartney commented Oct 9, 2018

mmccartney commented Oct 10, 2018 •

edited

kelset commented Oct 10, 2018

facebook-github-bot left a comment

react-native-bot commented Oct 12, 2018

Fix Xcode 10 errors relating to third-party #21458

Fix Xcode 10 errors relating to third-party #21458

Conversation

mmccartney commented Oct 3, 2018 • edited

Test Plan:

Release Notes:

kelset commented Oct 3, 2018

kelset commented Oct 3, 2018 • edited

radex commented Oct 4, 2018 • edited

radex commented Oct 4, 2018 • edited

mmccartney commented Oct 4, 2018

kelset commented Oct 4, 2018

radex commented Oct 4, 2018

ikesyo commented Oct 4, 2018 • edited

mmccartney commented Oct 4, 2018

ikesyo commented Oct 4, 2018 • edited

mmccartney commented Oct 4, 2018

mmccartney commented Oct 4, 2018

radex Oct 5, 2018

Choose a reason for hiding this comment

radex commented Oct 5, 2018

kelset commented Oct 5, 2018

radko93 commented Oct 5, 2018

mmccartney commented Oct 5, 2018 • edited

radex commented Oct 6, 2018

mmccartney commented Oct 6, 2018

radex commented Oct 6, 2018

mmccartney commented Oct 8, 2018

mmccartney commented Oct 8, 2018

radko93 commented Oct 9, 2018 • edited

kelset commented Oct 9, 2018

radko93 commented Oct 9, 2018

mmccartney commented Oct 9, 2018

mmccartney commented Oct 9, 2018

mmccartney commented Oct 10, 2018 • edited

kelset commented Oct 10, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented Oct 12, 2018

mmccartney commented Oct 3, 2018 •

edited

kelset commented Oct 3, 2018 •

edited

radex commented Oct 4, 2018 •

edited

radex commented Oct 4, 2018 •

edited

ikesyo commented Oct 4, 2018 •

edited

ikesyo commented Oct 4, 2018 •

edited

mmccartney commented Oct 5, 2018 •

edited

radko93 commented Oct 9, 2018 •

edited

mmccartney commented Oct 10, 2018 •

edited