Use xcpretty for iOS build output if installed #15607

tabrindle · 2017-08-22T18:07:25Z

iOS/xcodebuild logging output sucks. It's completely unreadable because of how xcodebuild takes arguments. Fortunately a ruby gem has existed to fix this for years: xcpretty. You simply pipe xcodebuilds output to xcpretty and it cleans it up, showing relevant info in a more readable format.

This PR implements xcpretty output. It checks to see if xcpretty is in PATH, then uses it by default if it is, and can be disabled using a --verbose cli flag.

Test Plan

Create a project from master
run react-native run-ios
Confirm output looks like this:

Scanning 662 folders for symlinks in /Users/tabrindle/Developer/react-native/node_modules (5ms)
Found Xcode project Shop.xcodeproj
We couldn't boot your defined simulator due to an already booted simulator. We are limited to one simulator launched at a time.
Launching iPhone 6 (iOS 10.3)...
Building using "xcodebuild -project Shop.xcodeproj -configuration Debug -scheme Shop -destination id=0CD9ABF3-A8E4-43D6-A52B-F437FF0F45A0 -derivedDataPath build"
▸ Building React/yoga [Debug]
▸ Check Dependencies
▸ Building React/double-conversion [Debug]
▸ Check Dependencies
▸ Running script 'Install Third Party'
▸ Building React/third-party [Debug]
▸ Check Dependencies
▸ Building React/jschelpers [Debug]
▸ Check Dependencies
▸ Building React/cxxreact [Debug]
▸ Check Dependencies
▸ Building React/React [Debug]
▸ Check Dependencies
▸ Running script 'Start Packager'
▸ Running script 'Include RCTJSCProfiler'
▸ Building RCTSettings/RCTSettings [Debug]
▸ Check Dependencies
▸ Building BVLinearGradient/BVLinearGradient [Debug]
▸ Check Dependencies
▸ Building RCTActionSheet/RCTActionSheet [Debug]
▸ Check Dependencies
▸ Building RCTLinking/RCTLinking [Debug]
▸ Check Dependencies
▸ Building RCTWebSocket/fishhook [Debug]
▸ Check Dependencies
▸ Building RCTWebSocket/RCTWebSocket [Debug]
▸ Check Dependencies
▸ Building RCTText/RCTText [Debug]
▸ Check Dependencies
▸ Building RCTNetwork/RCTNetwork [Debug]
▸ Check Dependencies
▸ Building RCTAnimation/RCTAnimation [Debug]
▸ Check Dependencies
▸ Building RCTGeolocation/RCTGeolocation [Debug]
▸ Check Dependencies
▸ Building ReactNativeNavigation/ReactNativeNavigation [Debug]
▸ Check Dependencies
▸ Building RNI18n/RNI18n [Debug]
▸ Check Dependencies
▸ Building RCTImage/RCTImage [Debug]
▸ Check Dependencies
▸ Building RNVectorIcons/RNVectorIcons [Debug]
▸ Check Dependencies
▸ Building RCTVibration/RCTVibration [Debug]
▸ Check Dependencies
▸ Building Shop/Shop [Debug]
▸ Check Dependencies
▸ Running script 'Bundle React Native code and images'
▸ Building Shop/ShopTests [Debug]
▸ Check Dependencies
▸ Build Succeeded
Installing build/Build/Products/Debug-iphonesimulator/Shop.app
Launching com.marketamerica.MAMobile
com.marketamerica.MAMobile: 43497```
- run `react-native run-ios --verbose`
- confirm output looks as normal

Nothing else should be affected. This is simply a developer experience change. If xcpretty is not already installed, nothing will change at all. Many of us using fastlane have been spoiled by this for years.

pull-bot · 2017-08-22T18:14:45Z

@facebook-github-bot label Core Team

Generated by 🚫 dangerJS

eslint-bot · 2017-08-22T18:15:31Z

local-cli/runIOS/runIOS.js

+      stdio: [ 0, 'pipe', 'ignore', ]
+    });
+  } catch (error) {
+    console.log(error)


semi: Missing semicolon.

janicduplessis · 2017-08-22T20:48:39Z

local-cli/runIOS/runIOS.js

+      stdio: [ 0, 'pipe', 'ignore', ]
+    });
+  } catch (error) {
+    console.log(error);


I don't think we should log this, having this error print out could be confusing.

Good point- was just a relic of debug code. Will remove.

janicduplessis · 2017-08-22T20:51:00Z

local-cli/runIOS/runIOS.js

+    if (!verbose) {
+      var xcpretty = xcprettyAvailable();
+      if (xcpretty) {
+        var xcprettyProcess = child_process.spawn('xcpretty', [], { stdio: ['pipe', process.stdout, process.stderr] });


I'd rather avoid using var scoping here and just use let/const instead.

Normally I agree, but I was hoping to avoid running the synchronous xcprettyAvailable function if I could avoid it when verbose is enabled. I can flatten this out if this is not considered an issue.

I suppose I could just declare the the let outside the block. Feels less clean to me, but whatever you want! 👍

Yea I was just thinking declaring the let outside the block

shergin · 2017-08-25T05:01:17Z

local-cli/runIOS/runIOS.js

@@ -160,30 +170,44 @@ function buildProject(xcodeProject, udid, scheme, configuration = 'Debug', launc
      '-derivedDataPath', 'build',
    ];
    console.log(`Building using "xcodebuild ${xcodebuildArgs.join(' ')}"`);
+    let xcpretty, xcprettyProcess;


Why do we need xcpretty process?

It is to store a reference to the spawned child process to pipe the console output to. Needs to be declared at the same scope as the xcpretty variable to be accessible in subsequent blocks.

Sorry, I meant "Why do we need xcpretty variable here?".

(It looks redundant, we already have *process.)

xcpretty is simply a boolean to show that it is available. Could possibly be renamed to be more specific if that is a point of confusion.

One can only start the process if it is available. A couple other switches depend on it as well.

Essentially I don't want a variable to possibly Have different types based on state. Variables are cheap, and I believe this makes it more clear.

I am okay with both renaming or using xcprettyProcess as a flag.

I think we should get rid of xcpretty and just use xcprettyProcess as the flag.

just use xcpretty - either is falsy or contains the process

eslint-bot · 2017-08-29T19:19:25Z

local-cli/runIOS/runIOS.js

+  } catch (error) {
+    return false;
+  }
+  return true;


no-trailing-spaces: Trailing spaces not allowed.

@eslint-bot Idk what you're smoking.

facebook-github-bot · 2017-08-30T22:01:57Z

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-08-31T11:58:19Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

tabrindle · 2017-09-05T15:09:04Z

@shergin Are you ok with this PR?

tabrindle · 2017-09-08T14:19:02Z

@hramos can you take a look at this?

shergin · 2017-09-08T14:56:15Z

Something (probably unrelated to this PR) went wrong when I tried to land it.
Let's try to land it again on Monday.

facebook-github-bot · 2017-09-08T15:59:51Z

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: iOS/xcodebuild logging output sucks. It's completely unreadable because of how xcodebuild takes arguments. Fortunately a ruby gem has existed to fix this for years: xcpretty. You simply pipe xcodebuilds output to xcpretty and it cleans it up, showing relevant info in a more readable format. This PR implements xcpretty output. It checks to see if xcpretty is in PATH, then uses it by default if it is, and can be disabled using a --verbose cli flag. - Create a project from master - run `react-native run-ios` - Confirm output looks like this: ```tabrindle-mbp:shop-rn tabrindle$ make Scanning 662 folders for symlinks in /Users/tabrindle/Developer/react-native/node_modules (5ms) Found Xcode project Shop.xcodeproj We couldn't boot your defined simulator due to an already booted simulator. We are limited to one simulator launched at a time. Launching iPhone 6 (iOS 10.3)... Building using "xcodebuild -project Shop.xcodeproj -configuration Debug -scheme Shop -destination id=0CD9ABF3-A8E4-43D6-A52B-F437FF0F45A0 -derivedDataPath build" ▸ Building React/yoga [Debug] ▸ Check Dependencies ▸ Building React/double-conversion [Debug] ▸ Check Dependencies ▸ Running script 'Install Third Party' ▸ Building React/third-party [Debug] ▸ Check Dependencies ▸ Building React/jschelpers [Debug] ▸ Check Dependencies ▸ Building React/cxxreact [Debug] ▸ Check Dependencies ▸ Building React/React [Debug] ▸ Check Dependencies ▸ Running script 'Start Packager' ▸ Running script 'Include RCTJSCProfiler' ▸ Building RCTSettings/RCTSettings [Debug] ▸ Check Dependencies ▸ Building BVLinearGradient/BVLinearGradient [Debug] ▸ Check Dependencies ▸ Building RCTActionSheet/RCTActionSheet [Debug] ▸ Check Dependencies ▸ Building RCTLinking/RCTLinking [Debug] ▸ Check Dependencies ▸ Building RCTWebSocket/fishhook [Debug] ▸ Check Dependencies ▸ Building RCTWebSocket/RCTWebSocket [Debug] ▸ Check Dependencies ▸ Building RCTText/RCTText [Debug] ▸ Check Dependencies ▸ Building RCTNetwork/RCTNetwork [Debug] ▸ Check Dependencies ▸ Building RCTAnimation/RCTAnimation [Debug] ▸ Check Dependencies ▸ Building RCTGeolocation/RCTGeolocation [Debug] ▸ Check Dependencies ▸ Building ReactNativeNavigation/ReactNativeNavigation [Debug] ▸ Check Dependencies ▸ Building RNI18n/RNI18n [Debug] ▸ Check Dependencies ▸ Building RCTImage/RCTImage [Debug] ▸ Check Dependencies ▸ Building RNVectorIcons/RNVectorIcons [Debug] ▸ Check Dependencies ▸ Building RCTVibration/RCTVibration [Debug] ▸ Check Dependencies ▸ Building Shop/Shop [Debug] ▸ Check Dependencies ▸ Running script 'Bundle React Native code and images' ▸ Building Shop/ShopTests [Debug] ▸ Check Dependencies ▸ Build Succeeded Installing build/Build/Products/Debug-iphonesimulator/Shop.app Launching com.marketamerica.MAMobile com.marketamerica.MAMobile: 43497``` - run `react-native run-ios --verbose` - confirm output looks as normal Nothing else should be affected. This is simply a developer experience change. If xcpretty is not already installed, nothing will change at all. Many of us using fastlane have been spoiled by this for years. Closes facebook/react-native#15607 Differential Revision: D5740684 Pulled By: shergin fbshipit-source-id: 09ef21414b8b65be92595b19502b843fa943403d

use xcpretty if installed

6b8ee75

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 22, 2017

eslint-bot reviewed Aug 22, 2017

View reviewed changes

Trevor Brindle added 3 commits August 22, 2017 15:09

accidentally a semicolon

d96a5fa

fix other lint errors in runIOS.js

5175aa2

runIOS.js linting

57ef7f5

janicduplessis suggested changes Aug 22, 2017

View reviewed changes

Trevor Brindle and others added 4 commits August 22, 2017 17:03

remove error logging for xcprettyAvailable()

4f3a192

use let instead of var

dde8fe9

fix if block spacing

2361aea

Merge branch 'master' into optionally-use-xcpretty

2cda5c3

hramos added the Core Team label Aug 24, 2017

janicduplessis approved these changes Aug 24, 2017

View reviewed changes

shergin suggested changes Aug 25, 2017

View reviewed changes

remove extra xcprettyProcess variable

1cd2cbe

just use xcpretty - either is falsy or contains the process

eslint-bot reviewed Aug 29, 2017

View reviewed changes

remove unnecessary ternary

2fef71a

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Aug 30, 2017

facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Aug 31, 2017

tabrindle added 2 commits August 31, 2017 14:02

Merge branch 'master' into optionally-use-xcpretty

7f03f49

Merge branch 'master' into optionally-use-xcpretty

1247214

Merge branch 'master' into optionally-use-xcpretty

25a98b9

Merge branch 'master' into optionally-use-xcpretty

a700519

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Sep 8, 2017

facebook-github-bot closed this in cb8a116 Sep 8, 2017

Use xcpretty for iOS build output if installed #15607

Use xcpretty for iOS build output if installed #15607

Uh oh!

Conversation

tabrindle commented Aug 22, 2017

Test Plan

Uh oh!

pull-bot commented Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tabrindle Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Aug 30, 2017

Uh oh!

facebook-github-bot commented Aug 31, 2017

Uh oh!

tabrindle commented Sep 5, 2017

Uh oh!

tabrindle commented Sep 8, 2017

Uh oh!

shergin commented Sep 8, 2017

Uh oh!

facebook-github-bot commented Sep 8, 2017

Uh oh!

Uh oh!

pull-bot commented Aug 22, 2017 •

edited

Loading

tabrindle Aug 22, 2017 •

edited

Loading