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

SCons: Properly NoCache all text files #101008

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Jan 1, 2025

Turns out, reading files in the cache becomes prohibitively slow once it achieves moderate size. Instead of attempting to optimize what was already a bandage solution, this tackles the root issue of text files being incorrectly cached. NoCache was never properly applied to several builders, most notably all .glsl.gen.h files, so this sweeps through and ensures they're actually enforced. Consequently, the dedicated text cache check has been entirely discarded; there's no reasonable situation where that will occur anymore, and the performance hit isn't worth it in the first place.

@Repiteo Repiteo added this to the 4.4 milestone Jan 1, 2025
@Repiteo Repiteo requested a review from AThousandShips January 1, 2025 21:55
@Repiteo Repiteo requested a review from a team as a code owner January 1, 2025 21:55
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.

Seems fine. Just to be safe I'd merge after beta1 so we don't risk introducing a buildsystem regression at the last minute for beta1.

@Repiteo Repiteo force-pushed the scons/nocache-all-text branch from 5ac55f5 to 73278bf Compare January 16, 2025 23:00
@Repiteo Repiteo merged commit fd88acc into godotengine:master Jan 16, 2025
20 checks passed
@AThousandShips
Copy link
Member

We've got some spurious errors in compile output in CI following this, see https://github.com/godotengine/godot/actions/runs/12819416963/job/35765666274#step:11:1959

Some cases completely overflow the log

print_error(f'Failed to remove cache file "{file}"; skipping.')
count -= 1
if verbose:
print_info(f"Purged {count} file{'s' if count else ''} from cache.")
Copy link
Member

Choose a reason for hiding this comment

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

Identified the issue, the loop should break here, will write up a fix

@AThousandShips
Copy link
Member

@Repiteo Repiteo deleted the scons/nocache-all-text branch January 17, 2025 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants