Allow builds to skip iOS and watchOS device SDKs when the codesigning identity is not installed #583
Conversation
…OS device builds. The "iphonesimulator" and "watchsimulator" platform values also skip the device SDKs
|
Should I make any changes to this guy? |
|
Most of the core contributors have been pretty busy recently. I'll try to take a look at this soon. |
|
Anything I can do to help with this review? |
|
I started reviewing all the open PRs over the weekend. I hope to get to this sometime this week. Sorry for the delay! |
|
Thank you for opening a pull request! And sorry it took me so long to look at this. It's been a busy month. 😄 However, as discussed in #281, we're not interested in adding a command-line flag for this. Carthage should build for device if it's able and automatically build only for the simulator in the case where code signing identities haven't been set up. |
|
Ok, and what about specifying "iphonesimulator" or "watchsimulator"? In master, those have no semantic difference from "iphoneos" or "watchos" – should I keep my change to treat those as simulator-only? |
|
No, I don't think that's something that Carthage should be concerned with. |
…he machine has any iOS or iPhone signing identities configured, rather than relying on a command line flag. It's calling the command line tool "security" to list the signing identities. At present, the "should build for all platforms" test case is failing (on my machine), since it's expecting the ARM architectures, and I don't have any iOS signing identities. I'm not sure what the correct way to isolate that configuration is with ReactiveCocoa
|
@mdiep I updated (and simplified) the pull request to check with the "security" command's output, and only build for devices if an iOS signing identity is present. Can you please take a look? Also, this is my first time writing ReactiveCocoa code, so any constructive feedback is greatly appreciated. |
|
That's great! Thanks for tackling this! I'll add this to my todo list and try to review it in the next few days. |
…ing identities installed, rather than many errors that are less clear
| } | ||
| |> map { (identityLine: String) -> String? in | ||
| var error: NSError? = nil | ||
| let regex = NSRegularExpression(pattern: "\".+\"", options: nil, error: &error) |
There was a problem hiding this comment.
I'd really like to see this split out into a private static variable, like targetSettingsRegex above.
It'd also be nice for it to be a bit more specific. I think something like this would work:
^\s*\d\)\s+\w+\s+"(.+)"$
Note the use of a capturing group for the actual name of the identity. Bonus points if you use AllowCommentsAndWhitespace to separate the comments to make it a little clearer. 😄
|
Thanks again for tackling this! ✨ I left some higher level notes above, but I think you're on the right track here. 👍 The other thing that I'd like to see is for Carthage to print a warning if it falls back to a simulator only build. |
…eparated out command line output into separate method)
…s output. Opted to use Swift comments instead of regex ones, since I would have had to insert a \n for each line
|
Alright, I've taken care of cleaning up the regex stuff, but now I'm left to the hard part and can use some guidance. The only way I see to get any settings from the project in Carthage is through I think we need to get the |
|
With Will that do what we need? |
|
Thanks for telling me it worked for you – allowed me to find my bug and now I'm seeing |
… string, and started using that, in combination with the CODE_SIGN_IDENTITY build setting, to determine whether a build is possible for each SDK
|
@ikesyo I'll let you give this another look. You can merge when you're satisfied with it! Thank you for reviewing. ❤️ |
|
Looks good now! I'm happy to merge the great work of @abbeycode and @mdiep 💖 |
Allow builds to skip iOS and watchOS device SDKs when the codesigning identity is not installed
|
Thanks for the pull request, @abbeycode! And thank you for being patient through the review process!! |
|
No problem, it was a great learning experience. When do you expect the next release to be cut? |
|
I plan to do a release sometime this week. |
|
👍 |
|
@mdiep Could you update this thread when a new release is cut so we can make a new homebrew release? |
|
I just released 0.9 Sad Effects. 🎉 |
|
@mdiep Can I ask you to release 0.9.0 on Homebrew? Been looking forward to this release for several months. |
|
I have made the PR for Carthage 0.9 in Homebrew Homebrew/legacy-homebrew#43884 |
|
@abbeycode Could you take a look at #776. I think that that bug was introduced by this patch. |
|
It seems like Homebrew/legacy-homebrew#43884 has stalled a bit (but it's on the home stretch). What's holding it up at this point? |
|
Was this feature released? |
|
@AAverin It was released, and you don't need to do anything to use it. It skips device SDKs (only building for the simulator) when there are no signing identities present. |
This addresses issues #576, #235, and #281, by adding a new
--simulator-onlyflag, and also properly treating theiphonesimulatorandwatchsimulatorvalues for the--platformflag.I added associated value for
PlatformandBuildPlatformattached to the.iOSand.watchOSvalues, in addition to passing asimulatorOnlyparameter around between functions. After adding the associated values, I also had to implement the==overload to keep the enumsEquatableand allow them to be used inswitchstatements.