CocoaPods near-term followup #4028

chrisbobbe · 2020-04-08T01:48:01Z

Working from the list at #3987 (comment) and following.

This does not include:

moving the Start Metro script (opened as ios build: Move "Start Metro" script into a script file, make it not run for release build. #4027)
finding out exactly what's going on (and what should be going on) with libz — our latest knowledge is at Build: Use "Autolinking" (RN >= v0.60) #4026 (comment), but I think action can wait till we're on RN v0.60.
unimodules (that's Add react-native-unimodules so we can use Expo packages #4016)

chrisbobbe · 2020-04-08T03:16:32Z

Of course the main thing I'm hoping to learn is the many ways I'm sure the postinstall script could be improved. 🙂

chrisbobbe · 2020-04-08T18:06:39Z

(Marking as P1 because CocoaPods was marked P1, pinging @gnprice)

gnprice

Thanks @chrisbobbe !

Some shell-script comments below, as you hoped 🙂 Haven't gotten to the rest yet, but wanted to not hold off on getting you these.

@andersk Do you know of another good resource to point to for the key points of writing shell scripts? I linked to two below but I feel like there are a few other good things to mention that they don't cover.

gnprice · 2020-04-08T20:35:40Z

tools/postinstall

+ROOT_DIR=$(git rev-parse --show-toplevel)
+
+pod_install() (
+  [[ "$OSTYPE" == "darwin"* ]] || { exit 0; };


Huh, TIL!

Looks like OSTYPE is a Bash feature:
https://www.gnu.org/software/bash/manual/bash.html#index-OSTYPE

Yeah! 🙂 I made sure to check that same doc before using this solution I found on SO.

Good thought 😉

gnprice · 2020-04-08T20:42:53Z

tools/postinstall

+ROOT_DIR=$(git rev-parse --show-toplevel)
+
+pod_install() (
+  [[ "$OSTYPE" == "darwin"* ]] || { exit 0; };


For the control flow:

Use return; exit will exit the whole script.

Let's let the return have its own line, for the same reasons we do in JS -- make it easy to scan vertically and see where control exits the function.

No need for braces; foo || bar works fine.

So, with return on its own line, and removing the braces:

[[ "$OSTYPE" == "darwin"* ]] || return

oh — I guess, return 0?

Yeah -- plain return will pass on the previous command's exit code, which here would be failure (and then cause the whole script to exit with failure, given the set -e.)

gnprice · 2020-04-08T20:50:32Z

tools/postinstall

+
+ROOT_DIR=$(git rev-parse --show-toplevel)
+
+pod_install() (


This is a subtle one! You want a { at the end of this line (and matching at the end of the function.) Just like JS (and C and ...) in that respect.

This version... is actually perfectly legal shell. Formally, the bit after pod_install() is just any "compound command" -- it could be an if or while, or a ( ... ) like this, or a { ... }. In practice you always just want to write { ... }.

The difference between the last two is subtle: the round parens cause a new shell to be run as a child process (a "subshell"). Effects include:

setting variables inside the function won't affect the caller

exit will only exit the function, not the whole script (so the exit 1 in the error case won't have the right effect)

Mmm, makes sense. I didn't know about return, but I did find the round parens (and I think you've mentioned those before), so I was actually using them intentionally, so that the first exit (which applies unless you're on macOS) would only exit the subshell and not the whole script. But that second exit, if we don't have CocoaPods, should exit the whole script, which I guess I forgot about.

And it sounds like curly braces is the done thing, so I'm much happier to use them, now I know about return. 🙂

Yep -- subshells are useful, but I would be super confused to see a codebase where they were used on all functions 🙂 If you did want a subshell to apply to a whole function, I think I'd want to write the braces anyway and then the parens inside those, so it stood out as more visually distinct.

gnprice · 2020-04-08T20:55:21Z

tools/postinstall

+pod_install() (
+  [[ "$OSTYPE" == "darwin"* ]] || { exit 0; };
+  hash pod 2>/dev/null || {
+    echo >&2 "It looks like you haven't installed CocoaPods. Please do so, following the instructions at https://reactnative.dev/docs/0.60/getting-started, at 'React Native CLI Quickstart' > 'Xcode & CocoaPods', and run `yarn` again."


For a long message like this, the code can be made cleaner by using a "here-document". See tools/test for some examples, or tools/lib/ensure-coreutils.sh for some examples in a shorter context.

The `yarn` inside the double-quoted string here is a command substitution that will actually run yarn.

Oh, nice; yeah, we don't want to actually run yarn here. Will it run yarn if I use a "here-document", e.g.:

cat >&2 <<EOF The postinstall script (see scripts.postinstall in package.json) requires CocoaPods. Please run: sudo gem install cocoapods as instructed at https://reactnative.dev/docs/0.60/getting-started, under 'React Native CLI Quickstart' > 'Xcode & CocoaPods'. Then, run `yarn` again. EOF

and, if it's safe to include the backticks, is that a desired bit of formatting that we should keep?

Aha! ShellCheck doesn't want me to use the backticks:

Line 19: Then, run `yarn` again. ^-- SC2006: Use $(...) notation instead of legacy backticked `...`. Did you mean: (apply this, apply all SC2006) Then, run $(yarn) again.

Maybe I'll put it on its own line, the same as sudo gem install cocoapods, above.

A here-document <<word does parameter expansion, command substitution, and arithmetic expansion. A here-document <<'word' does not. (Documentation for POSIX shell and Bash.)

gnprice · 2020-04-08T20:59:16Z

tools/postinstall

@@ -0,0 +1,18 @@
+#!/bin/bash
+


Write set -eu at the top.

A couple of useful sources with discussion:
https://sipb.mit.edu/doc/safe-shell/
https://jvns.ca/blog/2017/03/26/bash-quirks/

(One of those recommends also set -o pipefail, but I recall concluding at some point that that could introduce its own hard-to-avoid kinds of bugs.)

andersk · 2020-04-08T21:41:25Z

@andersk Do you know of another good resource to point to for the key points of writing shell scripts? I linked to two below but I feel like there are a few other good things to mention that they don't cover.

Yes! Run ShellCheck on all of your shell scripts and get them down to zero diagnostics. Run it every time you commit. Run it in CI. Run it automatically in your editor. ShellCheck knows things that you didn’t know you needed to know. ShellCheck knows things that I didn’t know I needed to know. ShellCheck is excellent. ShellCheck is magical. ShellCheck is the one shining beacon of hope in the vast fiery hellscape that is the world of UNIX shell scripting. Run ShellCheck.

Run it.

chrisbobbe · 2020-04-08T22:09:00Z

Nice! That led me to the ShellCheck "gallery of bad code", via https://github.com/zulip/zulip/blob/7ff9b2250/docs/testing/linters.md#shell-scripts

gnprice

Thanks for the revisions! Comments below; I've now read the whole branch.

gnprice · 2020-04-09T00:16:15Z

tools/postinstall

+  [[ "$OSTYPE" == "darwin"* ]] ||
+    return 0


nit: put the operator at the start of the next line, rather than end of the previous -- that way it's easy to see how the stuff on the second line relates to the first one

Then you'll want a \ at the end of the first line so the command doesn't end there. See examples in tools/test, like:

run_deps() { files_check package.json yarn.lock \ || return 0

(Same thing applies in JS -- see our 13530c3 which overrides Prettier making the opposite choice here.)

tools/postinstall

gnprice · 2020-04-09T00:41:19Z

tools/postinstall

+The postinstall script (see scripts.postinstall in package.json)
+requires CocoaPods. Please run
+
+  sudo gem install cocoapods


Oof, it's sad for that to be the install instruction. There was a period when that was the usual way Python software would tell people to install it -- sudo pip install foo, so installing into a shared system location as root -- and for several reasons it tends to create a mess.

How about let's just link people to CocoaPods's own install instructions:
https://guides.cocoapods.org/using/getting-started.html
In fact, let's definitely replace the RN link with that one -- I see now that the RN instructions are just a copy-paste of part of that page. (Which maybe helps explain why the text on the RN page sounds a bit out-of-context.)

That page leads with the sudo gem install cocoapods approach... but then it presents a much better one. And that also leaves it in upstream's hands to improve the instructions. 🙂

(The better solution they give is a bit clumsy, and I hope someday the Ruby/gems community finds their way to making it smoother; but modulo some smoothening it's exactly the good solution that the Python community and pip have moved to as they've more or less gotten authors to stop telling people to sudo pip install. The Python equivalent is: pip install --user, plus add to your PATH but they use a directory that on modern Linux distros is already there by default.)

docs/howto/build-run.md

tools/postinstall

docs/howto/build-run.md

gnprice · 2020-04-09T01:01:56Z

docs/howto/build-run.md

+folder. A different folder is used for command-line builds and builds
+through Xcode.
+- If building from the command line with `react-native run-ios`, run
+  `rm -rf ios/build`.


Huh, I didn't know that! I guess I haven't used react-native run-ios in a long time :-) -- for whatever reason, I always use Xcode for running debug versions on iOS, even though I always use react-native run-android for Android.

Yep!

My heart skipped a beat when I realized, several commits on master after CocoaPods landed, that I could consistently build with Xcode and consistently NOT build with react-native run-ios. This fact made me doubt whether I had indeed done the full testing necessary (including building from the command line) before CocoaPods got merged.

Happily, I discovered that two different build folders are used, and, on clearing them both and finding that both builds worked fine afterward, I much more confidently remembered that I'd done all the required testing. 😝

gnprice · 2020-04-09T01:04:41Z

docs/howto/ios-tips.md

+If it's not your first time, and you haven't changed anything in the
+`ios` folder yourself, you might see build failures caused by
+interfering residue from a previous build. If you do, clear Xcode's
+build folder for ZulipMobile with `rm -rf`, or through Finder. The
+default path for the build folder is
+`~/Library/Developer/Xcode/DerivedData/ZulipMobile-diuocloqwafvdpeozujwphdrhalf`
+(the hash at the end will be different), but you can find it in Xcode
+at File > Workspace Settings > Build Location. If that doesn't fix it,
+see our [build failure troubleshooting doc][build-run-troubleshoot].
+


It'd be good to avoid duplicating these details -- let's find a good home for them, and then from elsewhere we can link to that (with just enough words around each link to make it easy to find when it's what you need).

Probably making a section in this ios-tips doc, like ### Clearing the build folder, would be a good place.

(with just enough words around each link to make it easy to find when it's what you need)

Sure, makes sense. I agree that its home should be in ios-tips, but it definitely needs a shoutout from Troubleshooting, as that's where people go for all other build failures.

docs/howto/ios-tips.md

chrisbobbe · 2020-04-09T18:40:12Z

Thanks for the review! OK, I've pushed the latest revision. I also noticed that we were using four-space indents in our shell scripts, so I adapted to that for the postinstall script.

gnprice

Thanks @chrisbobbe for the revisions! A few comments, all small.

docs/howto/build-run.md

docs/howto/ios-tips.md

package.json

tools/postinstall

There's a lot more that could be improved in this doc; at zulip#3307 (comment), we noted that we don't want to keep an up-to-date doc of everything in the /src directory; everything should be done inline.

chrisbobbe · 2020-04-10T17:08:37Z

Thanks for the review! I just pushed these tweaks, including one more: I was having the postinstall script print that it was running, so people would know where the output that followed was coming from. But yarn does that for us, so, no need.

On expo-application v2.1.0 (see 18c37ce), we were seeing this warning on initial runs of `yarn`: warning " > [email protected]" has unmet peer dependency "@unimodules/core@~1.0.0". (Same as in 5b6014e: "No such warning if `yarn` stops after step 'Resolving packages' and prints 'Already up-to-date'; but if it has any actual work to do, it always shows this warning in step 'Linking dependencies'.) Version 2.1.0 of expo-application declares that it depends on a version of @unimodules/core (1.0.0) that was already several months out-of-date at the time expo-application was first created, and it seems unlikely that this was well-considered. I filed expo/expo#7728 for this, and it was promptly addressed by releasing version 2.1.1 of expo-application that had "*" for its version constraint on the @unimodules/core peer dependency. So, switch to that new version. Even when we do that, though, we get the following: warning " > [email protected]" has unmet peer dependency "@unimodules/core@*". This goes away when we declare a version of @unimodules/core in our own dependencies, though. Greg did some digging and concluded that this is likely what we're supposed to do, actually, because it's declared as a "peer dependency": """ Here's the reference doc for it: https://docs.npmjs.com/files/package.json#peerdependencies but it's not very clear about what it actually *means* -- mostly it just describes a use-case. The closest I've found to clear docs on it may be this Yarn blog post: https://classic.yarnpkg.com/blog/2018/04/18/dependencies-done-right/ > The `peerDependencies` object guarantees that, for each entry, any > package that requires you will be requested to provide a copy of > the dependency listed here, ideally matching the version you > requested. It also guarantees that you’ll be able to access this > dependency through `require()`, and finally **it also guarantees > that the return of `require()` will be exactly the same version & > instance than the one your parent would obtain if they were to > `require()` it themselves**. [emphasis in original] And if you work through the consequences of that... it means in particular that the "parent" has to have that dependency and be able to `require()` it themselves. Which, if you're declaring your dependencies cleanly, means the parent should have it as a dependency in their own `package.json`. """ So, declare @unimodules/core in our own "dependencies", with the version specified by react-native-unimodules in its active version. This matching should be done each time we change versions of react-native-unimodules.

gnprice · 2020-04-10T19:27:45Z

And merged -- thanks again!

chrisbobbe added a-iOS a-tools labels Apr 8, 2020

chrisbobbe requested a review from gnprice April 8, 2020 01:48

chrisbobbe force-pushed the pr-cp-followup branch from 87deb25 to 4f4c6ec Compare April 8, 2020 01:49

chrisbobbe force-pushed the pr-cp-followup branch from 4f4c6ec to 09f1c1a Compare April 8, 2020 17:57

chrisbobbe added the P1 high-priority label Apr 8, 2020

gnprice reviewed Apr 8, 2020

View reviewed changes

chrisbobbe force-pushed the pr-cp-followup branch from 09f1c1a to 9f0081d Compare April 8, 2020 22:17

gnprice reviewed Apr 9, 2020

View reviewed changes

chrisbobbe force-pushed the pr-cp-followup branch from 9f0081d to 94a5480 Compare April 9, 2020 18:39

gnprice reviewed Apr 10, 2020

View reviewed changes

docs/howto/build-run.md Outdated Show resolved Hide resolved

docs/howto/ios-tips.md Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

tools/postinstall Outdated Show resolved Hide resolved

tools/postinstall Outdated Show resolved Hide resolved

Chris Bobbe added 5 commits April 10, 2020 10:05

cocoapods: Automate pod install with postinstall script.

e66604d

howto/ios-tips: Make sure to run .xcworkspace, not .xcodeproj.

487ee09

howto/build-run: Also suggest starting out with Xcode/Android Studio.

c96e3b8

docs/architecture: Remove long-outdated bit.

24f50dd

There's a lot more that could be improved in this doc; at zulip#3307 (comment), we noted that we don't want to keep an up-to-date doc of everything in the /src directory; everything should be done inline.

ios docs: Suggest clearing the build folders.

75f4946

chrisbobbe force-pushed the pr-cp-followup branch from 94a5480 to 79122e4 Compare April 10, 2020 17:07

chrisbobbe force-pushed the pr-cp-followup branch from 79122e4 to bc27f2d Compare April 10, 2020 17:10

gnprice merged commit bc27f2d into zulip:master Apr 10, 2020

chrisbobbe mentioned this pull request May 29, 2020

Run shellcheck, add to tools/test. #4130

Open

chrisbobbe deleted the pr-cp-followup branch November 6, 2020 03:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CocoaPods near-term followup #4028

CocoaPods near-term followup #4028

chrisbobbe commented Apr 8, 2020 •

edited

Loading

chrisbobbe commented Apr 8, 2020

chrisbobbe commented Apr 8, 2020

gnprice left a comment

gnprice Apr 8, 2020

chrisbobbe Apr 8, 2020

gnprice Apr 10, 2020

gnprice Apr 8, 2020

chrisbobbe Apr 8, 2020

chrisbobbe Apr 8, 2020

gnprice Apr 10, 2020

gnprice Apr 8, 2020

chrisbobbe Apr 8, 2020 •

edited

Loading

gnprice Apr 10, 2020

gnprice Apr 8, 2020

andersk Apr 8, 2020

chrisbobbe Apr 8, 2020 •

edited

Loading

chrisbobbe Apr 8, 2020

chrisbobbe Apr 8, 2020

andersk Apr 8, 2020

gnprice Apr 8, 2020

andersk commented Apr 8, 2020

chrisbobbe commented Apr 8, 2020 •

edited

Loading

gnprice left a comment

gnprice Apr 9, 2020

gnprice Apr 9, 2020

gnprice Apr 9, 2020

chrisbobbe Apr 9, 2020

gnprice Apr 9, 2020

chrisbobbe Apr 9, 2020

chrisbobbe commented Apr 9, 2020 •

edited

Loading

gnprice left a comment

chrisbobbe commented Apr 10, 2020

gnprice commented Apr 10, 2020


		ROOT_DIR=$(git rev-parse --show-toplevel)

		pod_install() (

CocoaPods near-term followup #4028

CocoaPods near-term followup #4028

Conversation

chrisbobbe commented Apr 8, 2020 • edited Loading

chrisbobbe commented Apr 8, 2020

chrisbobbe commented Apr 8, 2020

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andersk commented Apr 8, 2020

chrisbobbe commented Apr 8, 2020 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 9, 2020 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 10, 2020

gnprice commented Apr 10, 2020

chrisbobbe commented Apr 8, 2020 •

edited

Loading

chrisbobbe Apr 8, 2020 •

edited

Loading

chrisbobbe Apr 8, 2020 •

edited

Loading

chrisbobbe commented Apr 8, 2020 •

edited

Loading

chrisbobbe commented Apr 9, 2020 •

edited

Loading