wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.03k stars 607 forks source link

[pregen] Use pathlib in pre-generation scripts #6745

Closed pjreiniger closed 2 weeks ago

pjreiniger commented 2 weeks ago

This stems from this comment left on my other PR.

When I made the PWM generator, I copied the style of the other pregeneration tools. However the Path library is nice, and if that was requested in that PR, these other files should be updated for consistency. I also added type hints where possible, and snuck in command line arguments so that my bazel build can run the generation during a build.

In the cases where it existed, I removed the "only write if dirty" check. This isn't as crucial since the scripts don't get run during a build, and I thought it was better for the "least astonishment principle" if it always generates the files, as its name suggests.

github-actions[bot] commented 2 weeks ago

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

calcmogul commented 2 weeks ago

In the cases where it existed, I removed the "only write if dirty" check. This isn't as crucial since the scripts don't get run during a build, and I thought it was better for the "least astonishment principle" if it always generates the files

If the file contents didn't change, why would it need to rewrite the file? That's just making the build system do more work. wpiformat isn't run by the build system, yet it still only writes files that changed.

pjreiniger commented 2 weeks ago

Why would you run the script if the inputs haven't changed? Now that they are pre generated it is completely divorced from the gradle / cmake / bazel i(s smart enough to run it once and not overwrite the files if the diff is a no-op). The average user and builder will never run these scripts, either directly or indirectly by running any flavor of build.

hal/generate_usage_reporting.py, and wpimath/generate_quickbuf.py did not do a "is dirty check"

ntcore/generate_topics.py and wpimath/generate_numbers.py predate the pre-generation concept, it had to do a dirty check in order to not mess with the cmake builds. I have a feeling that */generate_hids.py was heavily inspired by the ntcore script and just copy pasta'd the write function.

calcmogul commented 2 weeks ago

Why would you run the script if the inputs haven't changed?

It's a principle thing. It's like two extra lines that saves the build system potential work. Whether the build runs the script or not is irrelevant.

pjreiniger commented 2 weeks ago

Selfishly, it actively interferes with bazel. It produces non hermeticity in the build. The inputs to the function don't change, but the outputs can, depending on if it has been run before.

Gold856 commented 2 weeks ago

ntcore/generate_topics.py and wpimath/generate_numbers.py predate the pre-generation concept, it had to do a dirty check in order to not mess with the cmake builds. I have a feeling that */generate_hids.py was heavily inspired by the ntcore script and just copy pasta'd the write function.

As the person who wrote the generate_hids.py scripts, I did, in fact, just copy the scripts from ntcore. Actually, I wrote all the scripts currently in main except for generate_numbers.py and generate_topics.py. Whether or not my scripts checked if the file was the same depended on whether or not I copied a script that checked if the file was same.

Starlight220 commented 2 weeks ago

Since there's an increasing number of these nearly-equivalent generation scripts, is there something we can do to reduce code duplication and ensure similarity?

I'm not saying to generate the generation scripts, but deduplicate them somehow.

pjreiniger commented 2 weeks ago

They all have their own snowflake bits, but there is probably some things that could be pulled into a library. I'd like to do that as a follow up after these two prs land