zulip / zulip-mobile

Zulip mobile apps for Android and iOS.
https://zulip.com/apps/
Apache License 2.0
1.29k stars 650 forks source link

Build: Use "Autolinking" (RN >= v0.60) #4026

Closed chrisbobbe closed 4 years ago

chrisbobbe commented 4 years ago

Once we're on RN >= v0.60 (that's #3548), we can further increase the ease of managing our native code dependencies, even more than we did on iOS with CocoaPods in #3987.

EDIT: Ah, I just realized that there'd been some previous discussion of this! That's here.

The autolinking docs are here.

The key step that we will no longer have to do is react-native link. Note that this distinguishes "autolinking" from "automatic linking" (what a choice of names!), described in the "Linking Libraries" doc. Note that the "Manual Linking" step, there, describing steps you can take through Xcode to modify the ios/project.pbxproj file, is completely obsolete following the use of CocoaPods in #3987.

I expect we'll want to improve this issue heading (I'm open to this message being edited, wiki-style), but for now, I wanted a place to put a specific comment about one of our dependencies, which I'll do in the next comment.

chrisbobbe commented 4 years ago

At https://github.com/zulip/zulip-mobile/pull/3987#discussion_r402002988, Greg was puzzled about the presence of a libz.tbd in project.pbxproj (our project's Xcode config):

This one isn't in the upstream template -- but it also looks like it never was. It entered our project file in a18c95b (#881).

Well, I guess that makes any cleanup of it orthogonal to this PR, then.

As that commit and PR hint at, but don't really say, libz was introduced when we first installed @sentry/react-native. It still receives some special treatment in the latest version: it appears under platforms.ios.sharedLibraries in @sentry/react-native's react-native.config.js, a file that we know is capable of causing changes in the project.pbxproj file when you run react-native link. The doc for that bit of a react-native.config.js file seems to be here.

I wanted to check whether libz would still be included in a fresh install of @sentry/react-native, now that we're using CocoaPods (which wasn't the case for the original install).

So, I ran react-native unlink @sentry/react-native, then manually removed libz from project.pbxproj (this wasn't done automatically!). Then I ran react-native link @sentry/react-native, and libz did indeed reappear in project.pbxproj.

I'm not sure how the react-native.config.js file gets treated with "autolinking". I understand that config file as something that makes things happen on react-native link, but I'm not sure what the behavior is when react-native link isn't explicitly run anymore. I'm wondering the same thing in the context of some font files included with react-native-vector-icons.

So, I think what I'd like to do when we start using "autolinking" is, as part of the required react-native unlink of everything

Autolinking is a replacement for react-native link. If you have been using React Native before version 0.60, please unlink native dependencies if you have any from a previous install.

— is to also remove libz from project.pbxproj (this can be done through Xcode in the left sidebar, with ZulipMobile > Frameworks > libz.tbd > [right-click] > Delete > Move to Trash).

Then, we can see if it gets re-added when we run pod install.

rk-for-zulip commented 4 years ago

So, I ran react-native unlink @sentry/react-native, then manually removed libz from project.pbxproj (this wasn't done automatically!).

Really! That's interesting, because it does seem to have done so for me.

I wonder if there's something else that depends on it less explicitly. Do you still see libz in the project file if you run the miniscript in that commit message to unlink all the dependencies? It's a few months old, so it still lists the old toast library, but that probably shouldn't matter for this.

(I'd just check myself, but apparently react-native link does approximately nothing to the Xcode project files if you run it on Linux.)

but I'm not sure what the behavior is when react-native link isn't explicitly run anymore. I'm wondering the same thing in the context of some font files included with react-native-vector-icons.

For me, it removed both libz and the fonts (same commit), and replaced neither; I ended up adding the fonts back in manually. I don't know if Sentry still needs an explicit libz link – I didn't add it myself, but I don't think I got to the point of testing Sentry, so that's not meaningful.

chrisbobbe commented 4 years ago

Aha, I'm glad to see these results and to have that nonce script to test with!

I'm also happy (one must rejoice when one can, dealing with iOS dependencies) to see what looks like a pattern.

I see

a) an automatic removal of libz after running that script from 33f4b41c8^, and b) no automatic removal of libz after running that script from 33f4b41c8

where 33f4b41c8 is where we started using CocoaPods. I think the behaviors of both react-native link and react-native unlink change dramatically in the presence of a Podfile.

chrisbobbe commented 4 years ago

And, indeed, I see a) and b) when I just run react-native unlink @sentry/react-native, so it's doubtful that some other dependency is involved, which is another win for our understanding.

chrisbobbe commented 4 years ago

I don't know if Sentry still needs an explicit libz link – I didn't add it myself, but I don't think I got to the point of testing Sentry, so that's not meaningful.

I saw that it built successfully without libz, but I didn't go to the trouble of getting Sentry working in debug mode, to test, either. Since libz is still mentioned in that react-native.config.js file in Sentry's latest master, I think it's plausible that it is still needed, but we'll have a better picture once we see if libz gets automatically re-added under the "autolinking" regime.

chrisbobbe commented 4 years ago

I ended up adding the fonts back in manually

They should be added by CocoaPods, regardless of whether you're using CocoaPods on RN v0.59 or RN v0.60. Did you have CocoaPods working in that commit?

On running pod install, CocoaPods reacts to the line

s.resources      = "Fonts/*.ttf"

in react-native-vector-icons' Podspec by creating a new build phase, called "[CP] Copy Pods Resources", in our ZulipMobile project, which makes sure the font files are included. I haven't fully tracked down what code handles the create/edit/delete of that build phase, but it looks like an important piece that's used by the build phase is a script generated by this code in CocoaPods.

There's been some discussion about how these fonts are handled here and here, in addition to my link on "wondering" above.

Also, the font files don't appear as "files" in "Resources", or anywhere else in the left sidebar, but rather, as "paths" in that new build phase; Greg mentions that here.

gnprice commented 4 years ago

So, I think what I'd like to do when we start using "autolinking" [...] is to also remove libz from project.pbxproj [...]

Then, we can see if it gets re-added when we run pod install.

One more observation on this bit: the RNSentry.podspec inside node_modules/@sentry/react-native/ pulls in the Sentry pod:

  s.dependency 'Sentry', '~> 4.4.0'

And in turn Sentry.podspec has this line:

  s.libraries = 'z', 'c++'

I expect that means that under pod install, a libz dependency (as well as libc++) will end up in the generated Pods.xcodeproj, but not in our own Xcode project.

chrisbobbe commented 4 years ago

And in turn Sentry.podspec has this line:

Ohhh — is the z -> libz transformation similar to the transformation we've seen with React -> libReact.a, etc.? I didn't make that connection, but it sounds reasonable.

chrisbobbe commented 4 years ago

I expect that means that under pod install, a libz dependency (as well as libc++) will end up in the generated Pods.xcodeproj, but not in our own Xcode project.

In the status quo, without "autolinking", we see what we expect: pod install doesn't put libz in in our own Xcode project.

But pod install isn't the complete linking procedure in the status quo, react-native link is (which, among other things, seems to run pod install if you have a Podfile). And react-native link does add it.

One interesting observation, given what you've just pointed out about Sentry.podspec, is that libz is nowhere to be found in Pods.xcodeproj after running just pod install in the status quo.

gnprice commented 4 years ago

Ohhh — is the z -> libz transformation similar to the transformation we've seen with React -> libReact.a, etc.?

Yup! Here's GCC's documentation for it: https://gcc.gnu.org/onlinedocs/gcc/Link-Options.html

-llibrary

Search the library named library when linking. [...]

Static libraries are archives of object files, and have file names like liblibrary.a. Some targets also support shared libraries, which typically have names like liblibrary.so. [...]

(Xcode is built on Clang, not GCC; but this convention is standard across all kinds of Unix-style compilers and linkers, and because we found Clang's documentation was thin when looking into the #include "foo.h" question, I just went straight to the GCC docs.)