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

Split Generated Source Into 26 files for Windows #633

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

samdeane
Copy link
Contributor

@samdeane samdeane commented Dec 18, 2024

[NOTE: this PR includes the changes from #615, so it would be easier to review once that one is merged]

The Problem

Windows is failing to build on the CI machine.

It appears to be due to the humungous size of the combined generated file, which I suspect causes some sort of exponential explosion in the compiler; the upshot of which is that it just hangs.

This PR attempts to fix that.

The Solution

Instead of generating a single enormous file, we generate 26 files SwiftGodotA.swift, SwiftGodotB.swift etc.

Each file contains all the types that start with that letter (or more specifically, all the types output by Printer instances whose names start with that letter).

This keeps the number of input files down to a manageable amount, but also the size of each file from getting too large.

If necessary we could use a different hash to produce more, smaller, files, but this seems to be enough to get the build to actually finish.

@samdeane samdeane force-pushed the fix-windows-build branch 2 times, most recently from d0c6d35 to c521766 Compare December 19, 2024 14:18
@samdeane samdeane marked this pull request as ready for review December 19, 2024 14:28
@samdeane
Copy link
Contributor Author

I still takes ~20 mins to build on Windows, but it does at least build now, and the minimal tests that don't need libgodot do actually link & run.

@migueldeicaza
Copy link
Owner

Could we undo the whitespace changes on the files that are unrelated to this? Like the EntryPoint changes have a lot of churn.

I am trying to merge a large set of internal changes for object ownership, and the whitespace changes every time become very time consuming.

@migueldeicaza
Copy link
Owner

Question, the generated split only applies to Windows, but on Unix we still keep all of our files the way we have them now?

@samdeane
Copy link
Contributor Author

Question, the generated split only applies to Windows, but on Unix we still keep all of our files the way we have them now?

That's the intention, yes.

@samdeane
Copy link
Contributor Author

Does Linux have a similar problem with path lengths & the linker?

I was actually thinking that I might try to add a Linux runner to the workflow (later, not as part of the PR) - but I wasn't sure what state the linux build was in.

@migueldeicaza
Copy link
Owner

I am not aware of issues with Linux, I think that would be fine.

@migueldeicaza
Copy link
Owner

The original comment states that I should merge firs the #615 change, but it seems like that one has the build broken, which this one fixes.

Should I just merge this one instead?

@migueldeicaza
Copy link
Owner

Btw, it would be really nice if we did not include whitespace changes on these PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants