Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Yoga] Refer to proper path name and use module import #306

Merged
merged 3 commits into from
Jun 7, 2017
Merged

[Yoga] Refer to proper path name and use module import #306

merged 3 commits into from
Jun 7, 2017

Conversation

weibel
Copy link
Contributor

@weibel weibel commented May 24, 2017

Fixes #25

@CLAassistant
Copy link

CLAassistant commented May 24, 2017

CLA assistant check
All committers have signed the CLA.

@ghost
Copy link

ghost commented May 24, 2017

🚫 CI failed with log

@weibel weibel changed the title Refer to proper path name and use module import Yoga: Refer to proper path name and use module import May 24, 2017
@weibel weibel changed the title Yoga: Refer to proper path name and use module import [Yoga] Refer to proper path name and use module import May 24, 2017
Copy link
Member

@appleguy appleguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weibel thanks for putting up this PR! I would be happy to approve the ASAvailability.h change if it is helpful to land that by itself. I'm not sure we should take the @import yoga change, though. Any other details you have here would be appreciated.

@@ -33,7 +33,7 @@
// If Yoga is available, make it available anywhere we use ASAvailability.
// This reduces Yoga-specific code in other files.
#ifndef YOGA_HEADER_PATH
#define YOGA_HEADER_PATH <Yoga/Yoga.h>
#define YOGA_HEADER_PATH <yoga/Yoga.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change definitely seems correct, and we should merge it.

@@ -21,7 +21,7 @@
#import <AsyncDisplayKit/ASEventLog.h>

#if YOGA
#import YOGA_HEADER_PATH
@import yoga;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to avoid this change if possible. Does your error still occur if this is not changed? Can you screenshot the error / warning that you get without this change, but with the HEADER_PATH change?

The reason we'd like to avoid this is that some build systems do not support @import. It would be the only usage in the entire framework, and I believe it should be possible to make this work reliably without using @import.

@appleguy
Copy link
Member

appleguy commented Jun 6, 2017

@weibel I'd love to hear back from you on this one, but for now I've removed the module change (since it breaks some build systems including the one I'm using).

I think the other change should be sufficient to fix your issue as it corrects the capitalization problem, but please let us know what the error is that you're seeing if this doesn't work.

@ghost
Copy link

ghost commented Jun 6, 2017

1 Warning
⚠️ Any source code changes should have an entry in CHANGELOG.md or have #trivial in their title.

Generated by 🚫 Danger

@appleguy appleguy merged commit 786ac15 into TextureGroup:master Jun 7, 2017
bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request Apr 25, 2018
)

* Change header path to fix TextureGroup#25

* Use module import

* Update ASDisplayNode+Beta.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants