Skip to content
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

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

PapyChacal
Copy link
Contributor

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.

@PapyChacal
Copy link
Contributor Author

I adding a dependency on the source generation too: now any modification to binding_generator.py triggers the source generation, as well as modifying extension_api.json or the header, meaning we don't have to use generate_bindings=yes so much.

I also enforced source generation when using generate_bindings=yes, cache or no cache.

@Calinou Calinou added the enhancement This is an enhancement on the current functionality label Sep 13, 2022
Copy link
Contributor

@Faless Faless left a 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.

SConstruct Outdated Show resolved Hide resolved
SConstruct Show resolved Hide resolved
SConstruct Outdated Show resolved Hide resolved
binding_generator.py Outdated Show resolved Hide resolved
binding_generator.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Faless Faless left a 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"]
Copy link
Contributor

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):

Suggested change
[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"],

Copy link
Contributor Author

@PapyChacal PapyChacal Sep 19, 2022

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)

Copy link
Contributor

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.
@PapyChacal
Copy link
Contributor Author

Last formatting and squash done, thanks!

@Faless Faless self-requested a review September 19, 2022 11:59
@Faless Faless merged commit 0b87aaa into godotengine:master Sep 19, 2022
@Faless
Copy link
Contributor

Faless commented Sep 19, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants