Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Xcode 10 errors relating to third-party #21458

Closed
wants to merge 5 commits into from

Conversation

mmccartney
Copy link
Contributor

@mmccartney mmccartney commented Oct 3, 2018

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

  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

Test Plan:

  1. Under Xcode 9 and Xcode 10, delete the third-party tree and build the React project
  2. Under Xcode 9 and Xcode 10, delete the third-party tree and build the RNTester project
  3. 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

@facebook-github-bot 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
@kelset
Copy link
Contributor

kelset commented Oct 3, 2018

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

@kelset kelset added the Platform: iOS iOS applications. label Oct 3, 2018
@kelset
Copy link
Contributor

kelset commented Oct 3, 2018

👋 @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
Copy link
Contributor

radex commented Oct 4, 2018

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.

captura de pantalla 2018-10-04 a las 14 37 38

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

@radex
Copy link
Contributor

radex commented Oct 4, 2018

btw @mmccartney @kelset, I just did this:

captura de pantalla 2018-10-04 a las 14 49 59

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
Copy link
Contributor Author

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
Copy link
Contributor

kelset commented Oct 4, 2018

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

@mmccartney mmccartney requested a review from hramos as a code owner October 4, 2018 13:48
@radex
Copy link
Contributor

radex commented Oct 4, 2018

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
Copy link
Contributor

ikesyo commented Oct 4, 2018

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

@mmccartney
Copy link
Contributor Author

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
Copy link
Contributor

ikesyo commented Oct 4, 2018

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@radex
Copy link
Contributor

radex commented Oct 5, 2018

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
Copy link
Contributor

kelset commented Oct 5, 2018

@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
Copy link
Contributor

radko93 commented Oct 5, 2018

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

@mmccartney
Copy link
Contributor Author

mmccartney commented Oct 5, 2018

@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
Copy link
Contributor

radex commented Oct 6, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor

radex commented Oct 6, 2018

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:

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

@mmccartney
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@radko93
Copy link
Contributor

radko93 commented Oct 9, 2018

@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
Copy link
Contributor

kelset commented Oct 9, 2018

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

@radko93
Copy link
Contributor

radko93 commented Oct 9, 2018

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

@mmccartney
Copy link
Contributor Author

@radko93 I rebased

@mmccartney
Copy link
Contributor Author

@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
Copy link
Contributor Author

mmccartney commented Oct 10, 2018

@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
Copy link
Contributor

kelset commented Oct 10, 2018

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 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
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@react-native-bot
Copy link
Collaborator

@mmccartney merged commit b44c5ae into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Oct 12, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Oct 12, 2018
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2019
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When new Xcode build system is used, Xcode build fails to download third-party dependencies
8 participants