Fix nightly build, add extra PR testing #1490
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adds carthage, cocoapods and deployment jobs to PR workflow
|
The CocoaPods issue is odd. Linting fails on with the --use-library argument. The build output is noisy. A LOT of warnings. Here's where ParseFacebookUtils-Dynamic-iOS fails, with some context: So the FBSDKCoreKit import appears to fail. Not sure why. |
|
I removed all dynamic library links and re-added. Tested the building of each one and everything seems to build just fine now. Committed my changes. |
|
Cocoapods could be an issue with dependency versions on the Podspec. Haven't looked at that. Also, if I'm building with cocoapods then linking is different than with carthage. One we pull from the Carthage builds folder. The other would be included in the pods so we shouldn't be building the dynamic from the cocoapods. That defeats the purpose of cocoapods to begin with is that it loads its dependencies. |
The changes you pushed do more than you describe here - there's much more removed in this change than there is added. It looks like you also pruned some of the system's frameworks from the project. As far as I can tell the net result of these changes is that the cocoapods build error (FBSDKCoreKit module is not found when building ParseFacebookUtils) is now exposed in the main build log, rather than being buried under a meaningless 'something failed' message. And this is definitely a good thing. But do how do we know which of the changes caused this result? I, at least, am not sure. It's good practice to make small, controlled changes to the codebase that alter the system behaviour (and the test results) in understood and controlled ways. Otherwise fixing and learning become very, very hard. And in order to understand the effects of our changes, it's vital that we test locally and understand the effects of our work before we push. As an example, the few changes I pushed to start this PR, I'd worked on for a day or 2 before pushing. I started trying a whole load of things. And when I got the result I was looking for, I started removing my changes until I found the smallest changeset that fixed the issue (Carthage build) before pushing. |
|
As always, not going after you personally here, just trying to share what many years of making my own mistakes has taught me :) |
|
You are correct about the additional lines removed. Pretty sure I still didn’t get them all, but what ones I did update I removed unnecessary lines to system frameworks that caused warnings since you can’t link system frameworks during the build process anyway Didn’t think to note it because they shouldn’t have been linked to begin with. So I removed all linked frameworks period and then added the Facebook frameworks alone back the right way. |
|
In this case I’d didn’t see the need to do an incremental change since linking those system libraries has never worked to begin with. Only caused warnings. |
|
Sure. And I thought about removing them myself when I fixed the Carthage build here; the build setting I changed makes manually linking system frameworks unnecessary. It's better imho to have a separate 'cleanup' commit once things are tested and shown to be fixed. That way we can be more sure that the cleanup doesn't affect anything. I don't know how many times I've tweaked something I was sure was harmless, unused, or isolated, only to then have to spend the next day or so fixing random broken stuff 😃 |
|
That probably still needs to happen. Figured I’d clean it all up for the targets that where experiencing linking issues. I think what issues we have left will be resolved with catching up the Podspec with the Carthage dependency versions. |
|
Actually looking at the podspec, FBSDKCoreKit is just plain missing 😃. I've added it and I'm testing now. EDIT: Didn't help, it's already a dependency of FBDSKLoginKit 😕 |
|
at some point we also need to figure out why we are doing a cocoapods build on both Travis and circle. Seems a bit redundant. |
|
Looking at the podspec wouldn’t listing all the system frameworks for each targets s.frameworks add the same warnings during the cocoapods build. |
|
There don't seem to be any warnings related to those in the build... but I'll be honest I didn't notice the warnings they were producing from the links in the project. FWIW the currently failing part of the test boils down to executing this: |
|
Looking at our rake file. I see So seems we are building this twice one using libraries the other not. Is that intentional you think? |
|
I do think it's intentional. I think one relates to our static targets while the other relates to dynamic. Currently the --using-libraries lint fails, while the other passes. |
|
Maybe I messed this up. So I relinked everything based on where the Facebook core and login frameworks are located for Carthage. Those locations would not be the same for cocoapods. Perhaps we need to re-examine that. |
|
I'm not sure I understand; could you dive deeper into that? It doesn't seem to have affected the build negatively as of right now. |
|
That said, the links to the FB libraries in the Vendor folder probably do need to be there:
|
|
... or maybe they don't. Perhaps adding ./Vendor to the framework search path does the same job. 🤷🏽♂️ But this does serve to confuse things (and possibly create new issues), since it's a change that may affect other things but doesn't seem to move us closer to the goal. |
|
So in order to build dynamic targets for Carthage Dependencies. I linked the Facebook frameworks from Carthage/Build/iOS. Same for tvOS. The issue with this is cocoapods when using dynamic frameworks doesn’t output dependencies to that same folder. |
|
Found an article that better explains the issue and a little bit about how to fix it. https://ppinera.es/2017/09/13/xcodembed/ |
|
Oh wait! Never mind. I thought this was a linking issue. The podspec just didn’t pass validation. |
|
Yes 😃 But it fails validation because it can't compile; it can't find the FB dependency library. |
|
Wait so this is happening with —use-frameworks set? |
|
Ok I can’t take it anymore. I’m going to get my computer booted instead of looking through logs |
Yes. And not without it. 🙂 |
|
Ok I figured it out. The new version of xcodebuild will build all targets in parallel making headers not available from FBSDKCore when FBSDKLoginKit is being built. Adding --use-modular-headers should fix it. |
|
@drdaz Check it out our cocoapods test passed! That one change fixed the whole thing. Now lets get this merged and well run the nightly build. |
|
@noobs2ninjas That's awesome! Great find! But why are the tests cancelled? Gotta see that they aren't broken by this change too (as unlikely as that may be). |
TomWFox
approved these changes
Apr 7, 2020
|
Sorry I cancelled it. CircleCI just takes forever to run through all the tests and you cant prioritize them. Rest assured, it is literally impossible for my change to break anything else. Only two lines that have changed since last commit and now cocoapods works. |
|
I manually re-ran the tests and they did as expected 😉. But why oh why does the nightly cocoapods still seem to fail the same as it did before these changes?? EDIT: It doesn't; I got confused by multiple mails about the nightly. But Carthage still fails 🧐 |
|
Just ran the Carthage test target here... and it passed 😬🤷🏽♂️ |
|
I just realised. I didn't wake up to a mail about the nightly failing this morning! 🎉🍾🎊 |
|
That’s absolutely maddening. We changed absolutely nothing. |
|
Heh... I guess that's another of our inconsistent tests. |
|
Its been working for days now. So, as far as I'm concerned this is fixed. :) Well done! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The nightly build is still broken, and our PR testing is insufficient to prevent these types of surprises.
This PR fixes the nightly, and adds testing to PRs so we can more easily see whether our changes will break builds on master, and should help keep master and it's tests in a more stable state.