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

Remove Plugin, Commit Generated Code #621

Closed

Conversation

samdeane
Copy link
Contributor

@samdeane samdeane commented Nov 29, 2024

Yes, I know. Committing generated code is not regarded as good practice (and generally I agree).

However, I believe that this situation is not the normal generated-code scenario. Furthermore, there are some other compelling reasons to do it.

TL;DR

The idealist in me loves the plugin, and thinks that it is definitely the right approach.

The pragmatist thinks that until Swift works a bit better, especially on Windows, checking in the generated code is more sensible.

This Time It's Different, Honest

In my mind, the normal scenario is:

  • the code is generated from a source file or files we have control over (eg a database schema)
  • the source for the generated code is changed frequently, or at least, at the same cadence as any other part of the source code

Another common argument against checking in generated code is that it has the potential to mess up the diff of otherwise unrelated changes.

This situation is different because:

  • the source for generation only changes when Godot changes, which is infrequent
  • even then, we only actually need to regenerate if we decide to support the additional Godot API, which may not be required for every Godot release
  • when we do regenerate because of a change to the Godot API, having the diff is positively useful
  • the other time we regenerate is because we've changed the generator; in this case, having the diff is also positively useful
  • Github has tools for suppressing the diff of generated files in PRs by default; so we only have to view them when we want to.

Other Positive Reasons To Do This

Faster Build Times

Swift doesn't have to check to see if it needs to run the generator each time we build.

Although in theory this check should be fairly fast, and the generator should only run once until you do a clean build, in practice the check seems to take a noticeable amount of time, and a full re-run of the generator seems to happen quite often. This is probably down to bugs in SwiftPM, but we can't fix those 🤷.

Much Faster Windows Build Times

Doing this, we also get around the ugly single-file hack for Windows (due to it not being able to cope with more than 32Kb of arguments to the generator tool).

The Swift 6 toolchain seems to completely die when trying to compile the single-file on Windows.

Removing this makes the Windows library build in a more reasonable time on my machine. I wouldn't say it's fast, but it does actually finish rather than just locking up forever.

Easier Hacking

For what it's worth, this also makes it easier to hack on the generated code. This could be useful for prototyping changes to the generator; rather than having to implement them in the generator, we can just modify a single generated file and test it in isolation.

Avoid SwiftPM Generator Bugs

Like this one, which is a massive PITA: swiftlang/swift-package-manager#7930

@samdeane samdeane marked this pull request as ready for review November 29, 2024 15:02
@migueldeicaza
Copy link
Owner

This is not going to happen, but I might consider a "SwiftGodotDigested" module that contains that version.

@samdeane
Copy link
Contributor Author

samdeane commented Dec 5, 2024

This is not going to happen

This makes me sad, but it is of course your prerogative.

I might consider a "SwiftGodotDigested" module that contains that version.

I think that I'll just stay in my own fork for now with this change in it, and base my work off that -- at least until Apple fix some bugs with code generator plugins so that they don't run all the damn time, and their output shows up in Xcode/VSCode, etc, etc.

@samdeane samdeane closed this Dec 5, 2024
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