Full Cotire Support by cwfitzgerald · Pull Request #142 · GameFoundry/B3DFramework

cwfitzgerald · 2018-06-04T00:12:15Z

This set of patches enables full cotire support. There are sill problems with building with cotire on mac, which are still to be fixed, but I am unable to fix them due to me not having a mac at all.

This also enabled -fdiagnotisics-color=always on ninja builds.

This resolves #139.

BearishSun · 2018-06-04T13:28:45Z


+## Set files disallowed in precompiled headers
+set_directory_properties(PROPERTIES
+    COTIRE_PREFIX_HEADER_IGNORE_PATH "${CMAKE_CURRENT_SOURCE_DIR};/usr/include/clang")


Is this still required since we have more robust search for inttypes.h?

cwfitzgerald · 2018-06-04T13:31:46Z

Nope, just forgot to remove it. I'll remove it when I get to work. --- Connor Fitzgerald Sent from my Phone. Please excuse my brevity.

…

On June 4, 2018 9:28:47 AM EDT, Marko Pintera ***@***.***> wrote: BearishSun commented on this pull request. > @@ -82,6 +82,10 @@ set(PHYSICS_MODULE_LIB bsfPhysX) configure_file("${BSF_SOURCE_DIR}/CMake/BsEngineConfig.h.in" "${BSF_SOURCE_DIR}/Foundation/bsfEngine/BsEngineConfig.h") configure_file("${BSF_SOURCE_DIR}/CMake/BsFrameworkConfig.h.in" "${BSF_SOURCE_DIR}/Foundation/bsfUtility/BsFrameworkConfig.h") +## Set files disallowed in precompiled headers +set_directory_properties(PROPERTIES + COTIRE_PREFIX_HEADER_IGNORE_PATH "${CMAKE_CURRENT_SOURCE_DIR};/usr/include/clang") Is this still required since we have more robust search for `inttypes.h`? -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #142 (review)

BearishSun · 2018-06-04T13:33:49Z

 #include "Scene/BsSceneManager.h"

+// Do not delete this, it prevents massive problems with windows headers across the codebase
+#include "Private/Win32/BsWin32Undefs.h"


Having a private header in a public header will not work since private headers don't get installed, so it will break in binary distributions. Also, I'd prefer to move this to a more generic location - perhaps even replacing all usages of "windows.h" with a custom header that automatically undefines the problematic macros?

Isn't this a public source file anyway?

Either way, I agree that replacing windows.h with a proxy header would be better. I'll get on that.

Ah sorry, I thought this was BsCamera.h, my bad.

Cotire only walks source files to build the PCH list, (though it gets transitively included ones) so the best way (besides doing what you suggested) is to have it included directly in a source file.

BearishSun · 2018-06-04T13:38:54Z

+# Force unity builds to isolate these files into their own section. 
+# COTIRE_START_NEW_UNITY_SOURCE causes the source file to be the first one included in a new file. 
+# This isolates the X11 headers which define inane things.
+set_source_files_properties ("bsfCore/Private/Linux/BsLinuxPlatform.cpp" PROPERTIES


Could this be something we do for Windows as well, to avoid the issues with "windows.h"? If so could we just automate the process and have cmake iterate over BS_CORE_SRC_PLATFORM_LINUX (and other platform-specific) files and ignore them all? I don't mind those files being excluded from the unity builds (and I'm assuming their headers are also excluded from PCH), if it means for a cleaner code without duplication.

Yes, I haven't tested unity builds on windows yet as I didn't get as much time on my desktop as I had hoped.

The reason I am hesitant to just mark this so cotire ignores it is that it would illuminate some of the purpose of the unity builds. By keeping all the X11 comparable headers in one place they get a single unity build together causing at least some level of unity-ness.

I'm worried about things breaking in weird ways for someone unfamiliar with the codebase.

i.e. just including a X11 header in some other Linux specific file will break the build (I'm assuming) - and likely in not a very obvious way. I don't like having such hidden requirements in the codebase, if there's a way to avoid them.

I'm torn, as the X11 header is possibly the worst header I've ever seen besides <windows.h> so I want to shun it's usage as much as possible, but I agree that we shouldn't leave the build breaking randomly.

I think I will let these ones out of the unity build.

BearishSun · 2018-06-04T13:47:55Z

Thanks for the PR, look good aside from the stuff I commented on. I'll take a look at what's wrong with macOS as soon as I get a chance.

Btw, is there a way to disable cotire messages in CMake log (e.g. cotire was enabled for target ...)?

Also, I don't see cotire.cmake included in the PR, but it is present in the examples PR. Was it disabled for the plain library build for a reason or?

cwfitzgerald · 2018-06-04T14:48:17Z

There doesn't seem to be a way to disable cotire messages, but if we're going to host cotire ourselves, we can just remove the message. It's here: https://github.com/sakra/cotire/blob/master/CMake/cotire.cmake#L2941

The reason I didn't include cotire in the library is that I thought you would want to have the default build not change, but have the option open if you could (it's less aggressive for me to say "hey you need to use this thing in your build" vs "hey would it be possible for me to put a thing to optionally use this thing in your build"). But if it is okay with you, I will go ahead and add cotire to this as well.

BearishSun · 2018-06-04T14:54:51Z

Having the ability do disable it would be nice (at least until we guarantee it works perfectly for everyone - this way I could even merge the PR earlier). So we include cotire, but add a ENABLE_COTIRE CMake option, disabled by default (for now)?

cwfitzgerald · 2018-06-04T15:00:30Z

Sounds good to me.

cwfitzgerald · 2018-06-04T16:35:47Z

Should be good now minus issues with windows unity builds and mac everything.

cwfitzgerald · 2018-06-04T16:57:23Z

There's issues with the windows.h solution....

I'll deal with it at home when I have all my resources with me.

BearishSun · 2018-06-04T19:39:19Z

I've taken a quick look. One of the issues is simple, BsWindows.h includes itself instead of <Windows.h>.

But another issue is that undefining near and far seems to screw up some other Windows headers. I'd be up for completely excluding Windows.h from PCH, but I haven't yet found a good way of locating it.

cwfitzgerald · 2018-06-04T20:05:52Z

There's already a function that locates windows.h, you can just ask cmake to find it. I would like to keep Windows.h (and it's giant size) in the PCH if possible. I'm going to be home and can actually debug it on windows in a bit.

BearishSun · 2018-06-04T20:09:28Z

That function doesn't find the header, at least not on my end.

I do wonder if Windows.h maybe even slows down the build ultimately, because of its giant size? Since we are including it in just a few files without PCH, but with PCH we are including it in every single file. Even in its precompiled format, it might be ultimately slowing things down. But I haven't profiled it.

BearishSun · 2018-06-04T20:19:48Z

While we're at it, some other Windows headers are only included in one single file (or perhaps a couple). Not sure if cotire detects that and can exclude it from PCH in that case? Would be nice to disable those as well and test build times.

But that's not necessary for this PR, just as a potential future improvement.

cwfitzgerald · 2018-06-04T21:07:49Z

I'll see what I can do about it, it will probably require another workaround like we did for clang. As for the other headers I can look into the common ones which are isolated in a couple files and detect them too. --- Connor Fitzgerald Sent from my Phone. Please excuse my brevity.

…

On June 4, 2018 4:19:50 PM EDT, Marko Pintera ***@***.***> wrote: While we're at it, some other Windows headers are only included in one single file (or perhaps a couple). Not sure if cotire detects that and can exclude it from PCH in that case? Would be nice to disable those as well and test build times. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #142 (comment)

cwfitzgerald · 2018-06-04T23:37:41Z

The problem is that right now I don't have a reliable way of detecting where windows.h is, as it works on my windows 10 machine. I may be able to try something with /showIncludes and parsing the output of that using good old regexes. I'm not sure exactly which ones include windows.h by transitive dependencies.

cwfitzgerald · 2018-06-05T05:08:19Z

Alright, I think I have everything down... except getting cotire to actually leave the header out. I am still fighting with it to see what the correct combination of slashes or whatever to get cotire to realize that we're talking about the same header.

BearishSun · 2018-06-05T09:50:27Z

Such a huge hassle for a single header :(

I did notice that cotire added a full path to windows.h in the precompiled header file, so I'm guessing it has some internal mechanism for locating it? Might be worth trying to find it. Although you seem to be nearly there anyway.

Another idea I had was to modify cotire so we can filter includes based on their name, instead of the full path.

cwfitzgerald · 2018-06-05T14:44:14Z

FOUND THE PROBLEM! Something with the whole X11 bit being on windows too was causing it to fuck up. I'm going to see what the final resolution of all this will be.

cwfitzgerald · 2018-06-05T15:48:25Z

I was wrong about the first solution, turns out I was searching for windows.h, finding it, then trying to get it to block the inclusion of Windows.h. CMake is case sensitive.... currently works, but the compiler can't find the .dll's it needs.

cwfitzgerald · 2018-06-05T17:31:31Z

Alright, that last push did it. I figured out all the problems (I hope). As of right now, the working/non working feature list is as follows

Working:

Windows PCH
Linux PCH
Linux Unity

Not Working

Windows Unity
Mac PCH

Untested

Mac Unity

I'm completely burnt out of staring at cmake problems and fighting with visual studio. It's good enough for my usecase right now, and I'll probably come back to this in a different PR to make cotire a more robust feature. I definitely don't think it should be advertised in docs until we know it's working right.

BearishSun · 2018-06-05T20:27:28Z

Sure, thanks for all the work. I'll merge when I get a moment to properly review everything so I don't rush it, sometimes this week. Then take a look at the mac stuff when I get more time.

BearishSun · 2018-06-06T16:02:08Z

Merged, f80c1c5. Thanks again.

cwfitzgerald added 7 commits May 31, 2018 16:07

Add preliminary cotire support

aee6a04

Fix clang pch issues

eb19718

Add colored ninja output; Enable clang header mitigations on AppleClang

58b7dc1

Fixed errors with PCH files on windows

556e77e

Silence other warnings

988c240

Changes with unity builds

7a5e54c

Enable conditional cotire

f391bca

cwfitzgerald mentioned this pull request Jun 4, 2018

Full Cotire Support GameFoundry/B3DFrameworkExamples#8

Closed

BearishSun reviewed Jun 4, 2018

View reviewed changes

Requested changes

46d5d07

Add ENABLE_COTIRE cache variable

fbb17df

cwfitzgerald added 3 commits June 4, 2018 19:42

[ci-skip] Work

acf07ee

[ci-skip] more work

9eaccae

[ci-skip] that bastard windows header

893ba88

FINALLY WORKS!

b88c686

BearishSun closed this Jun 6, 2018

cwfitzgerald deleted the cotire branch June 7, 2018 04:13

Conversation

cwfitzgerald commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwfitzgerald commented Jun 4, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BearishSun commented Jun 4, 2018

Uh oh!

cwfitzgerald commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BearishSun commented Jun 4, 2018

Uh oh!

cwfitzgerald commented Jun 4, 2018

Uh oh!

cwfitzgerald commented Jun 4, 2018

Uh oh!

cwfitzgerald commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BearishSun commented Jun 4, 2018

Uh oh!

cwfitzgerald commented Jun 4, 2018

Uh oh!

BearishSun commented Jun 4, 2018

Uh oh!

BearishSun commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwfitzgerald commented Jun 4, 2018 via email

Uh oh!

cwfitzgerald commented Jun 4, 2018

Uh oh!

cwfitzgerald commented Jun 5, 2018

Uh oh!

BearishSun commented Jun 5, 2018

Uh oh!

cwfitzgerald commented Jun 5, 2018

Uh oh!

cwfitzgerald commented Jun 5, 2018

Uh oh!

cwfitzgerald commented Jun 5, 2018

Uh oh!

BearishSun commented Jun 5, 2018

Uh oh!

BearishSun commented Jun 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cwfitzgerald commented Jun 4, 2018 •

edited

Loading

cwfitzgerald commented Jun 4, 2018 •

edited

Loading

cwfitzgerald commented Jun 4, 2018 •

edited

Loading

BearishSun commented Jun 4, 2018 •

edited

Loading