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

build: Import CocoaAsyncSocket using Carthage #350

Merged

Conversation

qmfrederik
Copy link

This is an alternative to #349, but as I mentioned it gives a compile-time error and I'm not sure how it can be suppressed (other than fixing it upstream)

@KazuCocoa
Copy link
Member

oops, I merged #349

@mykola-mokhnach
Copy link

@KazuCocoa no problems, simple create another PR to revert it

@KazuCocoa
Copy link
Member

🆗
I've created a PR to this PR https://github.com/qmfrederik/WebDriverAgent/pull/2/files
On my local, build succeeded with the change cc @qmfrederik

@qmfrederik
Copy link
Author

@KazuCocoa Thanks, I merged your PR. It still fails in CI, though

▸ Compiling RoutingHTTPServer.m

❌  /Users/runner/runners/2.170.1/work/1/s/Carthage/Build/iOS/CocoaAsyncSocket.framework/Headers/CocoaAsyncSocket.h:9:1: use of '@import' in framework header is discouraged, including this header requires -fmodules [-Werror,-Watimport-in-framework-header]

@import Foundation;
^



❌  /Users/runner/runners/2.170.1/work/1/s/WebDriverAgentLib/Vendor/RoutingHTTPServer/RoutingHTTPServer.h:20:9: could not build module 'CocoaAsyncSocket'

#import <CocoaAsyncSocket/GCDAsyncSocket.h>
 ~~~~~~~^

Are you using an older version of Xcode?

@KazuCocoa
Copy link
Member

mm, I used Xcode 11.3. Let me try an older one

@qmfrederik
Copy link
Author

I think the error is specific to Xcode 11.4. See this link: https://developer.apple.com/documentation/xcode_release_notes/xcode_11_4_release_notes?language=objc

@KazuCocoa
Copy link
Member

KazuCocoa commented Jun 12, 2020

quamotion#3 could be a workaround...
I was able to build this on Xcode 11.4.1

@KazuCocoa
Copy link
Member

KazuCocoa commented Jun 12, 2020

Note:

For Xcode 11.4+, we can specify -Wno-atimport-in-framework-header in Configurations/ProjectSettings.xcconfig to avoid the warning as qmfrederik#3 (since we should avoid the warning in Frameworks/CocoaAsyncSocket.framework)

For less than 11.3, we should not specify the flag since clang raises an error, invalid flag.

If we found a way to handle the flag dynamically, this PR is ready, I believe.

@KazuCocoa
Copy link
Member

KazuCocoa commented Jun 12, 2020

Like https://github.com/qmfrederik/WebDriverAgent/pull/3/files , when I added -Wno-atimport-in-framework-header as a flag for the xcodebuild, it was able to build for both 11.4+ and 11.3-.
We need to update documentation about building manually (add below), but it seems reasonable with Carthage so far.

#if __has_warning("-Watimport-in-framework-header")	
#pragma clang diagnostic ignored "-Watimport-in-framework-header"	
#endif

@mykola-mokhnach
Copy link

Cannot we simply push a PR to https://github.com/robbiehanson/CocoaAsyncSocket ?

I hope @robbiehanson would be happy to merge it ASAP

@qmfrederik
Copy link
Author

I've created robbiehanson/CocoaAsyncSocket#723. Let's see how it goes.

@KazuCocoa
Copy link
Member

Updated with the CocoaAsyncSocket fix
quamotion#3

@@ -7,7 +7,11 @@
* of patent rights can be found in the PATENTS file in the same directory.
*/

#import "GCDAsyncSocket.h"
#if __has_warning("-Watimport-in-framework-header")

Choose a reason for hiding this comment

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

I would say this is not necessary anymore after GCDAsyncSocket bump

Copy link
Member

@KazuCocoa KazuCocoa left a comment

Choose a reason for hiding this comment

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

yey

@mykola-mokhnach mykola-mokhnach merged commit a77e7bc into appium:master Jun 13, 2020
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