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

Merge the two canvas code generation scripts #50031

Open
gsnedders opened this issue Jan 10, 2025 · 1 comment
Open

Merge the two canvas code generation scripts #50031

gsnedders opened this issue Jan 10, 2025 · 1 comment
Labels

Comments

@gsnedders
Copy link
Member

I previously did this in #33613, but lacking review, this never landed. Since then, the two scripts have been substantially modified (https://github.com/web-platform-tests/wpt/commits/master/html/canvas/tools/gentestutils.py, https://github.com/web-platform-tests/wpt/commits/master/html/canvas/tools/gentestutilsunion.py).

I'm slightly surprised/saddened at the size of cc1bc6f (from #40176), modifying the one build script to make the output between the two scripts more similar, and changing 1502 generated files, given my old PR managed to merge the two scripts with only 138 generated files changed in total. Maybe by the point that it landed it was impossible to have a smaller diff, but it still feels very unfortunate that we went from merging the scripts being something that was done and didn't change many generated files at all, to something where even reducing the diff between the two scripts has involved changing an order of magnitude more generated files.

cc @graveljp, who seems to have done most of the updates since my early 2022 PR.

@gsnedders gsnedders added the html label Jan 10, 2025
@graveljp
Copy link
Contributor

Hi,

I'm sorry I didn't see #33613 before. Things have changed a lot since 2022. It is indeed unfortunate that these scripts got forked in the first place. I originally tried to maintain them in sync as I improved them, but this was like pulling a dead weight since our goal was to delete the old generator. In hindsight, merging them would indeed have been a better idea, but it's too late for that, the new script generator has seen a lot of improvements and new features in the last years.

The old test generator is almost unused now. The only tests that it still generates are the ones in html/canvas/tools/yaml/element/meta.yaml and .../offscreen/meta.yaml. I migrated one of these meta test in 215c5cc. I can try to find some time soon to complete that migration and delete the old generator.

Regarding the number of files changed, we have not been concerned with the impact on generated files. The whole point of a code generator is to be able to generate or modify lots of redundant code with a minimal configuration. The fact that the generated files are submitted in the repository is a mistake in my opinion. It's causing lots of problems, like noise in code review or generated files getting out of sync with the generator. It would be better if file were generated as part of the build or by the test runner.

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

No branches or pull requests

2 participants