Include CSSLayout.h consistently with other project includes

[robhogan]

Include CSSLayout headers in the same way as other project headers, ie #import <CSSLayout/CSSLayout.h> becomes #import "CSSLayout.h". CSSLayout is not a framework or system dependency, so shouldn't (AFAIK) be included with angle brackets. Doing so breaks framework builds, such as when RN is used as a pod in a swift project.

In combination with facebook/yoga#217 this fixes #9014 (specifically swift cocoapods projects). There is then no need for a separate CSSLayout pod subspec.

Tests run on the RN project in isolation (with changes inside CSSLayout itself also applied) and against a dummy swift project with RN included as a pod.

NB: This effectively reverts #9015 and may break non-swift cocoapods projects unless facebook/yoga#217 is merged and synced first.

Update: As discussed with @alloy and @emilsjolander, wrap these imports in a preprocessor macro so that #import <CSSLayout/CSSLayout.h> is used wherever it exists - needed for FB internal use as a library import.

[ghost]

By analyzing the blame information on this pull request, we identified @alloy and @ide to be potential reviewers.

[alloy]

Great work 👏

Indeed, seeing as CSSLayout is pulled in and builds as part of the 1 React target in the Xcode project, like any other core source code, the includes should not be using angle brackets.

As far as I can think of these are the permutations of how this should be able to build:

1. From the Xcode project that’s in the repo.
2. CocoaPods, as a static library
3. CocoaPods, as a dynamic library (framework) in an objc app
4. CocoaPods, as a dynamic library (framework) in a Swift app

Which of those have you verified?

[robhogan]

I've now tested all of them. All four work (build) if facebook/yoga#217 is also applied, but 2,3 and 4 fail without it.

Cases 3 and 4 were already failing, which is what motivated this work. So effectively this PR breaks case 2 unless that other PR is merged first.

[alloy]

Perfect then 👍

I was slightly worried about changing the path in a public header like this one breaking for other projects that include the header from a CSSLayout directory, but I think it should be ok as with #include "…" the pre-processor should look for the included file in the same directory the currently being processed source file lives in. And its tests pass ¯_(ツ)_/¯

[alloy]

Btw, I’m not entirely sure what the proper workflow is on updating the vendored source of CSSLayout, but you might still need to add a commit doing that.

[robhogan]

I'm not exactly sure what to do about the CSSLayout internal changes either, other than that I'm pretty sure they shouldn't stray out of sync so I'll wait for the other PR to be accepted first, and maybe add a commit to this PR with the synchronised changes.

There is a script to sync them but it looks like that's out of date (refers to just the two Layout source+header files) and I guess it wasn't used last time, since the readme has disappeared. Maybe I'll look at updating that too..

And yeah I'm fairly certain it's safe to use CSSLayout internal same-directory includes like the one you highlight, because as you say (assuming those files aren't separated from each other) any sane pre-processor/build config will look at the current directory first when resolving user includes regardless of the context.

[alloy]

cc @javache

[ide]

cc @emilsjolander for review (especially facebook/yoga#217)

[emilsjolander]

Internally we link against css-layout as a seperate library which is why the imports look like they do. We only copy it into the react-native source tree for purpose of open source. I enabled the appropriate header maps in the xcode project to handle the angle bracket includes. I'm not quite sure how cocoa pods works and why it doesn't handle this.

This is not a simple change to make though as it will break internal usage. I'll discuss this will some people from the core team tomorrow to get a sense of how to best solve it. Thanks for the heads up!

Regarding the css-layout diff, i've seen it but have yet had time to test it. I'll get it it tomorrow or the day after :)

[alloy]

Internally we link against css-layout as a seperate library which is why the imports look like they do. We only copy it into the react-native source tree for purpose of open source.

Aha. Makes sense.

Another solution then could be to do the same and ignore the vendored source. I.e. we’d create a podspec for CSSLayout and have the React podspec depend on that.

The only issue with this is, do you always tag a CSSLayout release when using (and thus merging) new CSSLayout code in React Native? If not, that means the dependency would go out of sync.

An added bonus would be that others (outside of RN) can depend on CSSLayout without clashes, but I have no idea if any other libs depend on it, so this might well just be a bonus without real-world benefits.

I enabled the appropriate header maps in the xcode project to handle the angle bracket includes.

I’m slightly confused which setting you’re referring to, as the project already built fine with the supplied Xcode project, no? Could you elaborate on which setting you mean / show a diff?

I'm not quite sure how cocoa pods works and why it doesn't handle this.

Based on what setting it is that you enabled we might be able to do the same from the podspec. In the end CocoaPods just creates a Xcode project and does allow settings to be set, but I can look into that once I hear back from you.

If this ends up working, it’s probably the least invasive solution.

[emilsjolander]

When changing to use library style imports I updated the header maps in the xcode project to reflect this change. See 4e056e9

[robhogan]

Ah, project header search paths would need to be named in the podspec too - cocoapods generates a React.xcconfig from there which is used by the Pods project to specify header search paths for that target. That's why the immediate issue people are seeing in #9014 is a file not found against #import <CSSLayout/CSSLayout.h>.

Adding the appropriate search path doesn't do the trick either though - it allows xcode to find the include but errors Include of non-modular header inside framework module when React is imported as a dynamic framework. AFAIU this isn't a cocoapods issue per se - it's the fact that importing CSSLayout as if it were an external library breaks the modularity of the importer, and prevents React being built as a dynamic framework.

[emilsjolander]

Separate cocoapod for css-layout would probably work. Problem is syncing releases with react-native. Currently every commit from css-layout is automatically synced into react-native.

[alloy]

Adding the appropriate search path … allows xcode to find the include but errors Include of non-modular header inside framework module when React is imported as a dynamic framework.

There’s the CLANG_ALLOW_NON_MODULAR_INCLUDES_IN_FRAMEWORK_MODULES option, but I believe that’s supposed to be set by the target using the library (e.g. the user’s app) and it’s disabled by default for a good reason.

AFAIU this isn't a cocoapods issue per se - it's the fact that importing CSSLayout as if it were an external library breaks the modularity of the importer, and prevents React being built as a dynamic framework.

Yup.

Another fix for this could be to make sure the CSSLayout headers are not referenced in any public React Native headers, because that’s essentially what this error is about, but quickly looking at https://github.com/rh389/react-native/blob/0088298e0ad1baa0c07d48feefa6e4e2bb4c14ce/React/Base/RCTConvert.h and https://github.com/rh389/react-native/blob/0088298e0ad1baa0c07d48feefa6e4e2bb4c14ce/React/Views/RCTShadowView.h that seems rather impractical.

[alloy]

Separate cocoapod for css-layout would probably work. Problem is syncing releases with react-native. Currently every commit from css-layout is automatically synced into react-native.

That’s what I guessed.

Unless you’d ideally like all synced versions to be tagged releases anyways, this might be a bit too invasive. In which case, back to:

This is not a simple change to make though as it will break internal usage. I'll discuss this will some people from the core team tomorrow to get a sense of how to best solve it. Thanks for the heads up!

Is there a simple example you can give how this would break so we can see if that’s fixable in any other way?

[emilsjolander]

Internally we want to continue referencing the separate library instead of the vendored code within react-native. As long as there is a way to continue doing that while using user style imports I would be fine with that.

[alloy]

Internally we want to continue referencing the separate library instead of the vendored code within react-native. As long as there is a way to continue doing that while using user style imports I would be fine with that.

Ah, am I right in understanding that your concern is about the changes in RN’s code (this PR) and not the changes in the CSSLayout code (that PR) ?

If so, we could probably adjust this PR to do something like:

#if __has_include(<CSSLayout/CSSLayout.h>)
#import <CSSLayout/CSSLayout.h>
#else
#import "CSSLayout.h"
#endif

[emilsjolander]

Good idea using pre-processor macros. Yeah that would work just fine. I'm not too worried about the css-layout PR, I just have not had the time to run it locally yet.

[robhogan]

Is it not also an option to add your external CSSLayout location to your USER_HEADER_SEARCH_PATHS when building React?

[alloy]

@rh389 That could probably work too, but I guess it’s simpler for them to have to make less project configuration settings when building internally than more. Do you think there would be a benefit to using that setting over using __has_include?

And also, seeing as you already have the configured test projects, could you see if everything works as expected when using __has_include as shown above?

[emilsjolander]

I'm not sure about using USER_HEADER_SEARCH_PATHS. Might work. But i think @alloy is right here that the less config changes we need to do the better. I don't see why __has_include wouldn't work. If you update the pull request I or somebody else on the team would be happy to test it.

[robhogan]

The only reason I suggest that method is that it keeps the open source version of RN free of concessions to facebook's internal use - in the eyes of an open source contributor it's not obvious what the macro is there for. Purely stylistic though, and it could be explained with a code comment.

I'll try it out now and update the PR if all's well.

[emilsjolander]

Thanks @rh389

[ghost]

@rh389 updated the pull request - view changes

[robhogan]

Looks good, PR updated. Verified:

• From the Xcode project that’s in the repo.
• CocoaPods, as a static library
• CocoaPods, as a dynamic library (framework) in an objc app
• CocoaPods, as a dynamic library (framework) in a Swift app

Someone just needs to verify that it works as desired internally.

Again, this is assuming facebook/yoga#217 is applied first as-is.

[robhogan]

@emilsjolander Have you had a chance to try this out internally?

[javache]

We should try to setup the projects and podspec in a way that makes this import behave like a framework (I see there's some discussion around this in #9014 already). Longer term we want to move open-source to use buck for building too, which is what we use internally and we can solve it at that level.

[javache]

@facebook-github-bot shipit

[ghost]

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

[alloy]

@javache Cool, I can look into setting up a podspec for CSSLayout as a framework soon.

[arthurnn]

what is the fix for this?
I am getting the #9014 issue, and see this wasnt merged. So what should I do?
thanks