-
-
Notifications
You must be signed in to change notification settings - Fork 597
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
Add options to use scons caching for faster iteration. #838
Conversation
I adding a dependency on the source generation too: now any modification to I also enforced source generation when using |
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.
Looks really good 🏆 , see review comments, there's also an extra deleted line before add_sources
which makes CI fail.
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.
One more style change required by static checks.
Also needs squashing, see https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase
SConstruct
Outdated
@@ -168,12 +168,19 @@ else: | |||
json_api_file = os.path.join(os.getcwd(), env["headers_dir"], "extension_api.json") | |||
|
|||
bindings = env.GenerateBindings( | |||
env.Dir("."), [json_api_file, os.path.join(env["headers_dir"], "godot", "gdnative_interface.h")] | |||
env.Dir("."), | |||
[json_api_file, os.path.join(env["headers_dir"], "godot", "gdnative_interface.h"), "binding_generator.py"] |
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.
One last change required by static checks (python formatting):
[json_api_file, os.path.join(env["headers_dir"], "godot", "gdnative_interface.h"), "binding_generator.py"] | |
[json_api_file, os.path.join(env["headers_dir"], "godot", "gdnative_interface.h"), "binding_generator.py"], |
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.
Just out of curiosity if you have time, I find this comma unsettling. What is the rationale for requiring it? (I added it anyway in the squash)
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.
It's the style enforced by black https://pypi.org/project/black/ .
I think it defaults to that style for functions where the ending parenthesis is on a new line.
I don't particularly like that style either, but it's totally valid python.
Sort the sets in source generation so they are generated consistently between runs; otherwise caching is useless.
2cbabef
to
8e717ac
Compare
Last formatting and squash done, thanks! |
Thanks! |
While working on godot-cpp I got tired of recompiling every object file when changing only some files' generation logic.
Thus this Pull Request: I added options to the SConstruct to leverage scons' caching capabilities. If activated, it stores every build output in the cache, and fetch it if the dependencies have the exact same content. With the option enable, recompiling after binding regeneration will only recompile objects where the generated sources have actually changed, and fetch all others from the cache.
This required to sort the sets used in binding_generator.py: otherwise the includes and forward-dcelaration are done in pseudo-random order, leading to different content despite being equivalent.
I didn't notice any impact on the generation time, but if this is a worry I can sort them only when caching is enabled.