SCons: Remove transitive includes in `libc++` by Repiteo · Pull Request #111767 · godotengine/godot

Repiteo · 2025-10-17T17:25:43Z

Helps address [TRACKER] Compile time issues caused by excessive includes #111218
Supersedes Improve compile time with libc++ by defining _LIBCPP_REMOVE_TRANSITIVE_INCLUDES in typedefs.h. #111557

I really like the idea of the above PR, but the implementation via typedefs.h would be inconsistent. It relies on all stdlib headers being defined after this header, which is unlikely when it's usually sorted so late in header chains & isn't a universal include for our repo. In order to guarantee this is applied for ALL scripts equally, the define is setup via SCons instead.

While this does the trick of catching overlooked cases early, that's proving to be a bit of a double-edged sword. Most notably: some thirdparty headers are caught in the crossfire, which I'll be gradually cleaning up over the next few days. This implementation currently excludes thirdparty compilation files, but could be expanded to include them as well, given we're already having to touch thirdparty code to some extent.

Ivorforce

When preparing #111557, I initially wanted to go for a SConstruct based approach as well.
However, I got caught up on 2 problems:

I don't know an easy way to query the build system whether libc++ is in use.
Editing SConstruct also affects thirdparty libraries, some of which break.

It seems like you've run into both as well, and decided to tackle them instead.
Your approach should work everywhere, but ends up a bit more complicated than #111557, and has to make some 'educated guesses' about when libc++ is used.

My approach (changing typedefs.h) is stupid simple. But it doesn't help in all compile units (depending on include order), and comes with a small risk of unexpected missing includes in headers, depending on include order of the compile unit.

I'd be fine with choosing either. Maybe time for more people to weigh in?

Ivorforce

Overall, after thinking about it some more, I think i prefer your approach.
But maybe we can simplify it some more?

Instead of patching 3rd party libraries, can we just selectively disable _LIBCPP_REMOVE_TRANSITIVE_INCLUDES on those that don't support it? That seems easier to maintain.

Ivorforce · 2025-10-19T09:07:13Z

SConstruct

        env.AppendUnique(CCFLAGS=["-fansi-escape-codes"])

+# Reduce transitive includes.
+if (methods.using_clang(env) and not env.msvc) or methods.using_emcc(env) or "-stdlib=libc++" in env["CXXFLAGS"]:


How sure are we that this works and won't break?
I suppose the worst that happens if it's wrong is that we don't save on compile time, or define a useless macro. But still, it feels a little weird to hardcode this.

Yeah, I suppose it isn't an absolute guarantee. The only certainty is msvc will NOT support this.

I'll just add the define universally then

Makes sense to me. Unless long compile commands are a problem, I don't see an actual downside to this.

Ivorforce · 2025-10-19T09:08:09Z

methods.py

+    if "_LIBCPP_REMOVE_TRANSITIVE_INCLUDES" in self["CPPDEFINES"]:
+        self["CPPDEFINES"] = [x for x in self["CPPDEFINES"] if x != "_LIBCPP_REMOVE_TRANSITIVE_INCLUDES"]


Why remove the flag when warnings are disabled?

disable_warnings is a bit of a misnomer; it's more "prepare thirdparty libraries". That is: this would disable the define for compiled thirdparty code

Repiteo · 2025-10-24T17:52:42Z

Got this simplified, albeit in the other direction. That is: instead of directly excluding _LIBCPP_REMOVE_TRANSITIVE_INCLUDES under certain circumstances or trying to include it granularly, I'll just apply it universally. This meant that even more thirdparty code had to be given patches, but it only affected libraries that already recieved patches, so this isn't introducing any "new" friction. The one exception is Jolt, but that's simply integrating an upstream change, so it too isn't adding any new friction

Ivorforce

Would it be possible to disable / remove the macro from the libraries that error instead of adding patches?

Repiteo · 2025-10-24T18:42:37Z

Only partially. That's what the disable_warnings() function was doing before. However, anything calling thirdparty headers, espcially in headers, could still need patches. Otherwise we're having the same problem of system headers potentially using transitive includes

Ivorforce · 2025-10-24T20:49:28Z

Only partially. That's what the disable_warnings() function was doing before. However, anything calling thirdparty headers, espcially in headers, could still need patches. Otherwise we're having the same problem of system headers potentially using transitive includes

Makes sense.
Then I got a new idea: How about we create our own wrapper headers that fix the header includes for them? Something like this:

// "thirdparty/jolt/wrapper.h

// _LIBCPP_REMOVE_TRANSITIVE_INCLUDES fixes
#include <type_traits>

#include "‎thirdparty/jolt_physics/Jolt/Core/Core.h‎"

We'd include this file instead of Jolt/Core/Core.h, and could disable the _LIBCPP_REMOVE_TRANSITIVE_INCLUDES for the Jolt library (removing the need for the patches).

Sorry about being so picky about this. But I'm thinking, this is 'just' a build performance PR after all. I'd feel a lot more comfortable if we could find a way to introduce the macro without increasing our maintenance cost / risk of future build failures with these libraries.

Repiteo · 2025-10-24T22:16:43Z

If Jolt in particular is a hangup, we could always hold out for an upstream sync where the change is already in place. Otherwise it'd mean editing a lot of files for a wrapper that'd be redundant not too long after

More likely you're just wanting to cover your bases about not causing potential breakage. To that end, I just added a new SCons flag: limit_transitive_includes. It's enabled by default, but disabling it will exclude the define

Ivorforce · 2025-10-24T22:31:48Z

More likely you're just wanting to cover your bases about not causing potential breakage. To that end, I just added a new SCons flag: limit_transitive_includes. It's enabled by default, but disabling it will exclude the define

Yes, I mainly wouldn't want this optimization to cause problems or maintenance effort down the line.
The option to disable this is a great idea. But it won't help people who run into build problems because they won't be able to connect the build problem to the build option on their own. The only really safe option I'd see is disabling it by default, though that would mean nearly nobody would benefit from it...

The Jolt example above was just serving as an example to explain a possible alternative to the patches.
Maybe I'm too hesitant to add in patches? I don't know our policy for them. If it's something we're normally happy to take for this kind of improvement I have no further complaints.

thirdparty/mingw-std-threads/mingw.condition_variable.h

Repiteo · 2025-10-24T23:43:54Z

thirdparty/basis_universal/patches/0006-explicit-includes.patch

Turns out 0005-msvc-include-ctype.patch was excluded from the thirdparty README, likely from sharing the 0005 prefix and being erroneously excluded in a rebase. Given it's already just adding a header, I consolidated it into this new patch file

Ivorforce

I tested this change and it compiled successfully, ~10% faster than master.
Code looks good to me now. I appreciate simplifying the implementation to this level!

I really hope that this won't cause too many issues down the line (I can imagine there will be a few), but the 10% compile time boost is worth it in my opinion.
If it ends up being too frail in practice, we could switch to transitive libc++ by default, and support non-transitive on a best-effort basis.

(I'm actually surprised at how few patches are required to get this working. I would have fully expected we'd need to disable this optimization for 3rd party libs.)

akien-mga

Seems fine.

Could you propose these changes upstream to the various libraries patched here? So hopefully they get merged and we can drop our patches eventually.

Repiteo added this to the 4.x milestone Oct 17, 2025

Repiteo requested a review from a team as a code owner October 17, 2025 17:25

Repiteo added the enhancement label Oct 17, 2025

Repiteo requested a review from a team as a code owner October 17, 2025 17:25

Repiteo added topic:core topic:buildsystem performance labels Oct 17, 2025

Repiteo marked this pull request as draft October 17, 2025 17:25

Repiteo removed request for a team October 17, 2025 17:26

Repiteo force-pushed the scons/libcpp-transitive branch from 12b1a1b to 97562b1 Compare October 17, 2025 17:32

Repiteo added the topic:thirdparty label Oct 17, 2025

Repiteo marked this pull request as ready for review October 17, 2025 18:43

Repiteo requested a review from a team as a code owner October 17, 2025 18:43

Repiteo requested review from a team and removed request for a team October 17, 2025 18:44

Repiteo mentioned this pull request Oct 17, 2025

Improve compile time with libc++ by defining _LIBCPP_REMOVE_TRANSITIVE_INCLUDES in typedefs.h. #111557

Closed

Ivorforce reviewed Oct 18, 2025

View reviewed changes

Ivorforce reviewed Oct 19, 2025

View reviewed changes

Repiteo force-pushed the scons/libcpp-transitive branch from 97562b1 to 5f6082c Compare October 24, 2025 17:35

Ivorforce reviewed Oct 24, 2025

View reviewed changes

Repiteo force-pushed the scons/libcpp-transitive branch from 5f6082c to 83f1008 Compare October 24, 2025 22:14

Ivorforce reviewed Oct 24, 2025

View reviewed changes

thirdparty/mingw-std-threads/mingw.condition_variable.h Show resolved Hide resolved

akien-mga self-requested a review October 24, 2025 23:08

Repiteo force-pushed the scons/libcpp-transitive branch 2 times, most recently from 821b5e8 to aa7ffce Compare October 24, 2025 23:41

Repiteo commented Oct 24, 2025

View reviewed changes

SCons: Remove transitive includes in libc++

ad02128

Repiteo force-pushed the scons/libcpp-transitive branch from aa7ffce to ad02128 Compare November 23, 2025 17:10

Ivorforce approved these changes Nov 23, 2025

View reviewed changes

akien-mga approved these changes Dec 3, 2025

View reviewed changes

Repiteo modified the milestones: 4.x, 4.6 Dec 4, 2025

Repiteo merged commit 63e14e1 into godotengine:master Dec 4, 2025
20 checks passed

Repiteo deleted the scons/libcpp-transitive branch December 4, 2025 03:07

		if "_LIBCPP_REMOVE_TRANSITIVE_INCLUDES" in self["CPPDEFINES"]:
		self["CPPDEFINES"] = [x for x in self["CPPDEFINES"] if x != "_LIBCPP_REMOVE_TRANSITIVE_INCLUDES"]

Uh oh!

Conversation

Repiteo commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivorforce left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivorforce left a comment

Choose a reason for hiding this comment

Uh oh!

Ivorforce Oct 19, 2025

Choose a reason for hiding this comment

Uh oh!

Repiteo Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivorforce Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Ivorforce Oct 19, 2025

Choose a reason for hiding this comment

Uh oh!

Repiteo Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Repiteo commented Oct 24, 2025

Uh oh!

Ivorforce left a comment

Choose a reason for hiding this comment

Uh oh!

Repiteo commented Oct 24, 2025

Uh oh!

Ivorforce commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Repiteo commented Oct 24, 2025

Uh oh!

Ivorforce commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Repiteo Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivorforce left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akien-mga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Repiteo commented Oct 17, 2025 •

edited

Loading

Ivorforce left a comment •

edited

Loading

Repiteo Oct 24, 2025 •

edited

Loading

Ivorforce commented Oct 24, 2025 •

edited

Loading

Ivorforce commented Oct 24, 2025 •

edited

Loading

Repiteo Oct 24, 2025 •

edited

Loading

Ivorforce left a comment •

edited

Loading