Skip to content

Add options to use scons caching for faster iteration.#838

Merged
Faless merged 1 commit into
godotengine:masterfrom
PapyChacal:scons-caching
Sep 19, 2022
Merged

Add options to use scons caching for faster iteration.#838
Faless merged 1 commit into
godotengine:masterfrom
PapyChacal:scons-caching

Conversation

@PapyChacal

Copy link
Copy Markdown
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
Copy Markdown
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

@Faless Faless left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread SConstruct Outdated
Comment thread SConstruct
Comment thread SConstruct Outdated
Comment thread binding_generator.py Outdated
Comment thread binding_generator.py Outdated

@Faless Faless left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread SConstruct Outdated
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
Copy Markdown
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"],

@PapyChacal PapyChacal Sep 19, 2022

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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

Faless commented Sep 19, 2022

Copy link
Copy Markdown
Contributor

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