Skip to content

SCons: Add CPPEXTPATH for external includes #1756

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

Merged
merged 1 commit into from
Apr 16, 2025

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Apr 2, 2025

Will enable SCons scripts to declare certain includes are external/system explicitly, for the sake of supressing warnings. Not too relevant in this isolated context, where we don't really care about warnings, but will prove useful for anything deriving from us (eg: the main repo's GDExtension text servers)

@Repiteo Repiteo added enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup labels Apr 2, 2025
@Repiteo Repiteo requested a review from a team as a code owner April 2, 2025 17:38
Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

This seems to match the Godot code, but it's a little beyond my scons and build knowledge to really review. If someone more knowledgeable can take a look, that'd be amazing; otherwise, I trust @Repiteo's expertise in this area and would be fine to go forward even without :-)

Does this need equivalent cmake changes? cc @enetheru

@Repiteo Repiteo force-pushed the scons/external-includes branch 3 times, most recently from 4a748bf to cbd32a6 Compare April 3, 2025 17:42
@enetheru
Copy link
Collaborator

enetheru commented Apr 3, 2025

Does this need equivalent cmake changes? cc @enetheru

No changes needed for CMake.
This type of thing was inside the cmake solution from before I touched it, even has an option to turn it on and off. Though come to think of it, in cmake its typically managed by the consumer, not the dependency, so not sure what utility it has there.

@Repiteo Repiteo force-pushed the scons/external-includes branch from cbd32a6 to 30bfa6f Compare April 4, 2025 14:40
@Repiteo
Copy link
Contributor Author

Repiteo commented Apr 4, 2025

Though come to think of it, in cmake its typically managed by the consumer, not the dependency, so not sure what utility it has there.

Oh? I really need to do a proper deep-dive into CMake at some point to get a feel for how that buildsystem operates, because that might be an even better method than what I've attempted here! I'm still sticking with this implementation to match the main repo for now, but that gives me something to think on.

@dsnopek
Copy link
Collaborator

dsnopek commented Apr 4, 2025

Though come to think of it, in cmake its typically managed by the consumer, not the dependency

Hm, yeah, from the perspective of the consumer, godot-cpp's includes are themselves external

@enetheru
Copy link
Collaborator

enetheru commented Apr 5, 2025

Oh? I really need to do a proper deep-dive into CMake at some point to get a feel for how that buildsystem operates,

Happy to get on a screen share and talk you through my undertanding, and provide links to useful videos and resources.

@dsnopek
Copy link
Collaborator

dsnopek commented Apr 15, 2025

@Repiteo Does this make any includes external, or just add the ability to do so?

@Repiteo
Copy link
Contributor Author

Repiteo commented Apr 16, 2025

This only adds the ability to do so. I would be able to make the Godot text servers GDExtension implementations use them though

Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

I trust @Repiteo's expertise in this area :-)

@dsnopek dsnopek merged commit 2a8d218 into godotengine:master Apr 16, 2025
11 checks passed
@Repiteo Repiteo deleted the scons/external-includes branch April 26, 2025 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.3 cherrypick:4.4 enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants