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

[CI] Add concurrency and scons caching. #853

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

Faless
Copy link
Contributor

@Faless Faless commented Sep 19, 2022

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

Faless commented Sep 19, 2022

Concurrency seems to work: https://github.com/godotengine/godot-cpp/actions/runs/3082763931

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

Faless commented Sep 19, 2022

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

Copy link
Member

@akien-mga akien-mga left a comment

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

Faless commented Sep 19, 2022

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

Faless commented Sep 19, 2022

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

Faless commented Sep 19, 2022

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

Faless commented Sep 19, 2022

Confirmed working cached run: https://github.com/godotengine/godot-cpp/actions/runs/3083410183

@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