google
Avoid outputting Python files for already generated types
Outputting Python files for already generated types may overwrite previously generated code in situations where flabuffer files are included, rendering them unusable. It can also lead to undesired extra files when including shared flatbuffer definitions from other namespaces.
Partially reverts changes from #8292 Fixes #8490
CC @anton-bobukh, I'm not sure what the intention was for adding the empty Python files for generated types, but it causes a pretty major regression. See #8490 for more info.
@dbaileychess would you mind taking a look at this PR? This Python regression has meant that I have been unable to update to recent versions of flatbuffers, and I'd imagine it is the same for others as well. I am open to a different approach if you have another idea, but looking through the code I can't think of a situation where outputting a file for an already generated type would be desirable.
@dbaileychess Pinging you again as there has been a new release yesterday. This regression has been present since the v24.12.23 release, and has rendered projects that generate for Python largely unusable when flatbuffers with include statements are used. I'm open to suggestions if you feel that this fix isn't appropriate, but I would like to get the regression fixed so it's possible to use newer releases.
@EmixamPP these were added in #8292 which introduced the problem. The cause of the regression is that commit chose to output empty files (apart from comments) for any "already generated" types, which occur when you include another .fbs file. This can lead to both:
- Overwriting previously generated files for those types with empty ones.
- Adding extra files on the filesystem you don't want, such as including a .fbs file for another library (in a different namespace) where you will import them from a different location.
This commit reverts to the previous behavior of ignoring "already generated" types, and as a result removes these empty files that did not exist previously.
CC @aardappel as I see you've merged in a couple PRs recently. I'm hoping to get some sort of fix in, or at least start a discussion if this isn't the proper resolution, as the last usable release is now over a year old.
I don't know too much about Python, but this does seem to make sense to me. I'll let the CI run, then we can merge unless Python users have objections.
@dbaileychess
@aardappel while the CI failed, the job is unrelated to my changes and also failed the last two merges to master.
In terms of objections, I pinged @anton-bobukh (who put in the original change that introduced the regression) a few months ago, but no response as of yet. This has been open for some time without any objections stated so far.
@aardappel I see there has been some recent activity. Would it be possible to merge in this fix as well? There certainly haven't been any objections yet in the 5 months this PR has been open, though I am open to alternatives if there is a better way to resolve the regression.
Thanks!
I'm not sure what's going on with the Kotlin build. It's not showing any useful logs, not sure if I just don't have permissions, but seeing that it's failing in "gradle/wrapper-validation-action" and the rest of the builds succeeded I'm thinking it's likely unrelated to my changes.