[Yoga] Update to the current framework version (1.3.0) requires changing import path. about texture HOT 15 CLOSED

@appleguy We would appreciate your help on this issue to fix this. I think the right way to fix this issue is to to change the way Yoga is currently integrated. The current way can be a problem for people that are using Texture and have Yoga as another pod defined. They don't necessarily would like to use the Yoga integration within Texture, but maybe use it somewhere else in the app.

We have to provide a way to use Texture without automatically enabling Yoga, if it's not explicitly defined somewhere, so just enabling it by looking at a specific path is not feasible anymore.

I would appreciate and rely on @appleguy to look into this issue and provide a fix sooner than later. Thanks!

from texture.

from texture.

@weibel hey there - could you take a look at this change and help me reproduce the issue?

#91

I tried building the examples/ASDKLayoutTransition project, which includes Yoga as a dependency in its Podfile, and I wasn't able to reproduce a build issue.

Is it possible that you have a case-sensitive filesystem on your computer? I would be happy to change the Yoga import if it has now become lowercase, if that fixes the issue for you.

from texture.

Oh - I downloaded Xcode 8.3 when it came out, but it looks like it didn't get installed. That's probably the cause, as the description pointed out.

@maicki your description is helpful as well; I'm not sure this reporter is experiencing any issues from the Yoga integration being enabled, though. I think all of our other integrations, like PINRemoteImage and IGListKit, function in the same way...as long as the runtime characteristics can never be changed by the existence of the Yoga framework without the user adding calls to #if YOGA-gated API definitions, is there any trouble with using __has_include?

I'd expect that the rename to a lowercase yoga/ is a one-time change for the framework, so I'm not too worried about fixing that. I also know that the gated code can't affect a user unless they engage with yoga-specific APIs, so I think we will be safe after I confirm the capitalization fix.

from texture.

@weibel I still wasn't able to hit the compiler warning after updating to 8.3.1. Let me know how you're able to reproduce - either send over a small test project, or see if you can modify the example project I mentioned to make it occur.

from texture.

@appleguy Yeah you are right all of our integrations changed to this way. It used to be that all our integration except Yoga were enabled by CocodaPods or Carthage if specifically defined via a subspec. Now all integrations look for the include path.

The problem I see in this integrations is: if users are using Texture, don't want to use this integration but e.g. use a different version of the library within a different part of the app. They declare Texture main spec in the Podfile and explicitly declare e.g. Yoga in there too with a version that is not supported yet in Texture (maybe it's older or newer ...). As we only look for the include header we will assume they would like to use Yoga in there, but that is not the case and compiler errors will happen as our integration is using either a too old or too new API.

cc @garrettmoon @Adlai-Holler See this conversion as I think it becomes a more general problem how we handle other libraries.

from texture.

OK, that makes sense. I will be very cautious about this, and we should consider if we want some other kind of feature flag setup.

For Yoga specifically, the good news is that it is very unlikely that they will make API-breaking changes regularly. I doubt they even realized the capitalization thing could cause issues. So, as long as the Yoga support in the framework only uses APIs they had in 1.0, both forward and backward compatibility is likely to work smoothly. That's especially true for clients not using the node-based Yoga APIs / who happen to just be using it in another part of the app — the only risk is if we don't build.

We should probably create another issue or design discussion around the right way to control framework dependencies (Yoga included), and meanwhile I'll try to find a way to reproduce this problem and make sure it is fixed.

from texture.

@appleguy I've tried to reproduce the issue on the demo project, but had no luck. This week I'll work on our integration of Texture, and hope to get to the bottom of what's going on.

from texture.

@weibel awesome, thanks! Please do check if you have a Case Sensitive filesystem -- this could be a real issue, but possibly only reproduces on case sensitive. I would of course still want to fix it if that's what is going on.

Note that Yoga 1.4.0 is now out (although it is not up on Cocoapods), so you might want to try with that as well.

from texture.

@weibel ping - let me know if you were able to get past this issue. Even though you didn't see it in the test project, is it still an issue in your main one?

from texture.

@weibel could you try the latest Yoga? It is now on 1.5.0 (even on cocoapods - https://cocoapods.org/pods/Yoga).

If you could close out this task in the event you can no longer reproduce, that would be helpful. If you can reproduce, I'm still eager to get this fixed up. Thanks for your help!

from texture.

I upgraded to the new Yoga, but the problem remained so I have dug a bit into it.

In our project we treat warnings as errors, so I think that's the reason I can't compile.
It's rumoured that the -Wno-nonportable-include-path warning has been included in Xcode in preparation for the APFS file system which is case sensitive. When APFS is rolled out these warnings will become errors, so they should be dealt with before then
Some discussion here http://clang-developers.42468.n3.nabble.com/case-insensitive-include-warning-td4050906.html

In my podfile I tried with inhibit_all_warnings! to no avail. So I'll tinker a bit with the warning settings

from texture.

Related: #25

from texture.

#91 did not fix the issue I had. I have made this PR to show what was needed to make our particular project compile #306

from texture.

@weibel Awesome, thank you for the PR! I think we are close to a fix. Check out my comments and let me know what you think. Apologies for the latency in responding.

from texture.