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

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

Merged
merged 2 commits into from
Jun 18, 2024

Conversation

pjreiniger
Copy link
Contributor

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.

@pjreiniger pjreiniger requested review from a team as code owners June 16, 2024 03:17
Copy link
Contributor

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

@calcmogul
Copy link
Member

calcmogul commented Jun 16, 2024

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
Copy link
Contributor Author

pjreiniger commented Jun 16, 2024

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
Copy link
Member

calcmogul commented Jun 16, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

Gold856 commented Jun 16, 2024

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
Copy link
Member

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
Copy link
Contributor Author

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

@PeterJohnson PeterJohnson merged commit 66c0abb into wpilibsuite:main Jun 18, 2024
30 checks passed
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.

5 participants