Skip to content

[CI] Add concurrency and scons caching.#853

Merged
Faless merged 1 commit into
godotengine:masterfrom
Faless:build/4.x_ci_cache
Sep 19, 2022
Merged

[CI] Add concurrency and scons caching.#853
Faless merged 1 commit into
godotengine:masterfrom
Faless:build/4.x_ci_cache

Conversation

@Faless

@Faless Faless commented Sep 19, 2022

Copy link
Copy Markdown
Contributor

Followup on #838 .

Should make @akien-mga happy that we don't needlessly clog CI anymore 😉

@Faless Faless added enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup labels Sep 19, 2022
@Faless Faless added this to the 4.0 milestone Sep 19, 2022
@Faless

Faless commented Sep 19, 2022

Copy link
Copy Markdown
Contributor Author

@Faless
Faless requested a review from akien-mga September 19, 2022 13:28
@Faless

Faless commented Sep 19, 2022

Copy link
Copy Markdown
Contributor Author

I'll test cache by force-pushing once CI has finished.

@akien-mga akien-mga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great!

@akien-mga

Copy link
Copy Markdown
Member

BTW, we could also consider removing generate_bindings=yes to let it decide if it's needed (if it can restore that cache properly), but if so we should look into this potential bug with the dependency chain: #849 (comment)

@Faless

Faless commented Sep 19, 2022

Copy link
Copy Markdown
Contributor Author

we could also consider removing generate_bindings=yes to let it decide if it's needed

Oh, I missed that, yes, we definitely should, or caching will actually not work (see #838 (comment)).
But I think the current config should properly detect changes 100% of the times, now that binding_generator.py is also checked for changes, and that we have a check that the the file list is correct.

@Faless

Faless commented Sep 19, 2022

Copy link
Copy Markdown
Contributor Author

Oh, I missed that, yes, we definitely should, or caching will actually not work

(or, more correctly, the "debug" step will override it every time).

@PapyChacal

Copy link
Copy Markdown
Contributor

With the scons caching and generate_bindings=yes, the generated sources won't be cached directly, but their corresponding compiled object files will.
Meaning that on the next build with both cache and binding generation activated, the gen/* sources will indeed be generated again, but scons can see that they were generated to the same content and fetch the corresponding .o from cache, in negligible time.

That was the point of using the cache with content comparison: otherwise, the usual modification time check would have been enough 🙂

@Faless

Faless commented Sep 19, 2022

Copy link
Copy Markdown
Contributor Author

That was the point of using the cache with content comparison: otherwise, the usual modification time check would have been enough slightly_smiling_face

Ah right, my bad.
That is really cool! I guess that explains why I was seeing objects actually being retrieved from cache in the new build 😅 .

I guess we could leave the generate_bindings=yes for safety then since it won't impact the compilation time, but I'm not sure in which cases it might miss the proper generation with the recent fixes. I guess it's a question about taking the safest approach VS checking that the generate_bindings=no works as it should (CC @akien-mga )

@akien-mga

Copy link
Copy Markdown
Member

I'd say let's go with generate_bindings=no (or more like not specifying it) so that we test the recommended workflow (since that's the default value). So if for some reason the dependency chain is made incomplete again in the future, we'd catch it with failing CI builds.

@Faless

Faless commented Sep 19, 2022

Copy link
Copy Markdown
Contributor Author

@Faless
Faless merged commit a330342 into godotengine:master Sep 19, 2022
@Faless
Faless deleted the build/4.x_ci_cache branch September 19, 2022 17:03
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 topic:buildsystem Related to the buildsystem or CI setup

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants