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

Propagate swift version / copts #85

Merged
merged 1 commit into from
Oct 18, 2018
Merged

Propagate swift version / copts #85

merged 1 commit into from
Oct 18, 2018

Conversation

rahul-malik
Copy link
Collaborator

No description provided.

@jerrymarino
Copy link
Collaborator

@rahul-malik - Awesome! This seems to be causing a CI issue:

/Users/travis/build/pinterest/xchammer/IntegrationTests/Sandbox/UrlGet/Vendor/Stripe/Stripe/NSError+Stripe.h:11:9: fatal error: 'StripeError.h' file not found
#import "StripeError.h"
        ^~~~~~~~~~~~~~~
1 error generated.

It's not immediately clear why..

@jerrymarino
Copy link
Collaborator

@rahul-malik it'd also be good to get a sample project with swift in it, so we can make sure everything is working correctly.

@@ -161,6 +161,8 @@ struct XCBuildSettings: Encodable {
var moduleMapFile: First<String>?
// Disable Xcode derived headermaps, be explicit to avoid divergence
var useHeaderMap: First<String>? = First("NO")
var swiftVersion: First<String>?
Copy link
Collaborator

Choose a reason for hiding this comment

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

@rahul-malik is this the correct data type in Xcode?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep it is!

@@ -211,6 +215,7 @@ struct XCBuildSettings: Encodable {
var container = encoder.container(keyedBy: CodingKeys.self)

try container.encode(copts.joined(separator: " "), forKey: .copts)
try container.encode(swiftCopts.joined(separator: " "), forKey: .copts)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@rahul-malik did you intend to set OTHER_SWIFT_FLAGS here? This LOC overwrites OTHER_CFLAGS

settings.swiftVersion <>= First(swiftVersion)
case .swiftc_opts:
if let coptsArray = value as? [String] {
let processedOpts = coptsArray.map { opt -> String in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to patch these for bazel-genfiles ( and for external in a followup ). We should have an example of a swift genfile in a swift_library in a sample. Consider either adding this to UrlGet or just creating a new project where we can add all the swift edge cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably patch this for external with the PR that handles external repos. Adding a new example is a good idea

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lets do this in a follow up PR

@jerrymarino
Copy link
Collaborator

Hey @rahul-malik - this one LGTM, mostly. Would you add an example with some .swift files? To create a new sample and test, the Frankenstein app is a pretty minimal starting point.. It'd be good to exemplify this into a sample where we can add all relevant swift cases.

@rahul-malik rahul-malik force-pushed the rmalik-swift-opts branch 3 times, most recently from 8614324 to dca0af2 Compare October 18, 2018 19:10
- Adds support for SWIFT_VERSION, and OTHER_SWIFT_FLAGS (swift copts)
- Add Tailor project to test suite
@rahul-malik rahul-malik merged commit 20a6ce0 into master Oct 18, 2018
@jerrymarino jerrymarino deleted the rmalik-swift-opts branch October 18, 2018 20:40
}
settings.swiftVersion <>= First(swiftVersion)
case .has_swift_dependency, .has_swift_info:
guard let swiftVersion = try? ShellOut.shellOut(to: "xcrun swift package tools-version") else {
Copy link
Contributor

Choose a reason for hiding this comment

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

This only works if there's a Package.swift, which not all projects will have. I've opened an issue: #124

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.

3 participants