-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Unroll some unity files in the nounity build #2376
Conversation
Jenkins Build SummaryBuilt from this commit Built at 20180221 - 09:28:33 Test Results
|
Changes look good to me. Everything builds well for me on mac OS. I'll give a thumbs up after the debug nounity failures on Jenkins are figured out. That's interesting because debug.nounity worked just fine for me on mac OS -- both cmake and scons. You also did us the favor of identifying a bit of dead code. If you'd like to remove that dead code in this pull request you can cherry-pick this commit: scottschurr@f22f907 And, just for a thought for future work, we could consider replacing uses of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, now I see why the debug.nounity builds failed. This pull request is not (yet) sitting atop #2368. Once those includes are in place this commit will build just fine on Jenkins.
Thanks for making this change. 👍.
@scottschurr -- yep, in this case, I wanted to see the failure (to make sure I captured the original deficiency). Now that we have, I can either cherry pick or just wait and rebase. Thanks for your change...I'll grab that shortly. |
I am seeing linker errors with nounity on VS2017. This occurs using the VS integrated cmake and the cmake generated VS solution. The unity builds compile fine.
|
@miguelportilla the nounity builds are expected to fail in some way because we are missing some key includes. I'm surprised it's a linker error instead of compile error, but chalk that up to VC. If you cherry-pick the changes in #2368, I would expect nounity to work. |
@mellery451 Cherry-picking #2368 did not help. Spookyhash is used in the beast hash implementation so I suspect one of the CMakeList.txt changes dealing with beast hash may have something to do with it. Reverting the CMakeList.txt changes (back to 0.90.0-rc1) allows the non unity build to link. |
@mellery451 Last commit fixes the issue. The non-unity build is successfully linking. 👍 |
rebased. this builds properly with the @MarkusTeufelberger |
Also check out #2399 :-) |
FIXES: RIPD-1597 Add includes, remove unused getStackBacktrace() implementation.
Should also fix #2159 by the way. |
Merged in 605ace7 |
This PR makes the nounity (classic) build behave slightly more correctly by unrolling a few more unity sources. I expect the nounity builds to fail on this branch without the changes in #2368, so this change should only be merged along with or after PR2368 (which is already passed and low risk).