Skip to content

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Oct 6, 2025

Despite looking promising on-paper, this change proved to be much more trouble than it's worth. The reason for that was revealed when working on #108611, as it came down to SCons not tracking external includes properly. While that might've sufficed in isolation, this fix required a somewhat invasive and largely undocumented solution. Combined with it being in a stagnant/unreviewed state, the much safer option is to simply revert to the original implementation

This isn't a full revert, as there've been hotfixes surrounding this initial change since merging, so those are already applied. Beyond that, the sections that necessitated warning suppression pragmas now use the warning wrapper macros instead, as those would've been applied if they were still around

Note that this reimplements hardcoded -isystem flags to the repo, as those were around prior to this original PR. Meaning: the same issues that CPPEXTPATH revealed had already existed in the repo for many years, but either never came up or were addressed in an isolated context. As such, a followup PR will remove these as well, and apply the proper warning wrappers to the thirdparty includes as necessary

Bugsquad edit:

@Repiteo Repiteo added this to the 4.6 milestone Oct 6, 2025
@Repiteo Repiteo requested a review from akien-mga October 6, 2025 17:12
@Repiteo Repiteo requested review from a team as code owners October 6, 2025 17:12
@Repiteo Repiteo added bug topic:buildsystem topic:thirdparty regression cherrypick:4.5 Considered for cherry-picking into a future 4.5.x release labels Oct 6, 2025
@Repiteo Repiteo force-pushed the scons/revert-cppextpath branch 2 times, most recently from c793a7c to dd2d83a Compare October 6, 2025 17:40
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@akien-mga akien-mga merged commit 9052d31 into godotengine:master Oct 7, 2025
39 of 40 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.5.1.

@akien-mga akien-mga removed the cherrypick:4.5 Considered for cherry-picking into a future 4.5.x release label Oct 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants