Skip to content
This repository was archived by the owner on Oct 30, 2018. It is now read-only.

Declare framework dependencies in podspec to enable compiling with ccache#666

Merged
dzenbot merged 1 commit intoslackhq:masterfrom
ButterflyNetwork:podspec-dependencies
Aug 17, 2018
Merged

Declare framework dependencies in podspec to enable compiling with ccache#666
dzenbot merged 1 commit intoslackhq:masterfrom
ButterflyNetwork:podspec-dependencies

Conversation

@jgavris
Copy link
Copy Markdown
Contributor

@jgavris jgavris commented Apr 12, 2018

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've been mindful about doing atomic commits, adding documentation to my changes, not refactoring too much.
  • I've added a descriptive title and added any useful information for the reviewer. Where appropriate, I've attached a screenshot and/or screencast (gif preferrably).
  • I've listed my changes on the [Changelog(https://github.com/slackhq/SlackTextViewController/blob/master/CHANGELOG.md) file.
  • I've read, agree to, and signed the Contributor License Agreement (CLA).

PR Summary

Users may want to build the pod with Clang modules disabled -fno-modules because they are using ccache which doesn't support -fmodules. When modules are disabled, auto-linking of framework modules is also disabled, so these dependencies need to be expressed explicitly.

@jgavris jgavris force-pushed the podspec-dependencies branch from 92f8392 to 91ea8bb Compare April 12, 2018 18:25
@jgavris
Copy link
Copy Markdown
Contributor Author

jgavris commented Apr 27, 2018

Ping (perhaps @dzenbot) does this reasonable? Aiming to get all my C-language pods ccache compatible.

@jgavris
Copy link
Copy Markdown
Contributor Author

jgavris commented May 24, 2018

👋 friendly bump

@jgavris
Copy link
Copy Markdown
Contributor Author

jgavris commented Jun 26, 2018

@dzenbot friendly bump

@jgavris jgavris force-pushed the podspec-dependencies branch 2 times, most recently from 0c81e50 to 7e72fe5 Compare July 19, 2018 03:02
@jgavris
Copy link
Copy Markdown
Contributor Author

jgavris commented Jul 19, 2018

@dzenbot We'd love to integrate this without forking it, any luck?

@jgavris jgavris force-pushed the podspec-dependencies branch 2 times, most recently from 1f1ea41 to 51fafe4 Compare July 19, 2018 17:02
Copy link
Copy Markdown

@dzenbot dzenbot left a comment

Choose a reason for hiding this comment

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

Looks good, but please remove the changes from Source/SLKTextViewController

Comment thread Source/SLKTextViewController.m Outdated
[self cancelAutoCompletion];
}

- (void)acceptAutoCompletionWithAttributedString:(NSAttributedString *)attributedString keepPrefix:(BOOL)keepPrefix
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This seems like a bad conflict. This method is no longer on the code base.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Whoops, this is from our fork. I'll remove!

    Users may want to build the pod with Clang modules disabled `-fno-modules` because they are using `ccache` which doesn't support `-fmodules`. When modules are disabled, auto-linking of framework modules is also disabled, so these dependencies need to be expressed explicitly.
@jgavris jgavris force-pushed the podspec-dependencies branch from 51fafe4 to 222c9dc Compare August 17, 2018 17:33
@jgavris
Copy link
Copy Markdown
Contributor Author

jgavris commented Aug 17, 2018

@dzenbot updated

@dzenbot dzenbot merged commit 2761b4e into slackhq:master Aug 17, 2018
@jgavris jgavris deleted the podspec-dependencies branch August 17, 2018 18:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants