-
-
Notifications
You must be signed in to change notification settings - Fork 21k
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
added Scons modules #define generator #32453
added Scons modules #define generator #32453
Conversation
Generated file describes what modules are available |
There's already code to define The main reason for that is that passing all There's also All in all this is more something for thorough proposal on how to refactor the way module dependencies are handled. Your PR lacks description so it's not clear what you want to achieve, but if the aim is just to allow disabling regex support, a simpler fix would be to define append |
scene/gui/rich_text_label.cpp
Outdated
@@ -29,6 +29,7 @@ | |||
/*************************************************************************/ | |||
|
|||
#include "rich_text_label.h" | |||
#include "../../modules/modules_enabled.gen.h" |
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.
This should be below together with other includes. It should also start from the root, we don't use relative paths.
modules/regex/regex.h
should not be included if the module is disabled.
I wanted to do this while attempting to not compile Bullet-related code if the module is disabled. In fact I've successfully done this, except I realized that there's quite a bunch of places already to wrap with MODULE_BULLET_ENABLED. The idea has originated from #32216.
Also GDSCRIPT_ENABLED. I think this needs godot-proposal again. 🤔 |
I like this as an alternative to passing MODULE_*_ENABLED (but just to be clear @jeronimo-schreyer works for me). I never really liked adding those defines outside of modules/ and in reality almost none of the modules use them, except that ones that use other modules, like ogg and vorbis, and it makes sense for code outside modules/ to sometimes need to know if a module is there. This header would allow anyone to know about the modules, without having to pollute the compile command (and causing a full rebuild when one module is enabled/disabled). This doesn't remove the defines from the command line to keep compatibility, it's just to address the issue of regexp used in the RichTextLabel (regexp can't be used in consoles because it has a jit) |
modules/SCsub
Outdated
for x in env.module_list: | ||
if (x in env.disabled_modules): | ||
continue | ||
env_modules.Append(CPPDEFINES=["MODULE_" + x.upper() + "_ENABLED"]) | ||
SConscript(x + "/SCsub") | ||
|
||
env.Command("modules_enabled.gen.h", [], generate_modules_enabled) |
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.
This should use CommandNoCache
so that it doesn't get cached on CI, but re-generated each time.
The method should also be called with run_in_subprocess
to avoid build issues on Windows. See #17595 for context and core/SCsub
, main/SCsub
and editor/SCsub
for examples of how it's done for existing .gen.h
files.
IMO this change is fine, but then the old style should be removed, we don't need to keep both. It only breaks compatibility for thirdparty modules that would happen to reuse our own |
Apart from above comments, I'd like to see this changed:
|
There's one potential issue with this approach though, which is that any time a module is enabled or disabled, the header will change and this will force recompilation of all the files that include it. This could be particularly problematic if we also remove the hardcoded So we may need to tread cautiously here as our incremental builds are already a bit on the edge. |
I squashed previous commits into one |
Superseded by #35963. Thanks for the contribution nevertheless, which was used as basis for that bigger PR. |
No description provided.